[FFmpeg-devel] libsvtav1: pass color description info

these fields are only available past svt-av1 0.8.7

Signed-off-by: Christopher Degawa <ccom@randomderp.com>
---
 libavcodec/libsvtav1.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

On Thu, Jul 22, 2021 at 9:02 PM Christopher Degawa <ccom@randomderp.com>
wrote:

> these fields are only available past svt-av1 0.8.7
>
> Signed-off-by: Christopher Degawa <ccom@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
> +
>  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;
> +    } else {
> +        param->color_primaries = avctx->color_primaries;
> +        param->matrix_coefficients = avctx->colorspace;
> +        param->transfer_characteristics = avctx->color_trc;
> +    }
> +#endif
> +
>      return 0;
>  }
>
> --
> 2.32.0
>

ping

On Fri, Jul 23, 2021 at 5:02 AM Christopher Degawa <ccom@randomderp.com> wrote:
>
> these fields are only available past svt-av1 0.8.7
>
> Signed-off-by: Christopher Degawa <ccom@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

On Fri, Jul 30, 2021 at 4:48 AM Jan Ekström <jeebjp@gmail.com> wrote:

> On Fri, Jul 23, 2021 at 5:02 AM Christopher Degawa <ccom@randomderp.com>
> wrote:
> > +#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.
>

They aren't in svt-av1 at this moment, I could change this to where it just
requires 0.8.7 from pkg-config in the configure script and that would
probably be for the better considering the location of
EbSvtAv1EncConfiguration inside SvtContext


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

This portion is partly copy/pasted from libaomenc.c and internally svt-av1
uses similar/exact code that aom uses when writing out and those changes
were done by Lynne and James, so I would probably defer to them on this one
https://github.com/FFmpeg/FFmpeg/commit/6a2f3f60ae02c8c3c62645b1d54ecc86bb21080d#diff-6a73483f2ead14e3748e317541bafceb16ef47ecc0011d1924adbfceb7f9b2ceR757
https://github.com/FFmpeg/FFmpeg/commit/36e51c190bb9cca4bb846e7dae4aebc6570ff258#diff-6a73483f2ead14e3748e317541bafceb16ef47ecc0011d1924adbfceb7f9b2ceR751

> +    } 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 :<)
>

So far, out of the av1 encoder libraries present in ffmpeg, only rav1e
seems to pass that. Looking inside svt-av1, we have a field
"chroma_sample_position" in one of the structs, however that struct isn't
used in the API nor do we have any code to pass that field around, so it's
practically useless right now. I could try adding that as an API accessible
field, but that would require 0.8.8 at the minimum as svt-av1 doesn't have
any way to determine API stuff incrementally in the headers.

On Fri, Jul 30, 2021 at 9:51 PM Christopher Degawa <ccom@randomderp.com> wrote:
>
> On Fri, Jul 30, 2021 at 4:48 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> > On Fri, Jul 23, 2021 at 5:02 AM Christopher Degawa <ccom@randomderp.com>
> > wrote:
> > > +#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.
> >
>
> They aren't in svt-av1 at this moment, I could change this to where it just
> requires 0.8.7 from pkg-config in the configure script and that would
> probably be for the better considering the location of
> EbSvtAv1EncConfiguration inside SvtContext
>
>
> > > +#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.
> >
>
> This portion is partly copy/pasted from libaomenc.c and internally svt-av1
> uses similar/exact code that aom uses when writing out and those changes
> were done by Lynne and James, so I would probably defer to them on this one
> https://github.com/FFmpeg/FFmpeg/commit/6a2f3f60ae02c8c3c62645b1d54ecc86bb21080d#diff-6a73483f2ead14e3748e317541bafceb16ef47ecc0011d1924adbfceb7f9b2ceR757
> https://github.com/FFmpeg/FFmpeg/commit/36e51c190bb9cca4bb846e7dae4aebc6570ff258#diff-6a73483f2ead14e3748e317541bafceb16ef47ecc0011d1924adbfceb7f9b2ceR751
>

Interesting, I do see this stuff in libaom and rav1e as well... Is
there somewhere in a spec in AV1 that the only primaries and transfer
function for RGB that is supported out of the possibilities is
specifically sRGB? In rav1e the check is at least called "is_srgb",
but it seems to be utilized for the sanity checks required for RGB
(4:4:4 being utilized etc).

My guesstimate is that the spec actually doesn't require sRGB
specifically, and this is just some sort of cargo cult being done...
Otherwise basically the encoders would have to error out in case you
try to configure them to something else than sRGB (since silently
ignoring set metadata is not nice)? Setting the matrix coefficients
for RGB automagically if the library doesn't do it via a separate
pixel/sample format separation of course makes sense.

Jan

> > +    } 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 :<)
> >
>
> So far, out of the av1 encoder libraries present in ffmpeg, only rav1e
> seems to pass that. Looking inside svt-av1, we have a field
> "chroma_sample_position" in one of the structs, however that struct isn't
> used in the API nor do we have any code to pass that field around, so it's
> practically useless right now. I could try adding that as an API accessible
> field, but that would require 0.8.8 at the minimum as svt-av1 doesn't have
> any way to determine API stuff incrementally in the headers.

I think it's worth adding support for setting at some point (unless
AV1 got rid of this value compared to H.273/4 values), but it's great
that you're adding support for color space flagging :) .

Jan