[FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes

[FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes

[Leo Izen]

Changes in v8:
- Use avutil/csp for both encoding and decoding
- Handle the non-XYB case with an attached ICC Profile on decoding
- clean up some code and segment it out to static functions

Leo Izen (2):
  avcodec/libjxldec: properly tag output colorspace
  avcodec/libjxlenc: properly read input colorspace

 libavcodec/libjxldec.c | 142 +++++++++++++++++++++++++++++++++++---
 libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
 2 files changed, 256 insertions(+), 39 deletions(-)

-- 
2.36.1

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

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

[Leo Izen]

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;
+        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;
+    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) {
+                    if (avctx->colorspace == AVCOL_SPC_RGB)
+                        avctx->color_primaries = libjxl_get_primaries(avctx, &jxl_color);
+                    avctx->color_trc = libjxl_get_trc(avctx, &jxl_color);
+                }
+                /* fall back on wide gamut if enum values fail */
+                if (avctx->color_primaries == AVCOL_PRI_UNSPECIFIED) {
+                    if (avctx->colorspace == AVCOL_SPC_RGB) {
+                        av_log(avctx, AV_LOG_WARNING, "Falling back on wide gamut output\n");
+                        jxl_color.primaries = JXL_PRIMARIES_2100;
+                        avctx->color_primaries = AVCOL_PRI_BT2020;
+                    }
+                    /* libjxl requires this set even for grayscale */
+                    jxl_color.white_point = JXL_WHITE_POINT_D65;
+                }
+                if (avctx->color_trc == AVCOL_TRC_UNSPECIFIED) {
+                    if (ctx->jxl_pixfmt.data_type == JXL_TYPE_FLOAT
+                        || ctx->jxl_pixfmt.data_type == JXL_TYPE_FLOAT16) {
+                        av_log(avctx, AV_LOG_WARNING, "Falling back on Linear Light transfer\n");
+                        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
+                        avctx->color_trc = AVCOL_TRC_LINEAR;
+                    } else {
+                        av_log(avctx, AV_LOG_WARNING, "Falling back on iec61966-2-1/sRGB transfer\n");
+                        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
+                        avctx->color_trc = AVCOL_TRC_IEC61966_2_1;
+                    }
+                }
+                /* all colors will be in-gamut so we want accurate colors */
+                jxl_color.rendering_intent = JXL_RENDERING_INTENT_RELATIVE;
+                jxl_color.color_space = avctx->colorspace == AVCOL_SPC_RGB ? JXL_COLOR_SPACE_RGB : JXL_COLOR_SPACE_GRAY;
+                jret = JxlDecoderSetPreferredColorProfile(ctx->decoder, &jxl_color);
                 if (jret != JXL_DEC_SUCCESS)
-                    av_buffer_unref(&ctx->iccp);
+                    av_log(avctx, AV_LOG_WARNING, "Unable to set fallback color encoding\n");
             }
+
+            frame->color_trc = avctx->color_trc;
+            frame->color_primaries = avctx->color_primaries;
+            frame->colorspace = avctx->colorspace;
             continue;
         case JXL_DEC_NEED_IMAGE_OUT_BUFFER:
             av_log(avctx, AV_LOG_DEBUG, "NEED_IMAGE_OUT_BUFFER event emitted\n");
-- 
2.36.1

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

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

[Anton Khirnov]

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

Re: [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@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".

[FFmpeg-devel] [PATCH v8 2/2] avcodec/libjxlenc: properly read input colorspace

[Leo Izen]

Whether an ICC profile is present or not, the libjxl
encoder wrapper should now properly read colorspace tags
and forward them to libjxl appropriately, rather than just
assume sRGB as before. It will also print warnings when
colorimetric assumptions are made about the input data.
---
 libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
 1 file changed, 123 insertions(+), 30 deletions(-)

diff --git a/libavcodec/libjxlenc.c b/libavcodec/libjxlenc.c
index 8bebec6aeb..1c09b69345 100644
--- a/libavcodec/libjxlenc.c
+++ b/libavcodec/libjxlenc.c
@@ -27,6 +27,7 @@
 #include <string.h>
 
 #include "libavutil/avutil.h"
+#include "libavutil/csp.h"
 #include "libavutil/error.h"
 #include "libavutil/frame.h"
 #include "libavutil/libm.h"
@@ -117,7 +118,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
         return AVERROR_EXTERNAL;
     }
 
-    /* check for negative zero, our default */
+    /* check for negative, our default */
     if (ctx->distance < 0.0) {
         /* use ffmpeg.c -q option if passed */
         if (avctx->flags & AV_CODEC_FLAG_QSCALE)
@@ -133,7 +134,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
      */
     if (ctx->distance > 0.0 && ctx->distance < 0.01)
         ctx->distance = 0.01;
-    if (JxlEncoderOptionsSetDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
+    if (JxlEncoderSetFrameDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
         av_log(avctx, AV_LOG_ERROR, "Failed to set distance: %f\n", ctx->distance);
         return AVERROR_EXTERNAL;
     }
@@ -185,6 +186,52 @@ static av_cold int libjxl_encode_init(AVCodecContext *avctx)
     return 0;
 }
 
+/**
+ * Populate a JxlColorEncoding with the given enum AVColorPrimaries.
+ * @return < 0 upon failure, >= 0 upon success
+ */
+static int libjxl_populate_primaries(JxlColorEncoding *jxl_color, enum AVColorPrimaries prm)
+{
+    const AVColorPrimariesDesc *desc;
+
+    switch (prm) {
+    case AVCOL_PRI_BT709:
+        jxl_color->primaries = JXL_PRIMARIES_SRGB;
+        jxl_color->white_point = JXL_WHITE_POINT_D65;
+        return 0;
+    case AVCOL_PRI_BT2020:
+        jxl_color->primaries = JXL_PRIMARIES_2100;
+        jxl_color->white_point = JXL_WHITE_POINT_D65;
+        return 0;
+    case AVCOL_PRI_SMPTE431:
+        jxl_color->primaries = JXL_PRIMARIES_P3;
+        jxl_color->white_point = JXL_WHITE_POINT_DCI;
+        return 0;
+    case AVCOL_PRI_SMPTE432:
+        jxl_color->primaries = JXL_PRIMARIES_P3;
+        jxl_color->white_point = JXL_WHITE_POINT_D65;
+        return 0;
+    }
+
+    desc = av_csp_primaries_desc_from_id(prm);
+    if (!desc)
+        return AVERROR(EINVAL);
+
+    jxl_color->primaries = JXL_PRIMARIES_CUSTOM;
+    jxl_color->white_point = JXL_WHITE_POINT_CUSTOM;
+
+    jxl_color->primaries_red_xy[0] = av_q2d(desc->prim.r.x);
+    jxl_color->primaries_red_xy[1] = av_q2d(desc->prim.r.y);
+    jxl_color->primaries_green_xy[0] = av_q2d(desc->prim.g.x);
+    jxl_color->primaries_green_xy[1] = av_q2d(desc->prim.g.y);
+    jxl_color->primaries_blue_xy[0] = av_q2d(desc->prim.b.x);
+    jxl_color->primaries_blue_xy[1] = av_q2d(desc->prim.b.y);
+    jxl_color->white_point_xy[0] = av_q2d(desc->wp.x);
+    jxl_color->white_point_xy[1] = av_q2d(desc->wp.y);
+
+    return 1;
+}
+
 /**
  * Encode an entire frame. Currently animation, is not supported by
  * this encoder, so this will always reinitialize a new still image
@@ -223,49 +270,95 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
         info.exponent_bits_per_sample = info.bits_per_sample > 16 ? 8 : 5;
         info.alpha_exponent_bits = info.alpha_bits ? info.exponent_bits_per_sample : 0;
         jxl_fmt.data_type = info.bits_per_sample > 16 ? JXL_TYPE_FLOAT : JXL_TYPE_FLOAT16;
-        JxlColorEncodingSetToLinearSRGB(&jxl_color, info.num_color_channels == 1);
     } else {
         info.exponent_bits_per_sample = 0;
         info.alpha_exponent_bits = 0;
         jxl_fmt.data_type = info.bits_per_sample <= 8 ? JXL_TYPE_UINT8 : JXL_TYPE_UINT16;
-        JxlColorEncodingSetToSRGB(&jxl_color, info.num_color_channels == 1);
     }
 
-    if (info.bits_per_sample > 16
-        || info.xsize > (1 << 18) || info.ysize > (1 << 18)
-        || (info.xsize << 4) * (info.ysize << 4) > (1 << 20)) {
-        /*
-         * must upgrade codestream to level 10, from level 5
-         * the encoder will not do this automatically
-         */
-        if (JxlEncoderSetCodestreamLevel(ctx->encoder, 10) != JXL_ENC_SUCCESS) {
-            av_log(avctx, AV_LOG_ERROR, "Could not upgrade JXL Codestream level.\n");
-            return AVERROR_EXTERNAL;
-        }
-    }
+    /* JPEG XL format itself does not support limited range */
+    if (avctx->color_range == AVCOL_RANGE_MPEG ||
+        avctx->color_range == AVCOL_RANGE_UNSPECIFIED && frame->color_range == AVCOL_RANGE_MPEG)
+        av_log(avctx, AV_LOG_ERROR, "This encoder does not support limited (tv) range, colors will be wrong!\n");
+    else if (avctx->color_range != AVCOL_RANGE_JPEG && frame->color_range != AVCOL_RANGE_JPEG)
+        av_log(avctx, AV_LOG_WARNING, "Unknown color range, assuming full (pc)\n");
 
     /* bitexact lossless requires there to be no XYB transform */
     info.uses_original_profile = ctx->distance == 0.0;
 
-    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
-    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS) {
-        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
-    } else if (info.uses_original_profile) {
-        /*
-        * the color encoding is not used if uses_original_profile is false
-        * this just works around a bug in libjxl 0.7.0 and lower
-        */
-        if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS) {
-            av_log(avctx, AV_LOG_ERROR, "Failed to set JxlColorEncoding\n");
-            return AVERROR_EXTERNAL;
-        }
-    }
-
     if (JxlEncoderSetBasicInfo(ctx->encoder, &info) != JXL_ENC_SUCCESS) {
         av_log(avctx, AV_LOG_ERROR, "Failed to set JxlBasicInfo\n");
         return AVERROR_EXTERNAL;
     }
 
+    /* rendering intent doesn't matter here
+     * but libjxl will whine if we don't set it */
+    jxl_color.rendering_intent = JXL_RENDERING_INTENT_RELATIVE;
+
+    switch (avctx->color_trc && avctx->color_trc != AVCOL_TRC_UNSPECIFIED
+            ? avctx->color_trc : frame->color_trc) {
+    case AVCOL_TRC_BT709:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_709;
+        break;
+    case AVCOL_TRC_LINEAR:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
+        break;
+    case AVCOL_TRC_IEC61966_2_1:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
+        break;
+    case AVCOL_TRC_SMPTE428:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_DCI;
+        break;
+    case AVCOL_TRC_SMPTE2084:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_PQ;
+        break;
+    case AVCOL_TRC_ARIB_STD_B67:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_HLG;
+        break;
+    case AVCOL_TRC_GAMMA22:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
+        jxl_color.gamma = 2.2;
+        break;
+    case AVCOL_TRC_GAMMA28:
+        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
+        jxl_color.gamma = 2.8;
+        break;
+    default:
+        if (pix_desc->flags & AV_PIX_FMT_FLAG_FLOAT) {
+            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming Linear Light\n");
+            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
+        } else {
+            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming IEC61966-2-1/sRGB\n");
+            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
+        }
+    }
+
+    /* This should be implied to be honest
+     * but a libjxl bug makes it fail otherwise */
+    if (info.num_color_channels == 1)
+        jxl_color.color_space = JXL_COLOR_SPACE_GRAY;
+    else
+        jxl_color.color_space = JXL_COLOR_SPACE_RGB;
+
+    ret = libjxl_populate_primaries(&jxl_color,
+            avctx->color_primaries && avctx->color_primaries != AVCOL_PRI_UNSPECIFIED
+            ? avctx->color_primaries : frame->color_primaries);
+    if (ret < 0)
+        return ret;
+
+    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
+    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS)
+        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
+    if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS)
+        av_log(avctx, AV_LOG_WARNING, "Failed to set JxlColorEncoding\n");
+
+    /* depending on basic info, level 10 might
+     * be required instead of level 5 */
+    if (JxlEncoderGetRequiredCodestreamLevel(ctx->encoder) > 5) {
+        if (JxlEncoderSetCodestreamLevel(ctx->encoder, 10) != JXL_ENC_SUCCESS)
+            av_log(avctx, AV_LOG_WARNING, "Could not increase codestream level\n");
+    }
+
     jxl_fmt.endianness = JXL_NATIVE_ENDIAN;
     jxl_fmt.align = frame->linesize[0];
 
-- 
2.36.1

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

Re: [FFmpeg-devel] [PATCH v8 2/2] avcodec/libjxlenc: properly read input colorspace

[Anton Khirnov]

Quoting Leo Izen (2022-06-02 04:14:12)
> Whether an ICC profile is present or not, the libjxl
> encoder wrapper should now properly read colorspace tags
> and forward them to libjxl appropriately, rather than just
> assume sRGB as before. It will also print warnings when
> colorimetric assumptions are made about the input data.
> ---
>  libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 123 insertions(+), 30 deletions(-)
> 
> diff --git a/libavcodec/libjxlenc.c b/libavcodec/libjxlenc.c
> index 8bebec6aeb..1c09b69345 100644
> --- a/libavcodec/libjxlenc.c
> +++ b/libavcodec/libjxlenc.c
> @@ -27,6 +27,7 @@
>  #include <string.h>
>  
>  #include "libavutil/avutil.h"
> +#include "libavutil/csp.h"
>  #include "libavutil/error.h"
>  #include "libavutil/frame.h"
>  #include "libavutil/libm.h"
> @@ -117,7 +118,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
>          return AVERROR_EXTERNAL;
>      }
>  
> -    /* check for negative zero, our default */
> +    /* check for negative, our default */
>      if (ctx->distance < 0.0) {
>          /* use ffmpeg.c -q option if passed */
>          if (avctx->flags & AV_CODEC_FLAG_QSCALE)
> @@ -133,7 +134,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
>       */
>      if (ctx->distance > 0.0 && ctx->distance < 0.01)
>          ctx->distance = 0.01;
> -    if (JxlEncoderOptionsSetDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
> +    if (JxlEncoderSetFrameDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to set distance: %f\n", ctx->distance);
>          return AVERROR_EXTERNAL;
>      }
> @@ -185,6 +186,52 @@ static av_cold int libjxl_encode_init(AVCodecContext *avctx)
>      return 0;
>  }
>  
> +/**
> + * Populate a JxlColorEncoding with the given enum AVColorPrimaries.
> + * @return < 0 upon failure, >= 0 upon success
> + */
> +static int libjxl_populate_primaries(JxlColorEncoding *jxl_color, enum AVColorPrimaries prm)
> +{
> +    const AVColorPrimariesDesc *desc;
> +
> +    switch (prm) {
> +    case AVCOL_PRI_BT709:
> +        jxl_color->primaries = JXL_PRIMARIES_SRGB;
> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
> +        return 0;
> +    case AVCOL_PRI_BT2020:
> +        jxl_color->primaries = JXL_PRIMARIES_2100;
> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
> +        return 0;
> +    case AVCOL_PRI_SMPTE431:
> +        jxl_color->primaries = JXL_PRIMARIES_P3;
> +        jxl_color->white_point = JXL_WHITE_POINT_DCI;
> +        return 0;
> +    case AVCOL_PRI_SMPTE432:
> +        jxl_color->primaries = JXL_PRIMARIES_P3;
> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
> +        return 0;
> +    }
> +
> +    desc = av_csp_primaries_desc_from_id(prm);
> +    if (!desc)
> +        return AVERROR(EINVAL);
> +
> +    jxl_color->primaries = JXL_PRIMARIES_CUSTOM;
> +    jxl_color->white_point = JXL_WHITE_POINT_CUSTOM;
> +
> +    jxl_color->primaries_red_xy[0] = av_q2d(desc->prim.r.x);
> +    jxl_color->primaries_red_xy[1] = av_q2d(desc->prim.r.y);
> +    jxl_color->primaries_green_xy[0] = av_q2d(desc->prim.g.x);
> +    jxl_color->primaries_green_xy[1] = av_q2d(desc->prim.g.y);
> +    jxl_color->primaries_blue_xy[0] = av_q2d(desc->prim.b.x);
> +    jxl_color->primaries_blue_xy[1] = av_q2d(desc->prim.b.y);
> +    jxl_color->white_point_xy[0] = av_q2d(desc->wp.x);
> +    jxl_color->white_point_xy[1] = av_q2d(desc->wp.y);
> +
> +    return 1;

Any reason for returning 1 rather than 0? I don't see it making a
difference, so it just confuses the reader.

> +}
> +
>  /**
>   * Encode an entire frame. Currently animation, is not supported by
>   * this encoder, so this will always reinitialize a new still image
> @@ -223,49 +270,95 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>          info.exponent_bits_per_sample = info.bits_per_sample > 16 ? 8 : 5;
>          info.alpha_exponent_bits = info.alpha_bits ? info.exponent_bits_per_sample : 0;
>          jxl_fmt.data_type = info.bits_per_sample > 16 ? JXL_TYPE_FLOAT : JXL_TYPE_FLOAT16;
> -        JxlColorEncodingSetToLinearSRGB(&jxl_color, info.num_color_channels == 1);
>      } else {
>          info.exponent_bits_per_sample = 0;
>          info.alpha_exponent_bits = 0;
>          jxl_fmt.data_type = info.bits_per_sample <= 8 ? JXL_TYPE_UINT8 : JXL_TYPE_UINT16;
> -        JxlColorEncodingSetToSRGB(&jxl_color, info.num_color_channels == 1);
>      }
>  
> -    if (info.bits_per_sample > 16
> -        || info.xsize > (1 << 18) || info.ysize > (1 << 18)
> -        || (info.xsize << 4) * (info.ysize << 4) > (1 << 20)) {
> -        /*
> -         * must upgrade codestream to level 10, from level 5
> -         * the encoder will not do this automatically
> -         */

Is this not relevant anymore? I don't see what is it replaced by.

> -        if (JxlEncoderSetCodestreamLevel(ctx->encoder, 10) != JXL_ENC_SUCCESS) {
> -            av_log(avctx, AV_LOG_ERROR, "Could not upgrade JXL Codestream level.\n");
> -            return AVERROR_EXTERNAL;
> -        }
> -    }
> +    /* JPEG XL format itself does not support limited range */
> +    if (avctx->color_range == AVCOL_RANGE_MPEG ||
> +        avctx->color_range == AVCOL_RANGE_UNSPECIFIED && frame->color_range == AVCOL_RANGE_MPEG)
> +        av_log(avctx, AV_LOG_ERROR, "This encoder does not support limited (tv) range, colors will be wrong!\n");

If it's LOG_ERROR, then it should fail IMO. Otherwise it's a warning.

> +    else if (avctx->color_range != AVCOL_RANGE_JPEG && frame->color_range != AVCOL_RANGE_JPEG)
> +        av_log(avctx, AV_LOG_WARNING, "Unknown color range, assuming full (pc)\n");
>  
>      /* bitexact lossless requires there to be no XYB transform */
>      info.uses_original_profile = ctx->distance == 0.0;
>  
> -    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
> -    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS) {
> -        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
> -    } else if (info.uses_original_profile) {
> -        /*
> -        * the color encoding is not used if uses_original_profile is false
> -        * this just works around a bug in libjxl 0.7.0 and lower
> -        */
> -        if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS) {
> -            av_log(avctx, AV_LOG_ERROR, "Failed to set JxlColorEncoding\n");
> -            return AVERROR_EXTERNAL;
> -        }
> -    }
> -
>      if (JxlEncoderSetBasicInfo(ctx->encoder, &info) != JXL_ENC_SUCCESS) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to set JxlBasicInfo\n");
>          return AVERROR_EXTERNAL;
>      }
>  
> +    /* rendering intent doesn't matter here
> +     * but libjxl will whine if we don't set it */
> +    jxl_color.rendering_intent = JXL_RENDERING_INTENT_RELATIVE;
> +
> +    switch (avctx->color_trc && avctx->color_trc != AVCOL_TRC_UNSPECIFIED
> +            ? avctx->color_trc : frame->color_trc) {

Any reason for this ordering? I would think the frame information should
be preferred, if it is set.

> +    case AVCOL_TRC_BT709:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_709;
> +        break;
> +    case AVCOL_TRC_LINEAR:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
> +        break;
> +    case AVCOL_TRC_IEC61966_2_1:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
> +        break;
> +    case AVCOL_TRC_SMPTE428:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_DCI;
> +        break;
> +    case AVCOL_TRC_SMPTE2084:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_PQ;
> +        break;
> +    case AVCOL_TRC_ARIB_STD_B67:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_HLG;
> +        break;
> +    case AVCOL_TRC_GAMMA22:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
> +        jxl_color.gamma = 2.2;
> +        break;
> +    case AVCOL_TRC_GAMMA28:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
> +        jxl_color.gamma = 2.8;
> +        break;
> +    default:
> +        if (pix_desc->flags & AV_PIX_FMT_FLAG_FLOAT) {
> +            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming Linear Light\n");
> +            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
> +        } else {
> +            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming IEC61966-2-1/sRGB\n");
> +            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
> +        }
> +    }
> +
> +    /* This should be implied to be honest
> +     * but a libjxl bug makes it fail otherwise */
> +    if (info.num_color_channels == 1)
> +        jxl_color.color_space = JXL_COLOR_SPACE_GRAY;
> +    else
> +        jxl_color.color_space = JXL_COLOR_SPACE_RGB;
> +
> +    ret = libjxl_populate_primaries(&jxl_color,
> +            avctx->color_primaries && avctx->color_primaries != AVCOL_PRI_UNSPECIFIED
> +            ? avctx->color_primaries : frame->color_primaries);
> +    if (ret < 0)
> +        return ret;
> +
> +    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
> +    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS)
> +        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
> +    if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS)
> +        av_log(avctx, AV_LOG_WARNING, "Failed to set JxlColorEncoding\n");

Do we expect this to fail in non-error conditions?

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

Re: [FFmpeg-devel] [PATCH v8 2/2] avcodec/libjxlenc: properly read input colorspace

[Leo Izen]

On 6/23/22 10:16, Anton Khirnov wrote:
> Quoting Leo Izen (2022-06-02 04:14:12)
>> Whether an ICC profile is present or not, the libjxl
>> encoder wrapper should now properly read colorspace tags
>> and forward them to libjxl appropriately, rather than just
>> assume sRGB as before. It will also print warnings when
>> colorimetric assumptions are made about the input data.
>> ---
>>   libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
>>   1 file changed, 123 insertions(+), 30 deletions(-)
>>
>> diff --git a/libavcodec/libjxlenc.c b/libavcodec/libjxlenc.c
>> index 8bebec6aeb..1c09b69345 100644
>> --- a/libavcodec/libjxlenc.c
>> +++ b/libavcodec/libjxlenc.c
>> @@ -27,6 +27,7 @@
>>   #include <string.h>
>>   
>>   #include "libavutil/avutil.h"
>> +#include "libavutil/csp.h"
>>   #include "libavutil/error.h"
>>   #include "libavutil/frame.h"
>>   #include "libavutil/libm.h"
>> @@ -117,7 +118,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
>>           return AVERROR_EXTERNAL;
>>       }
>>   
>> -    /* check for negative zero, our default */
>> +    /* check for negative, our default */
>>       if (ctx->distance < 0.0) {
>>           /* use ffmpeg.c -q option if passed */
>>           if (avctx->flags & AV_CODEC_FLAG_QSCALE)
>> @@ -133,7 +134,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
>>        */
>>       if (ctx->distance > 0.0 && ctx->distance < 0.01)
>>           ctx->distance = 0.01;
>> -    if (JxlEncoderOptionsSetDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
>> +    if (JxlEncoderSetFrameDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
>>           av_log(avctx, AV_LOG_ERROR, "Failed to set distance: %f\n", ctx->distance);
>>           return AVERROR_EXTERNAL;
>>       }
>> @@ -185,6 +186,52 @@ static av_cold int libjxl_encode_init(AVCodecContext *avctx)
>>       return 0;
>>   }
>>   
>> +/**
>> + * Populate a JxlColorEncoding with the given enum AVColorPrimaries.
>> + * @return < 0 upon failure, >= 0 upon success
>> + */
>> +static int libjxl_populate_primaries(JxlColorEncoding *jxl_color, enum AVColorPrimaries prm)
>> +{
>> +    const AVColorPrimariesDesc *desc;
>> +
>> +    switch (prm) {
>> +    case AVCOL_PRI_BT709:
>> +        jxl_color->primaries = JXL_PRIMARIES_SRGB;
>> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
>> +        return 0;
>> +    case AVCOL_PRI_BT2020:
>> +        jxl_color->primaries = JXL_PRIMARIES_2100;
>> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
>> +        return 0;
>> +    case AVCOL_PRI_SMPTE431:
>> +        jxl_color->primaries = JXL_PRIMARIES_P3;
>> +        jxl_color->white_point = JXL_WHITE_POINT_DCI;
>> +        return 0;
>> +    case AVCOL_PRI_SMPTE432:
>> +        jxl_color->primaries = JXL_PRIMARIES_P3;
>> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
>> +        return 0;
>> +    }
>> +
>> +    desc = av_csp_primaries_desc_from_id(prm);
>> +    if (!desc)
>> +        return AVERROR(EINVAL);
>> +
>> +    jxl_color->primaries = JXL_PRIMARIES_CUSTOM;
>> +    jxl_color->white_point = JXL_WHITE_POINT_CUSTOM;
>> +
>> +    jxl_color->primaries_red_xy[0] = av_q2d(desc->prim.r.x);
>> +    jxl_color->primaries_red_xy[1] = av_q2d(desc->prim.r.y);
>> +    jxl_color->primaries_green_xy[0] = av_q2d(desc->prim.g.x);
>> +    jxl_color->primaries_green_xy[1] = av_q2d(desc->prim.g.y);
>> +    jxl_color->primaries_blue_xy[0] = av_q2d(desc->prim.b.x);
>> +    jxl_color->primaries_blue_xy[1] = av_q2d(desc->prim.b.y);
>> +    jxl_color->white_point_xy[0] = av_q2d(desc->wp.x);
>> +    jxl_color->white_point_xy[1] = av_q2d(desc->wp.y);
>> +
>> +    return 1;
> 
> Any reason for returning 1 rather than 0? I don't see it making a
> difference, so it just confuses the reader.

I was planning on using it later and then I ended up not using it. I can 
change it to return 0; for clarity.

>> +}
>> +
>>   /**
>>    * Encode an entire frame. Currently animation, is not supported by
>>    * this encoder, so this will always reinitialize a new still image
>> @@ -223,49 +270,95 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>>           info.exponent_bits_per_sample = info.bits_per_sample > 16 ? 8 : 5;
>>           info.alpha_exponent_bits = info.alpha_bits ? info.exponent_bits_per_sample : 0;
>>           jxl_fmt.data_type = info.bits_per_sample > 16 ? JXL_TYPE_FLOAT : JXL_TYPE_FLOAT16;
>> -        JxlColorEncodingSetToLinearSRGB(&jxl_color, info.num_color_channels == 1);
>>       } else {
>>           info.exponent_bits_per_sample = 0;
>>           info.alpha_exponent_bits = 0;
>>           jxl_fmt.data_type = info.bits_per_sample <= 8 ? JXL_TYPE_UINT8 : JXL_TYPE_UINT16;
>> -        JxlColorEncodingSetToSRGB(&jxl_color, info.num_color_channels == 1);
>>       }
>>   
>> -    if (info.bits_per_sample > 16
>> -        || info.xsize > (1 << 18) || info.ysize > (1 << 18)
>> -        || (info.xsize << 4) * (info.ysize << 4) > (1 << 20)) {
>> -        /*
>> -         * must upgrade codestream to level 10, from level 5
>> -         * the encoder will not do this automatically
>> -         */
> 
> Is this not relevant anymore? I don't see what is it replaced by.

It's replaced by the block at the end of this function that calls 
JxlEncoderGetRequiredCodestreamLevel. It's better to use the API call 
because there's other restrictions, not just frame size.

>> -        if (JxlEncoderSetCodestreamLevel(ctx->encoder, 10) != JXL_ENC_SUCCESS) {
>> -            av_log(avctx, AV_LOG_ERROR, "Could not upgrade JXL Codestream level.\n");
>> -            return AVERROR_EXTERNAL;
>> -        }
>> -    }
>> +    /* JPEG XL format itself does not support limited range */
>> +    if (avctx->color_range == AVCOL_RANGE_MPEG ||
>> +        avctx->color_range == AVCOL_RANGE_UNSPECIFIED && frame->color_range == AVCOL_RANGE_MPEG)
>> +        av_log(avctx, AV_LOG_ERROR, "This encoder does not support limited (tv) range, colors will be wrong!\n");
> 
> If it's LOG_ERROR, then it should fail IMO. Otherwise it's a warning.

In this case the error is that the colors are incorrect. I can change it 
to a warning or I could make it fail if it is known the colors will be 
wrong. Which one is preferable?

> 
>> +    else if (avctx->color_range != AVCOL_RANGE_JPEG && frame->color_range != AVCOL_RANGE_JPEG)
>> +        av_log(avctx, AV_LOG_WARNING, "Unknown color range, assuming full (pc)\n");
>>   
>>       /* bitexact lossless requires there to be no XYB transform */
>>       info.uses_original_profile = ctx->distance == 0.0;
>>   
>> -    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
>> -    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS) {
>> -        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
>> -    } else if (info.uses_original_profile) {
>> -        /*
>> -        * the color encoding is not used if uses_original_profile is false
>> -        * this just works around a bug in libjxl 0.7.0 and lower
>> -        */
>> -        if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS) {
>> -            av_log(avctx, AV_LOG_ERROR, "Failed to set JxlColorEncoding\n");
>> -            return AVERROR_EXTERNAL;
>> -        }
>> -    }
>> -
>>       if (JxlEncoderSetBasicInfo(ctx->encoder, &info) != JXL_ENC_SUCCESS) {
>>           av_log(avctx, AV_LOG_ERROR, "Failed to set JxlBasicInfo\n");
>>           return AVERROR_EXTERNAL;
>>       }
>>   
>> +    /* rendering intent doesn't matter here
>> +     * but libjxl will whine if we don't set it */
>> +    jxl_color.rendering_intent = JXL_RENDERING_INTENT_RELATIVE;
>> +
>> +    switch (avctx->color_trc && avctx->color_trc != AVCOL_TRC_UNSPECIFIED
>> +            ? avctx->color_trc : frame->color_trc) {
> 
> Any reason for this ordering? I would think the frame information should
> be preferred, if it is set.

I found when testing with ffmpeg.c that avctx is likely to be set and 
frame is not, but I can always change it around, and let avctx be the 
fallback.

>> +    case AVCOL_TRC_BT709:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_709;
>> +        break;
>> +    case AVCOL_TRC_LINEAR:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
>> +        break;
>> +    case AVCOL_TRC_IEC61966_2_1:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
>> +        break;
>> +    case AVCOL_TRC_SMPTE428:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_DCI;
>> +        break;
>> +    case AVCOL_TRC_SMPTE2084:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_PQ;
>> +        break;
>> +    case AVCOL_TRC_ARIB_STD_B67:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_HLG;
>> +        break;
>> +    case AVCOL_TRC_GAMMA22:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
>> +        jxl_color.gamma = 2.2;
>> +        break;
>> +    case AVCOL_TRC_GAMMA28:
>> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
>> +        jxl_color.gamma = 2.8;
>> +        break;
>> +    default:
>> +        if (pix_desc->flags & AV_PIX_FMT_FLAG_FLOAT) {
>> +            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming Linear Light\n");
>> +            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
>> +        } else {
>> +            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming IEC61966-2-1/sRGB\n");
>> +            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
>> +        }
>> +    }
>> +
>> +    /* This should be implied to be honest
>> +     * but a libjxl bug makes it fail otherwise */
>> +    if (info.num_color_channels == 1)
>> +        jxl_color.color_space = JXL_COLOR_SPACE_GRAY;
>> +    else
>> +        jxl_color.color_space = JXL_COLOR_SPACE_RGB;
>> +
>> +    ret = libjxl_populate_primaries(&jxl_color,
>> +            avctx->color_primaries && avctx->color_primaries != AVCOL_PRI_UNSPECIFIED
>> +            ? avctx->color_primaries : frame->color_primaries);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
>> +    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS)
>> +        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
>> +    if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS)
>> +        av_log(avctx, AV_LOG_WARNING, "Failed to set JxlColorEncoding\n");
> 
> Do we expect this to fail in non-error conditions?
> 

It shouldn't fail if there are no bugs, but there's historically been 
some libjxl bugs that made this fail in places it shouldn't (see the 
block earlier testing info.num_color_channels for an example). I'd 
prefer if a libjxl regression in this regard didn't break encoding in 
ffmpeg. I can always make this a fatal error if we would prefer 
otherwise. Thoughts?

- Leo Izen (thebombzen)
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v8 2/2] avcodec/libjxlenc: properly read input colorspace

[Niklas Haas]

On Wed, 01 Jun 2022 22:14:12 -0400 Leo Izen <leo.izen@gmail.com> wrote:
> Whether an ICC profile is present or not, the libjxl
> encoder wrapper should now properly read colorspace tags
> and forward them to libjxl appropriately, rather than just
> assume sRGB as before. It will also print warnings when
> colorimetric assumptions are made about the input data.
> ---
>  libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 123 insertions(+), 30 deletions(-)
> 
> diff --git a/libavcodec/libjxlenc.c b/libavcodec/libjxlenc.c
> index 8bebec6aeb..1c09b69345 100644
> --- a/libavcodec/libjxlenc.c
> +++ b/libavcodec/libjxlenc.c
> @@ -27,6 +27,7 @@
>  #include <string.h>
>  
>  #include "libavutil/avutil.h"
> +#include "libavutil/csp.h"
>  #include "libavutil/error.h"
>  #include "libavutil/frame.h"
>  #include "libavutil/libm.h"
> @@ -117,7 +118,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
>          return AVERROR_EXTERNAL;
>      }
>  
> -    /* check for negative zero, our default */
> +    /* check for negative, our default */
>      if (ctx->distance < 0.0) {
>          /* use ffmpeg.c -q option if passed */
>          if (avctx->flags & AV_CODEC_FLAG_QSCALE)
> @@ -133,7 +134,7 @@ static int libjxl_init_jxl_encoder(AVCodecContext *avctx)
>       */
>      if (ctx->distance > 0.0 && ctx->distance < 0.01)
>          ctx->distance = 0.01;
> -    if (JxlEncoderOptionsSetDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
> +    if (JxlEncoderSetFrameDistance(ctx->options, ctx->distance) != JXL_ENC_SUCCESS) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to set distance: %f\n", ctx->distance);
>          return AVERROR_EXTERNAL;
>      }
> @@ -185,6 +186,52 @@ static av_cold int libjxl_encode_init(AVCodecContext *avctx)
>      return 0;
>  }
>  
> +/**
> + * Populate a JxlColorEncoding with the given enum AVColorPrimaries.
> + * @return < 0 upon failure, >= 0 upon success
> + */
> +static int libjxl_populate_primaries(JxlColorEncoding *jxl_color, enum AVColorPrimaries prm)
> +{
> +    const AVColorPrimariesDesc *desc;
> +
> +    switch (prm) {
> +    case AVCOL_PRI_BT709:
> +        jxl_color->primaries = JXL_PRIMARIES_SRGB;
> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
> +        return 0;
> +    case AVCOL_PRI_BT2020:
> +        jxl_color->primaries = JXL_PRIMARIES_2100;
> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
> +        return 0;
> +    case AVCOL_PRI_SMPTE431:
> +        jxl_color->primaries = JXL_PRIMARIES_P3;
> +        jxl_color->white_point = JXL_WHITE_POINT_DCI;
> +        return 0;
> +    case AVCOL_PRI_SMPTE432:
> +        jxl_color->primaries = JXL_PRIMARIES_P3;
> +        jxl_color->white_point = JXL_WHITE_POINT_D65;
> +        return 0;
> +    }
> +
> +    desc = av_csp_primaries_desc_from_id(prm);
> +    if (!desc)
> +        return AVERROR(EINVAL);
> +
> +    jxl_color->primaries = JXL_PRIMARIES_CUSTOM;
> +    jxl_color->white_point = JXL_WHITE_POINT_CUSTOM;
> +
> +    jxl_color->primaries_red_xy[0] = av_q2d(desc->prim.r.x);
> +    jxl_color->primaries_red_xy[1] = av_q2d(desc->prim.r.y);
> +    jxl_color->primaries_green_xy[0] = av_q2d(desc->prim.g.x);
> +    jxl_color->primaries_green_xy[1] = av_q2d(desc->prim.g.y);
> +    jxl_color->primaries_blue_xy[0] = av_q2d(desc->prim.b.x);
> +    jxl_color->primaries_blue_xy[1] = av_q2d(desc->prim.b.y);
> +    jxl_color->white_point_xy[0] = av_q2d(desc->wp.x);
> +    jxl_color->white_point_xy[1] = av_q2d(desc->wp.y);
> +
> +    return 1;
> +}
> +
>  /**
>   * Encode an entire frame. Currently animation, is not supported by
>   * this encoder, so this will always reinitialize a new still image
> @@ -223,49 +270,95 @@ static int libjxl_encode_frame(AVCodecContext *avctx, AVPacket *pkt, const AVFra
>          info.exponent_bits_per_sample = info.bits_per_sample > 16 ? 8 : 5;
>          info.alpha_exponent_bits = info.alpha_bits ? info.exponent_bits_per_sample : 0;
>          jxl_fmt.data_type = info.bits_per_sample > 16 ? JXL_TYPE_FLOAT : JXL_TYPE_FLOAT16;
> -        JxlColorEncodingSetToLinearSRGB(&jxl_color, info.num_color_channels == 1);
>      } else {
>          info.exponent_bits_per_sample = 0;
>          info.alpha_exponent_bits = 0;
>          jxl_fmt.data_type = info.bits_per_sample <= 8 ? JXL_TYPE_UINT8 : JXL_TYPE_UINT16;
> -        JxlColorEncodingSetToSRGB(&jxl_color, info.num_color_channels == 1);
>      }
>  
> -    if (info.bits_per_sample > 16
> -        || info.xsize > (1 << 18) || info.ysize > (1 << 18)
> -        || (info.xsize << 4) * (info.ysize << 4) > (1 << 20)) {
> -        /*
> -         * must upgrade codestream to level 10, from level 5
> -         * the encoder will not do this automatically
> -         */
> -        if (JxlEncoderSetCodestreamLevel(ctx->encoder, 10) != JXL_ENC_SUCCESS) {
> -            av_log(avctx, AV_LOG_ERROR, "Could not upgrade JXL Codestream level.\n");
> -            return AVERROR_EXTERNAL;
> -        }
> -    }
> +    /* JPEG XL format itself does not support limited range */
> +    if (avctx->color_range == AVCOL_RANGE_MPEG ||
> +        avctx->color_range == AVCOL_RANGE_UNSPECIFIED && frame->color_range == AVCOL_RANGE_MPEG)
> +        av_log(avctx, AV_LOG_ERROR, "This encoder does not support limited (tv) range, colors will be wrong!\n");
> +    else if (avctx->color_range != AVCOL_RANGE_JPEG && frame->color_range != AVCOL_RANGE_JPEG)
> +        av_log(avctx, AV_LOG_WARNING, "Unknown color range, assuming full (pc)\n");
>  
>      /* bitexact lossless requires there to be no XYB transform */
>      info.uses_original_profile = ctx->distance == 0.0;
>  
> -    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
> -    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS) {
> -        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
> -    } else if (info.uses_original_profile) {
> -        /*
> -        * the color encoding is not used if uses_original_profile is false
> -        * this just works around a bug in libjxl 0.7.0 and lower
> -        */
> -        if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS) {
> -            av_log(avctx, AV_LOG_ERROR, "Failed to set JxlColorEncoding\n");
> -            return AVERROR_EXTERNAL;
> -        }
> -    }
> -
>      if (JxlEncoderSetBasicInfo(ctx->encoder, &info) != JXL_ENC_SUCCESS) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to set JxlBasicInfo\n");
>          return AVERROR_EXTERNAL;
>      }
>  
> +    /* rendering intent doesn't matter here
> +     * but libjxl will whine if we don't set it */
> +    jxl_color.rendering_intent = JXL_RENDERING_INTENT_RELATIVE;
> +
> +    switch (avctx->color_trc && avctx->color_trc != AVCOL_TRC_UNSPECIFIED
> +            ? avctx->color_trc : frame->color_trc) {
> +    case AVCOL_TRC_BT709:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_709;
> +        break;
> +    case AVCOL_TRC_LINEAR:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
> +        break;
> +    case AVCOL_TRC_IEC61966_2_1:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
> +        break;
> +    case AVCOL_TRC_SMPTE428:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_DCI;
> +        break;
> +    case AVCOL_TRC_SMPTE2084:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_PQ;
> +        break;
> +    case AVCOL_TRC_ARIB_STD_B67:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_HLG;
> +        break;
> +    case AVCOL_TRC_GAMMA22:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
> +        jxl_color.gamma = 2.2;
> +        break;
> +    case AVCOL_TRC_GAMMA28:
> +        jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_GAMMA;
> +        jxl_color.gamma = 2.8;
> +        break;
> +    default:
> +        if (pix_desc->flags & AV_PIX_FMT_FLAG_FLOAT) {
> +            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming Linear Light\n");
> +            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_LINEAR;
> +        } else {
> +            av_log(avctx, AV_LOG_WARNING, "Unknown transfer function, assuming IEC61966-2-1/sRGB\n");
> +            jxl_color.transfer_function = JXL_TRANSFER_FUNCTION_SRGB;
> +        }

The way this code is written, this is also the error you get when
specifying an explicit transfer function that is not in the list of
transfer functions. I feel like that deserves the same type of "not
supported, colors will definitely be wrong" error as the range mismatch
above.

> +    }
> +
> +    /* This should be implied to be honest
> +     * but a libjxl bug makes it fail otherwise */
> +    if (info.num_color_channels == 1)
> +        jxl_color.color_space = JXL_COLOR_SPACE_GRAY;
> +    else
> +        jxl_color.color_space = JXL_COLOR_SPACE_RGB;
> +
> +    ret = libjxl_populate_primaries(&jxl_color,
> +            avctx->color_primaries && avctx->color_primaries != AVCOL_PRI_UNSPECIFIED
> +            ? avctx->color_primaries : frame->color_primaries);
> +    if (ret < 0)
> +        return ret;
> +
> +    sd = av_frame_get_side_data(frame, AV_FRAME_DATA_ICC_PROFILE);
> +    if (sd && sd->size && JxlEncoderSetICCProfile(ctx->encoder, sd->data, sd->size) != JXL_ENC_SUCCESS)
> +        av_log(avctx, AV_LOG_WARNING, "Could not set ICC Profile\n");
> +    if (JxlEncoderSetColorEncoding(ctx->encoder, &jxl_color) != JXL_ENC_SUCCESS)
> +        av_log(avctx, AV_LOG_WARNING, "Failed to set JxlColorEncoding\n");
> +
> +    /* depending on basic info, level 10 might
> +     * be required instead of level 5 */
> +    if (JxlEncoderGetRequiredCodestreamLevel(ctx->encoder) > 5) {
> +        if (JxlEncoderSetCodestreamLevel(ctx->encoder, 10) != JXL_ENC_SUCCESS)
> +            av_log(avctx, AV_LOG_WARNING, "Could not increase codestream level\n");
> +    }
> +
>      jxl_fmt.endianness = JXL_NATIVE_ENDIAN;
>      jxl_fmt.align = frame->linesize[0];
>  
> -- 
> 2.36.1
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes

[Leo Izen]

On 6/1/22 22:14, Leo Izen wrote:
> Changes in v8:
> - Use avutil/csp for both encoding and decoding
> - Handle the non-XYB case with an attached ICC Profile on decoding
> - clean up some code and segment it out to static functions
> 
> Leo Izen (2):
>    avcodec/libjxldec: properly tag output colorspace
>    avcodec/libjxlenc: properly read input colorspace
> 
>   libavcodec/libjxldec.c | 142 +++++++++++++++++++++++++++++++++++---
>   libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
>   2 files changed, 256 insertions(+), 39 deletions(-)
> 

I believe this should make it into 5.1 as it fixes a known bug and 
improves existing behavior.

- Leo Izen (thebombzen)
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes

[Leo Izen]

On 6/9/22 07:31, Leo Izen wrote:
> On 6/1/22 22:14, Leo Izen wrote:
>> Changes in v8:
>> - Use avutil/csp for both encoding and decoding
>> - Handle the non-XYB case with an attached ICC Profile on decoding
>> - clean up some code and segment it out to static functions
>>
>> Leo Izen (2):
>>    avcodec/libjxldec: properly tag output colorspace
>>    avcodec/libjxlenc: properly read input colorspace
>>
>>   libavcodec/libjxldec.c | 142 +++++++++++++++++++++++++++++++++++---
>>   libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
>>   2 files changed, 256 insertions(+), 39 deletions(-)
>>
> 
> I believe this should make it into 5.1 as it fixes a known bug and 
> improves existing behavior.
> 
> - Leo Izen (thebombzen)

Bumping again for review.

- Leo Izen (thebombzen)
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH v8 0/2] libjxl Colorspace Fixes

[Leo Izen]

On 6/15/22 10:31, Leo Izen wrote:
> On 6/9/22 07:31, Leo Izen wrote:
>> On 6/1/22 22:14, Leo Izen wrote:
>>> Changes in v8:
>>> - Use avutil/csp for both encoding and decoding
>>> - Handle the non-XYB case with an attached ICC Profile on decoding
>>> - clean up some code and segment it out to static functions
>>>
>>> Leo Izen (2):
>>>    avcodec/libjxldec: properly tag output colorspace
>>>    avcodec/libjxlenc: properly read input colorspace
>>>
>>>   libavcodec/libjxldec.c | 142 +++++++++++++++++++++++++++++++++++---
>>>   libavcodec/libjxlenc.c | 153 +++++++++++++++++++++++++++++++++--------
>>>   2 files changed, 256 insertions(+), 39 deletions(-)
>>>
>>
>> I believe this should make it into 5.1 as it fixes a known bug and 
>> improves existing behavior.
>>
>> - Leo Izen (thebombzen)
> 
> Bumping again for review.
> 
> - Leo Izen (thebombzen)

Could someone please review this? Thanks.

- Leo Izen (thebombzen)
_______________________________________________
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".