[FFmpeg-devel] [PATCH v8 1/2] avcodec/libjxldec: properly tag output colorspace

[Niklas Haas]

On Thu, 23 Jun 2022 16:02:08 +0200 Anton Khirnov <anton at khirnov.net> wrote:
> Quoting Leo Izen (2022-06-02 04:14:11)
> > +    case JXL_TRANSFER_FUNCTION_GAMMA:
> > +        if (jxl_color->gamma > 2.199 && jxl_color->gamma < 2.201)
> > +            return AVCOL_TRC_GAMMA22;
> > +        else if (jxl_color->gamma > 2.799 && jxl_color->gamma < 2.801)
> > +            return AVCOL_TRC_GAMMA28;
> > +        else
> > +            av_log(avctx, AV_LOG_WARNING, "Unsupported gamma transfer: %f\n", jxl_color->gamma);
> > +        break;
> > +    default:
> > +        av_log(avctx, AV_LOG_WARNING, "Unknown transfer function: %d\n", jxl_color->transfer_function);
> > +    }
> > +
> > +    return AVCOL_TRC_UNSPECIFIED;
> > +}
> > +
> >  static int libjxl_decode_frame(AVCodecContext *avctx, AVFrame *frame, int *got_frame, AVPacket *avpkt)
> >  {
> >      LibJxlDecodeContext *ctx = avctx->priv_data;
> >      uint8_t *buf = avpkt->data;
> >      size_t remaining = avpkt->size, iccp_len;
> >      JxlDecoderStatus jret;
> > +    JxlColorEncoding jxl_color;
> >      int ret;
> >      *got_frame = 0;
> >  
> > @@ -189,16 +249,80 @@ static int libjxl_decode_frame(AVCodecContext *avctx, AVFrame *frame, int *got_f
> >              continue;
> >          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, &iccp_len);
> > -            if (jret == JXL_DEC_SUCCESS && iccp_len > 0) {
> > -                av_buffer_unref(&ctx->iccp);
> > -                ctx->iccp = av_buffer_alloc(iccp_len);
> > -                if (!ctx->iccp)
> > -                    return AVERROR(ENOMEM);
> > -                jret = JxlDecoderGetColorAsICCProfile(ctx->decoder, &ctx->jxl_pixfmt, JXL_COLOR_PROFILE_TARGET_ORIGINAL, ctx->iccp->data, iccp_len);
> > +            jret = JxlDecoderGetColorAsEncodedProfile(ctx->decoder, NULL, JXL_COLOR_PROFILE_TARGET_ORIGINAL, &jxl_color);
> > +            if (jret == JXL_DEC_SUCCESS) {
> > +                /* enum values describe the colors of this image */
> > +                jret = JxlDecoderSetPreferredColorProfile(ctx->decoder, &jxl_color);
> > +                if (jret == JXL_DEC_SUCCESS)
> > +                    jret = JxlDecoderGetColorAsEncodedProfile(ctx->decoder, &ctx->jxl_pixfmt, JXL_COLOR_PROFILE_TARGET_DATA, &jxl_color);
> > +            } else {
> > +                /* an ICC Profile is present in the stream */
> > +                if (ctx->basic_info.uses_original_profile) {
> > +                    av_log(avctx, AV_LOG_VERBOSE, "Using embedded ICC Profile\n");
> > +                    /* an ICC profile is present, and we can meaningfully get it,
> > +                     * because the pixel data is not XYB-encoded */
> > +                    jret = JxlDecoderGetICCProfileSize(ctx->decoder, &ctx->jxl_pixfmt, JXL_COLOR_PROFILE_TARGET_DATA, &iccp_len);
> > +                    if (jret == JXL_DEC_SUCCESS && iccp_len > 0) {
> > +                        av_buffer_unref(&ctx->iccp);
> > +                        ctx->iccp = av_buffer_alloc(iccp_len);
> > +                        if (!ctx->iccp)
> > +                            return AVERROR(ENOMEM);
> > +                        jret = JxlDecoderGetColorAsICCProfile(ctx->decoder, &ctx->jxl_pixfmt, JXL_COLOR_PROFILE_TARGET_DATA, ctx->iccp->data, iccp_len);
> > +                        if (jret != JXL_DEC_SUCCESS) {
> > +                            av_log(avctx, AV_LOG_WARNING, "Unable to obtain ICCP from header\n");
> > +                            av_buffer_unref(&ctx->iccp);
> > +                        }
> > +                    }
> > +                }
> > +                /* when libjxl adds JxlDecoderSetICCProfile() properly handle the XYB case as well */
> > +            }
> > +
> > +            avctx->color_range = frame->color_range = AVCOL_RANGE_JPEG;
> > +            if (ctx->jxl_pixfmt.num_channels >= 3)
> > +                avctx->colorspace = AVCOL_SPC_RGB;
> > +            avctx->color_primaries = AVCOL_PRI_UNSPECIFIED;
> > +            avctx->color_trc = AVCOL_TRC_UNSPECIFIED;
> > +
> > +            if (!ctx->iccp) {
> > +                /* checking enum values */
> > +                if (jret == JXL_DEC_SUCCESS) {
> 
> This whole case: block is starting to look overly complex and would IMO
> benefit from being moved to a separate function (or multiple. And the
> control flow is starting to look complicated, e.g. it is not immediately
> obvious which call does jret come from here.
> 
> Other than these "style" issues, I see no issues with the patch. Then
> again I'm far from being an exper on colorspaces.

The logic looks correct to me but I needed an explanation of the control
flow on IRC to verify that fact. I think you should, at the very least,
document here explicitly the four cases you explained to me.

But ideally, I'd restructure the logic to make it fully explicit which
case is wanting to do what, rather than trying to be clever and ensuring
the control flow implicitly does the right thing.