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

Jan Ekström jeebjp at gmail.com
Mon Aug 2 12:25:01 EEST 2021


On Fri, Jul 30, 2021 at 9:51 PM Christopher Degawa <ccom at randomderp.com> wrote:
>
> 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
>

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


More information about the ffmpeg-devel mailing list