Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
From: Niklas Haas <ffmpeg@haasn.xyz>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Cc: Leo Izen <leo.izen@gmail.com>
Subject: Re: [FFmpeg-devel] [PATCH v8 1/2] avcodec/libjxldec: properly tag output colorspace
Date: Fri, 24 Jun 2022 12:14:22 +0200
Message-ID: <20220624121422.GB13065@haasn.xyz> (raw)
In-Reply-To: <165599292890.13099.500634592857519914@lain>

On Thu, 23 Jun 2022 16:02:08 +0200 Anton Khirnov <anton@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.
_______________________________________________
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".

  reply	other threads:[~2022-06-24 10:14 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-02  2:14 [FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes Leo Izen
2022-06-02  2:14 ` [FFmpeg-devel] [PATCH v8 1/2] avcodec/libjxldec: properly tag output colorspace Leo Izen
2022-06-23 14:02   ` Anton Khirnov
2022-06-24 10:14     ` Niklas Haas [this message]
2022-06-02  2:14 ` [FFmpeg-devel] [PATCH v8 2/2] avcodec/libjxlenc: properly read input colorspace Leo Izen
2022-06-23 14:16   ` Anton Khirnov
2022-06-23 15:33     ` Leo Izen
2022-06-23 21:46   ` Niklas Haas
2022-06-09 11:31 ` [FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes Leo Izen
2022-06-15 14:31   ` Leo Izen
2022-06-22 16:28     ` Leo Izen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220624121422.GB13065@haasn.xyz \
    --to=ffmpeg@haasn.xyz \
    --cc=ffmpeg-devel@ffmpeg.org \
    --cc=leo.izen@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git