From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 2F9E243213 for ; Fri, 24 Jun 2022 10:14:32 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 2754868B701; Fri, 24 Jun 2022 13:14:29 +0300 (EEST) Received: from haasn.dev (haasn.dev [78.46.187.166]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 625AF68B656 for ; Fri, 24 Jun 2022 13:14:23 +0300 (EEST) Received: from haasn.dev (unknown [10.30.0.2]) by haasn.dev (Postfix) with ESMTP id 10A294A209; Fri, 24 Jun 2022 12:14:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=haasn.xyz; s=mail; t=1656065663; bh=aZAy6gP8ynihDlrYX2EAmrpHXJn55VzHUk8PECZsNuQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=eVJjhtuP7IWRaS0v3ApKwfC4SLvoFGw0EMbPwsBZHHRvgrUZLvTF+9vNCef4wUo2L oPNNLa/B7CNDTB5jvrtzsQjGy8btEmria4F4T5Fn3eobuwsKedpwFuH5HFMQwgxoT5 MMWziWB2KsKNQUBP8oJ61mx4iMwLZBSlS80J6++w= Date: Fri, 24 Jun 2022 12:14:22 +0200 Message-ID: <20220624121422.GB13065@haasn.xyz> From: Niklas Haas To: FFmpeg development discussions and patches In-Reply-To: <165599292890.13099.500634592857519914@lain> References: <20220602021412.58306-1-leo.izen@gmail.com> <20220602021412.58306-2-leo.izen@gmail.com> <165599292890.13099.500634592857519914@lain> MIME-Version: 1.0 Content-Disposition: inline Subject: Re: [FFmpeg-devel] [PATCH v8 1/2] avcodec/libjxldec: properly tag output colorspace X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Leo Izen Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: On Thu, 23 Jun 2022 16:02:08 +0200 Anton Khirnov 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".