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".
next prev parent 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