Message ID | 20210723020210.317634-1-ccom@randomderp.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] libsvtav1: pass color description info | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
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
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; }
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(+)