[FFmpeg-devel] [PATCH] libsvtav1: pass color description info

[Jan Ekström]

On Fri, Jul 23, 2021 at 5:02 AM Christopher Degawa <ccom at randomderp.com> wrote:
>
> these fields are only available past svt-av1 0.8.7
>
> Signed-off-by: Christopher Degawa <ccom at randomderp.com>
> ---
>  libavcodec/libsvtav1.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index fabc4e6428..6c12777911 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -37,6 +37,14 @@
>  #include "avcodec.h"
>  #include "profiles.h"
>
> +#ifndef SVTAV1_MAKE_VERSION
> +#define SVTAV1_MAKE_VERSION(x,y,z) ((x) << 16 | (y) << 8 | z)
> +#endif
> +
> +#ifndef SVTAV1_CURR_VERSION
> +#define SVTAV1_CURR_VERSION SVTAV1_MAKE_VERSION(SVT_VERSION_MAJOR, SVT_VERSION_MINOR, SVT_VERSION_PATCHLEVEL)
> +#endif
> +

How new SVT-AV1 would be required to have these macros? They are
sensible but if it's not a large bump due to SVT-AV1 being a
relatively new project it might just make sense to bump the
requirement to keep ifdefs out of the module for now.

>  typedef enum eos_status {
>      EOS_NOT_REACHED = 0,
>      EOS_SENT,
> @@ -218,6 +226,18 @@ static int config_enc_params(EbSvtAv1EncConfiguration *param,
>      param->tile_columns = svt_enc->tile_columns;
>      param->tile_rows    = svt_enc->tile_rows;
>
> +#if SVTAV1_CURR_VERSION >= SVTAV1_MAKE_VERSION(0, 8, 7)
> +    if (desc->flags & AV_PIX_FMT_FLAG_RGB) {
> +        param->color_primaries = AVCOL_PRI_BT709;
> +        param->matrix_coefficients = AVCOL_SPC_RGB;
> +        param->transfer_characteristics = AVCOL_TRC_IEC61966_2_1;

I would limit to forcing the AVCOL_SPC_RGB. It is valid to f.ex.
encode RGB with BT.2020 primaries and PQ transfer function. And if the
other values are not set then they're effectively unknown. Thus maybe
it makes sense to either set values, or set them if they are not
_UNSPECIFIED (depending on if SVT-AV1 handles unset with a different
value to _UNSPECIFIED) - and then in case of RGB make sure that the
matrix coefficients are set to RGB? That way the if should be very
short and otherwise the two cases would share code.

> +    } else {
> +        param->color_primaries = avctx->color_primaries;
> +        param->matrix_coefficients = avctx->colorspace;
> +        param->transfer_characteristics = avctx->color_trc;

Out of interest, what about chroma location? (although now that I
checked, it seems to be mostly not passed in many other encoder
wrappers - so this is not a blocker :<)

> +    }
> +#endif
> +
>      return 0;
>  }
>
> --
> 2.32.0

Jan