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

[Christopher Degawa]

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

> On Fri, Jul 23, 2021 at 5:02 AM Christopher Degawa <ccom at 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.