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

[Anton Khirnov <anton@khirnov.net>]

Quoting Leo Izen (2022-06-02 04:14:11)
> Whether an ICC profile is present or not, the decoder
> should now properly tag the colorspace of pixel data
> received by the decoder.
> ---
>  libavcodec/libjxldec.c | 142 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 133 insertions(+), 9 deletions(-)
> 
> diff --git a/libavcodec/libjxldec.c b/libavcodec/libjxldec.c
> index cd4bca3343..d2e0616124 100644
> --- a/libavcodec/libjxldec.c
> +++ b/libavcodec/libjxldec.c
> @@ -27,6 +27,7 @@
>  #include "libavutil/avassert.h"
>  #include "libavutil/buffer.h"
>  #include "libavutil/common.h"
> +#include "libavutil/csp.h"
>  #include "libavutil/error.h"
>  #include "libavutil/mem.h"
>  #include "libavutil/pixdesc.h"
> @@ -92,7 +93,7 @@ static av_cold int libjxl_decode_init(AVCodecContext *avctx)
>      return libjxl_init_jxl_decoder(avctx);
>  }
>  
> -static enum AVPixelFormat libjxl_get_pix_fmt(AVCodecContext *avctx, JxlBasicInfo *basic_info, JxlPixelFormat *format)
> +static enum AVPixelFormat libjxl_get_pix_fmt(void *avctx, const JxlBasicInfo *basic_info, JxlPixelFormat *format)
>  {
>      format->endianness = JXL_NATIVE_ENDIAN;
>      format->num_channels = basic_info->num_color_channels + (basic_info->alpha_bits > 0);
> @@ -129,12 +130,71 @@ static enum AVPixelFormat libjxl_get_pix_fmt(AVCodecContext *avctx, JxlBasicInfo
>      return AV_PIX_FMT_NONE;
>  }
>  
> +static enum AVColorPrimaries libjxl_get_primaries(void *avctx, const JxlColorEncoding *jxl_color)
> +{
> +    AVColorPrimariesDesc desc;
> +    enum AVColorPrimaries prim;
> +
> +    /* libjxl populates these double values even if it uses an enum space */
> +    desc.prim.r.x = av_d2q(jxl_color->primaries_red_xy[0], 300000);
> +    desc.prim.r.y = av_d2q(jxl_color->primaries_red_xy[1], 300000);
> +    desc.prim.g.x = av_d2q(jxl_color->primaries_green_xy[0], 300000);
> +    desc.prim.g.y = av_d2q(jxl_color->primaries_green_xy[1], 300000);
> +    desc.prim.b.x = av_d2q(jxl_color->primaries_blue_xy[0], 300000);
> +    desc.prim.b.y = av_d2q(jxl_color->primaries_blue_xy[1], 300000);
> +    desc.wp.x = av_d2q(jxl_color->white_point_xy[0], 300000);
> +    desc.wp.y = av_d2q(jxl_color->white_point_xy[1], 300000);
> +
> +    prim = av_csp_primaries_id_from_desc(&desc);
> +    if (prim == AVCOL_PRI_UNSPECIFIED) {
> +        /* try D65 with the same primaries */
> +        /* BT.709 uses D65 white point */
> +        const AVWhitepointCoefficients *d65 = &av_csp_primaries_desc_from_id(AVCOL_PRI_BT709)->wp;
> +        desc.wp = *d65;

Omitting the intermediate pointer seems simpler to me:
           desc.wp = av_csp_primaries_desc_from_id(AVCOL_TRC_BT709)->wp


> +        av_log(avctx, AV_LOG_WARNING, "Changing unknown white point to D65\n");
> +        prim = av_csp_primaries_id_from_desc(&desc);
> +    }
> +
> +    return prim;
> +}
> +
> +static enum AVColorTransferCharacteristic libjxl_get_trc(void *avctx, const JxlColorEncoding *jxl_color)
> +{
> +    switch (jxl_color->transfer_function) {
> +    case JXL_TRANSFER_FUNCTION_709:
> +        return AVCOL_TRC_BT709;
> +    case JXL_TRANSFER_FUNCTION_LINEAR:
> +        return AVCOL_TRC_LINEAR;
> +    case JXL_TRANSFER_FUNCTION_SRGB:
> +        return AVCOL_TRC_IEC61966_2_1;
> +    case JXL_TRANSFER_FUNCTION_PQ:
> +        return AVCOL_TRC_SMPTE2084;
> +    case JXL_TRANSFER_FUNCTION_DCI:
> +        return AVCOL_TRC_SMPTE428;
> +    case JXL_TRANSFER_FUNCTION_HLG:
> +        return AVCOL_TRC_ARIB_STD_B67;

cosmetics nit: it seems more readable to put the case and the return on
the same line in this case

> +    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.

-- 
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".