Re: [FFmpeg-devel] [PATCH v12 2/4] avcodec/libjxl: add Jpeg XL decoding via libjxl

[Anton Khirnov <anton@khirnov.net>]

Quoting Leo Izen (2022-04-02 22:12:08)
> +        case JXL_DEC_COLOR_ENCODING:
> +            av_log(avctx, AV_LOG_DEBUG, "COLOR_ENCODING event emitted\n");
> +            jret = JxlDecoderGetICCProfileSize(ctx->decoder, &ctx->jxl_pixfmt, JXL_COLOR_PROFILE_TARGET_ORIGINAL, &ctx->iccp_len);

Does iccp_len need to be a context variable? Seems to me it's only used
in this block.

> +            if (jret == JXL_DEC_SUCCESS && ctx->iccp_len > 0) {
> +                av_buffer_unref(&ctx->iccp);
> +                av_assert2(!ctx->iccp);

Useless assert, you can rely on basic functions to do what they should.

> +                ctx->iccp = av_buffer_alloc(ctx->iccp_len);
> +                if (!ctx->iccp)
> +                    return AVERROR(ENOMEM);
> +                jret = JxlDecoderGetColorAsICCProfile(ctx->decoder, &ctx->jxl_pixfmt, JXL_COLOR_PROFILE_TARGET_ORIGINAL, ctx->iccp->data, ctx->iccp_len);
> +                if (jret != JXL_DEC_SUCCESS)
> +                    av_freep(&ctx->iccp);

av_buffer_unref?
> +            }
> +            continue;
> +        case JXL_DEC_FRAME:
> +        case JXL_DEC_NEED_IMAGE_OUT_BUFFER:
> +            /*
> +             * We don't do this at basic info time
> +             * because it will happen again when we
> +             * rewind anyway
> +             */
> +            av_log(avctx, AV_LOG_DEBUG, "%s event emitted\n", jret == JXL_DEC_FRAME ? "FRAME" : "NEED_IMAGE_OUT_BUFFER");
> +            ret = ff_get_buffer(avctx, frame, 0);

Is it absolutely guaranteed that this will always happen before
JXL_DEC_FULL_IMAGE in the same libjxl_decode_frame() invocation?

Can it happen that you get JXL_DEC_NEED_IMAGE_OUT_BUFFER/JXL_DEC_FRAME,
then JXL_DEC_NEED_MORE_INPUT, which causes you to return, then
JXL_DEC_FULL_IMAGE on the next libjxl_decode_frame() call?

> +        case JXL_DEC_SUCCESS:
> +            av_log(avctx, AV_LOG_DEBUG, "SUCCESS event emitted\n");
> +            /*
> +             * The file has finished decoding
> +             * reset the decoder to let us
> +             * reuse it again for the next image
> +             */
> +            JxlDecoderReset(ctx->decoder);
> +            libjxl_init_jxl_decoder(avctx);
> +            buf = avpkt->data;
> +            remaining = avpkt->size;

Why reset buf? The same data is not going to be decoded again, is it?

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".