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

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(-)

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?

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)

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