[FFmpeg-devel,v3,10/12] avcodec/libsvtav1: add support for writing out CLL and MDCV

These two were added in 28e23d7f348c78d49a726c7469f9d4e38edec341
and 3558c1f2e97455e0b89edef31b9a72ab7fa30550 for version 0.9.0 of
SVT-AV1, which is also our minimum requirement right now.

In other words, no additional version limiting conditions seem
to be required.

Additionally, add a FATE test which verifies that pass-through of
the MDCV/CLL side data is working during encoding.
---
 libavcodec/libsvtav1.c         | 70 ++++++++++++++++++++++++++++++++++
 tests/fate/enc_external.mak    |  5 +++
 tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
 3 files changed, 89 insertions(+)
 create mode 100644 tests/ref/fate/libsvtav1-hdr10

Jan Ekström:
> These two were added in 28e23d7f348c78d49a726c7469f9d4e38edec341
> and 3558c1f2e97455e0b89edef31b9a72ab7fa30550 for version 0.9.0 of
> SVT-AV1, which is also our minimum requirement right now.
> 
> In other words, no additional version limiting conditions seem
> to be required.
> 
> Additionally, add a FATE test which verifies that pass-through of
> the MDCV/CLL side data is working during encoding.
> ---
>  libavcodec/libsvtav1.c         | 70 ++++++++++++++++++++++++++++++++++
>  tests/fate/enc_external.mak    |  5 +++
>  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
>  3 files changed, 89 insertions(+)
>  create mode 100644 tests/ref/fate/libsvtav1-hdr10
> 
> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> index f2b73361d8..d4f9fa14ba 100644
> --- a/libavcodec/libsvtav1.c
> +++ b/libavcodec/libsvtav1.c
> @@ -24,9 +24,11 @@
>  #include <EbSvtAv1ErrorCodes.h>
>  #include <EbSvtAv1Enc.h>
>  
> +#include "libavutil/bswap.h"
>  #include "libavutil/common.h"
>  #include "libavutil/frame.h"
>  #include "libavutil/imgutils.h"
> +#include "libavutil/mastering_display_metadata.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/pixdesc.h"
>  #include "libavutil/avassert.h"
> @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
>  
>  }
>  
> +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
> +                        const AVMasteringDisplayMetadata *mdcv)
> +{
> +    struct EbSvtAv1ChromaPoints *points[] = {
> +        &dst->r,
> +        &dst->g,
> +        &dst->b,
> +    };
> +
> +    if (!mdcv->has_primaries)
> +        goto skip_primaries;
> +
> +    for (int i = 0; i < 3; i++) {
> +        struct EbSvtAv1ChromaPoints *dst = points[i];
> +        const  AVRational           *src = mdcv->display_primaries[i];
> +
> +        dst->x =
> +            AV_BSWAP16C(av_rescale_q(1, src[0],
> +                                     (AVRational){ 1, (1 << 16) }));

Can you explain what's the matter with the AV_BSWAP16C here? And if I am
not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
<< 16) (if we had multiplications of AVRationals by integers).

> +        dst->y =
> +            AV_BSWAP16C(av_rescale_q(1, src[1],
> +                                     (AVRational){ 1, (1 << 16) }));
> +    }
> +
> +    dst->white_point.x =
> +        AV_BSWAP16C(av_rescale_q(1, mdcv->white_point[0],
> +                                 (AVRational){ 1, (1 << 16) }));
> +    dst->white_point.y =
> +        AV_BSWAP16C(av_rescale_q(1, mdcv->white_point[1],
> +                                 (AVRational){ 1, (1 << 16) }));
> +
> +skip_primaries:
> +    if (!mdcv->has_luminance)
> +        return;
> +
> +    dst->max_luma =
> +        AV_BSWAP32C(av_rescale_q(1, mdcv->max_luminance,
> +                                 (AVRational){ 1, (1 << 8) }));
> +    dst->min_luma =
> +        AV_BSWAP32C(av_rescale_q(1, mdcv->min_luminance,
> +                                 (AVRational){ 1, (1 << 14) }));
> +}
> +
> +static void handle_side_data(AVCodecContext *avctx,
> +                             EbSvtAv1EncConfiguration *param)
> +{
> +    const AVFrameSideDataSet set = avctx->side_data_set;
> +    const AVFrameSideData *cll_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_CONTENT_LIGHT_LEVEL);
> +    const AVFrameSideData *mdcv_sd =
> +        av_side_data_set_get_item(set, AV_FRAME_DATA_MASTERING_DISPLAY_METADATA);
> +
> +    if (cll_sd) {
> +        const AVContentLightMetadata *cll =
> +            (AVContentLightMetadata *)cll_sd->data;
> +
> +        param->content_light_level.max_cll  = AV_BSWAP16C(cll->MaxCLL);
> +        param->content_light_level.max_fall = AV_BSWAP16C(cll->MaxFALL);
> +    }
> +
> +    if (mdcv_sd) {
> +        handle_mdcv(&param->mastering_display,
> +                    (AVMasteringDisplayMetadata *)mdcv_sd->data);
> +    }
> +}
> +
>  static int config_enc_params(EbSvtAv1EncConfiguration *param,
>                               AVCodecContext *avctx)
>  {
> @@ -273,6 +341,8 @@ FF_ENABLE_DEPRECATION_WARNINGS
>      /* 2 = IDR, closed GOP, 1 = CRA, open GOP */
>      param->intra_refresh_type = avctx->flags & AV_CODEC_FLAG_CLOSED_GOP ? 2 : 1;
>  
> +    handle_side_data(avctx, param);
> +
>  #if SVT_AV1_CHECK_VERSION(0, 9, 1)
>      while ((en = av_dict_get(svt_enc->svtav1_opts, "", en, AV_DICT_IGNORE_SUFFIX))) {
>          EbErrorType ret = svt_av1_enc_parse_parameter(param, en->key, en->value);
> diff --git a/tests/fate/enc_external.mak b/tests/fate/enc_external.mak
> index 7eabebcc51..d787941c16 100644
> --- a/tests/fate/enc_external.mak
> +++ b/tests/fate/enc_external.mak
> @@ -2,5 +2,10 @@ FATE_ENC_EXTERNAL-$(call ENCDEC, LIBX264 H264, MOV, H264_DEMUXER) += fate-libx26
>  fate-libx264-simple: CMD = enc_external $(TARGET_SAMPLES)/h264-conformance/BA1_Sony_D.jsv \
>      mp4 "-c:v libx264" "-show_entries frame=width,height,pix_fmt,pts,pkt_dts -of flat"
>  
> +# test for SVT-AV1 MDCV and CLL passthrough during encoding
> +FATE_ENC_EXTERNAL-$(call ENCDEC, LIBSVTAV1 HEVC, MOV, HEVC_DEMUXER LIBDAV1D_DECODER) += fate-libsvtav1-hdr10
> +fate-libsvtav1-hdr10: CMD = enc_external $(TARGET_SAMPLES)/hevc/hdr10_plus_h265_sample.hevc \
> +    mp4 "-c:v libsvtav1" "-show_frames -show_entries frame=side_data_list -of flat"
> +
>  FATE_SAMPLES_FFMPEG_FFPROBE += $(FATE_ENC_EXTERNAL-yes)
>  fate-enc-external: $(FATE_ENC_EXTERNAL-yes)
> diff --git a/tests/ref/fate/libsvtav1-hdr10 b/tests/ref/fate/libsvtav1-hdr10
> new file mode 100644
> index 0000000000..6f0d34903b
> --- /dev/null
> +++ b/tests/ref/fate/libsvtav1-hdr10
> @@ -0,0 +1,14 @@
> +frames.frame.0.side_data_list.side_data.0.side_data_type="Mastering display metadata"
> +frames.frame.0.side_data_list.side_data.0.red_x="17367/65536"
> +frames.frame.0.side_data_list.side_data.0.red_y="45220/65536"
> +frames.frame.0.side_data_list.side_data.0.green_x="9830/65536"
> +frames.frame.0.side_data_list.side_data.0.green_y="3932/65536"
> +frames.frame.0.side_data_list.side_data.0.blue_x="44564/65536"
> +frames.frame.0.side_data_list.side_data.0.blue_y="20972/65536"
> +frames.frame.0.side_data_list.side_data.0.white_point_x="20493/65536"
> +frames.frame.0.side_data_list.side_data.0.white_point_y="21561/65536"
> +frames.frame.0.side_data_list.side_data.0.min_luminance="82/16384"
> +frames.frame.0.side_data_list.side_data.0.max_luminance="256000/256"
> +frames.frame.0.side_data_list.side_data.1.side_data_type="Content light level metadata"
> +frames.frame.0.side_data_list.side_data.1.max_content=1000
> +frames.frame.0.side_data_list.side_data.1.max_average=200

On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > These two were added in 28e23d7f348c78d49a726c7469f9d4e38edec341
> > and 3558c1f2e97455e0b89edef31b9a72ab7fa30550 for version 0.9.0 of
> > SVT-AV1, which is also our minimum requirement right now.
> >
> > In other words, no additional version limiting conditions seem
> > to be required.
> >
> > Additionally, add a FATE test which verifies that pass-through of
> > the MDCV/CLL side data is working during encoding.
> > ---
> >  libavcodec/libsvtav1.c         | 70 ++++++++++++++++++++++++++++++++++
> >  tests/fate/enc_external.mak    |  5 +++
> >  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
> >  3 files changed, 89 insertions(+)
> >  create mode 100644 tests/ref/fate/libsvtav1-hdr10
> >
> > diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> > index f2b73361d8..d4f9fa14ba 100644
> > --- a/libavcodec/libsvtav1.c
> > +++ b/libavcodec/libsvtav1.c
> > @@ -24,9 +24,11 @@
> >  #include <EbSvtAv1ErrorCodes.h>
> >  #include <EbSvtAv1Enc.h>
> >
> > +#include "libavutil/bswap.h"
> >  #include "libavutil/common.h"
> >  #include "libavutil/frame.h"
> >  #include "libavutil/imgutils.h"
> > +#include "libavutil/mastering_display_metadata.h"
> >  #include "libavutil/opt.h"
> >  #include "libavutil/pixdesc.h"
> >  #include "libavutil/avassert.h"
> > @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
> >
> >  }
> >
> > +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
> > +                        const AVMasteringDisplayMetadata *mdcv)
> > +{
> > +    struct EbSvtAv1ChromaPoints *points[] = {
> > +        &dst->r,
> > +        &dst->g,
> > +        &dst->b,
> > +    };
> > +
> > +    if (!mdcv->has_primaries)
> > +        goto skip_primaries;
> > +
> > +    for (int i = 0; i < 3; i++) {
> > +        struct EbSvtAv1ChromaPoints *dst = points[i];
> > +        const  AVRational           *src = mdcv->display_primaries[i];
> > +
> > +        dst->x =
> > +            AV_BSWAP16C(av_rescale_q(1, src[0],
> > +                                     (AVRational){ 1, (1 << 16) }));
>
> Can you explain what's the matter with the AV_BSWAP16C here? And if I am
> not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
> << 16) (if we had multiplications of AVRationals by integers).

It's basically a way that I got the big-endian values written out to
the uint16_t. Same goes for the uint32_t values and AV_BSWAP32C. Not
sure if there is a better macro for handling this on both BE and LE.

SVT-AV1 seems to just write these values into the bit stream as-is and
the interface seems to be defined around that (f.ex. in the string
based interface `x << 8 | x >> 8` is always done to the parsed value).

https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/Lib/Encoder/Codec/EbEntropyCoding.c#L3626-3637

Jan

Jan Ekström:
> On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> Jan Ekström:
>>> These two were added in 28e23d7f348c78d49a726c7469f9d4e38edec341
>>> and 3558c1f2e97455e0b89edef31b9a72ab7fa30550 for version 0.9.0 of
>>> SVT-AV1, which is also our minimum requirement right now.
>>>
>>> In other words, no additional version limiting conditions seem
>>> to be required.
>>>
>>> Additionally, add a FATE test which verifies that pass-through of
>>> the MDCV/CLL side data is working during encoding.
>>> ---
>>>  libavcodec/libsvtav1.c         | 70 ++++++++++++++++++++++++++++++++++
>>>  tests/fate/enc_external.mak    |  5 +++
>>>  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
>>>  3 files changed, 89 insertions(+)
>>>  create mode 100644 tests/ref/fate/libsvtav1-hdr10
>>>
>>> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
>>> index f2b73361d8..d4f9fa14ba 100644
>>> --- a/libavcodec/libsvtav1.c
>>> +++ b/libavcodec/libsvtav1.c
>>> @@ -24,9 +24,11 @@
>>>  #include <EbSvtAv1ErrorCodes.h>
>>>  #include <EbSvtAv1Enc.h>
>>>
>>> +#include "libavutil/bswap.h"
>>>  #include "libavutil/common.h"
>>>  #include "libavutil/frame.h"
>>>  #include "libavutil/imgutils.h"
>>> +#include "libavutil/mastering_display_metadata.h"
>>>  #include "libavutil/opt.h"
>>>  #include "libavutil/pixdesc.h"
>>>  #include "libavutil/avassert.h"
>>> @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
>>>
>>>  }
>>>
>>> +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
>>> +                        const AVMasteringDisplayMetadata *mdcv)
>>> +{
>>> +    struct EbSvtAv1ChromaPoints *points[] = {
>>> +        &dst->r,
>>> +        &dst->g,
>>> +        &dst->b,
>>> +    };
>>> +
>>> +    if (!mdcv->has_primaries)
>>> +        goto skip_primaries;
>>> +
>>> +    for (int i = 0; i < 3; i++) {
>>> +        struct EbSvtAv1ChromaPoints *dst = points[i];
>>> +        const  AVRational           *src = mdcv->display_primaries[i];
>>> +
>>> +        dst->x =
>>> +            AV_BSWAP16C(av_rescale_q(1, src[0],
>>> +                                     (AVRational){ 1, (1 << 16) }));
>>
>> Can you explain what's the matter with the AV_BSWAP16C here? And if I am
>> not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
>> << 16) (if we had multiplications of AVRationals by integers).
> 
> It's basically a way that I got the big-endian values written out to
> the uint16_t. Same goes for the uint32_t values and AV_BSWAP32C. 

This would only work on a little endian system (on big endian systems,
this approach would write the values as little-endian); the correct way
to write BE is via AV_WB16/32.

Not
> sure if there is a better macro for handling this on both BE and LE.
> 
> SVT-AV1 seems to just write these values into the bit stream as-is and
> the interface seems to be defined around that (f.ex. in the string
> based interface `x << 8 | x >> 8` is always done to the parsed value).

Honestly, I do not get why they are using intswap16/32 unconditionally
even for BE systems. It seems to contradict their own documentation of
always putting BE values into the struct.

> 
> https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/Lib/Encoder/Codec/EbEntropyCoding.c#L3626-3637
> 

The correct reference would be
https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/API/EbSvtAv1Enc.h#L93
("values are stored in BE format"); you should not rely on internals of
other projects.
The reason for using BE should be explicitly documented.

- Andreas

On Tue, Aug 22, 2023 at 12:59 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Jan Ekström:
> > On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
> > <andreas.rheinhardt@outlook.com> wrote:
> >>
> >> Jan Ekström:
> >>> These two were added in 28e23d7f348c78d49a726c7469f9d4e38edec341
> >>> and 3558c1f2e97455e0b89edef31b9a72ab7fa30550 for version 0.9.0 of
> >>> SVT-AV1, which is also our minimum requirement right now.
> >>>
> >>> In other words, no additional version limiting conditions seem
> >>> to be required.
> >>>
> >>> Additionally, add a FATE test which verifies that pass-through of
> >>> the MDCV/CLL side data is working during encoding.
> >>> ---
> >>>  libavcodec/libsvtav1.c         | 70 ++++++++++++++++++++++++++++++++++
> >>>  tests/fate/enc_external.mak    |  5 +++
> >>>  tests/ref/fate/libsvtav1-hdr10 | 14 +++++++
> >>>  3 files changed, 89 insertions(+)
> >>>  create mode 100644 tests/ref/fate/libsvtav1-hdr10
> >>>
> >>> diff --git a/libavcodec/libsvtav1.c b/libavcodec/libsvtav1.c
> >>> index f2b73361d8..d4f9fa14ba 100644
> >>> --- a/libavcodec/libsvtav1.c
> >>> +++ b/libavcodec/libsvtav1.c
> >>> @@ -24,9 +24,11 @@
> >>>  #include <EbSvtAv1ErrorCodes.h>
> >>>  #include <EbSvtAv1Enc.h>
> >>>
> >>> +#include "libavutil/bswap.h"
> >>>  #include "libavutil/common.h"
> >>>  #include "libavutil/frame.h"
> >>>  #include "libavutil/imgutils.h"
> >>> +#include "libavutil/mastering_display_metadata.h"
> >>>  #include "libavutil/opt.h"
> >>>  #include "libavutil/pixdesc.h"
> >>>  #include "libavutil/avassert.h"
> >>> @@ -146,6 +148,72 @@ static int alloc_buffer(EbSvtAv1EncConfiguration *config, SvtContext *svt_enc)
> >>>
> >>>  }
> >>>
> >>> +static void handle_mdcv(struct EbSvtAv1MasteringDisplayInfo *dst,
> >>> +                        const AVMasteringDisplayMetadata *mdcv)
> >>> +{
> >>> +    struct EbSvtAv1ChromaPoints *points[] = {
> >>> +        &dst->r,
> >>> +        &dst->g,
> >>> +        &dst->b,
> >>> +    };
> >>> +
> >>> +    if (!mdcv->has_primaries)
> >>> +        goto skip_primaries;
> >>> +
> >>> +    for (int i = 0; i < 3; i++) {
> >>> +        struct EbSvtAv1ChromaPoints *dst = points[i];
> >>> +        const  AVRational           *src = mdcv->display_primaries[i];
> >>> +
> >>> +        dst->x =
> >>> +            AV_BSWAP16C(av_rescale_q(1, src[0],
> >>> +                                     (AVRational){ 1, (1 << 16) }));
> >>
> >> Can you explain what's the matter with the AV_BSWAP16C here? And if I am
> >> not mistaken, then the av_rescale_q() above is equivalent to src[0] * (1
> >> << 16) (if we had multiplications of AVRationals by integers).
> >
> > It's basically a way that I got the big-endian values written out to
> > the uint16_t. Same goes for the uint32_t values and AV_BSWAP32C.
>
> This would only work on a little endian system (on big endian systems,
> this approach would write the values as little-endian); the correct way
> to write BE is via AV_WB16/32.
>

Yup, that was the correct one (I knew mine was technically incorrect
as it would always do the swap, but kept it as part of the first
versions). Thanks.

So it ends up being like the following as the WB macros deal with
pointers and not in x = THING() style.

AV_WB16(&dst->x,
        av_rescale_q(1, src[0], (AVRational){ 1, (1 << 16) }));

Applied this as well as the more classic if (has_primaries) style that
you mentioned with the libx264 wrapper patch in my v4 branch.

> Not
> > sure if there is a better macro for handling this on both BE and LE.
> >
> > SVT-AV1 seems to just write these values into the bit stream as-is and
> > the interface seems to be defined around that (f.ex. in the string
> > based interface `x << 8 | x >> 8` is always done to the parsed value).
>
> Honestly, I do not get why they are using intswap16/32 unconditionally
> even for BE systems. It seems to contradict their own documentation of
> always putting BE values into the struct.
>

Either an oversight, or due to BE not really being a consideration
(which kind of makes sense with even PPC going LE now). Not that I can
do much more than guess.

> >
> > https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/Lib/Encoder/Codec/EbEntropyCoding.c#L3626-3637
> >
>
> The correct reference would be
> https://gitlab.com/AOMediaCodec/SVT-AV1/-/blob/7233e7ead5fe8a0f1dc33b91aba0c5a24cb9f673/Source/API/EbSvtAv1Enc.h#L93
> ("values are stored in BE format"); you should not rely on internals of
> other projects.
> The reason for using BE should be explicitly documented.
>

I based the decision of putting BE values there on the public headers.
I only looked into the internals after writing "seems to just write
these values into the bit stream", and then ending up looking at the
internals to see whether it looked like that or not.

Jan