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

[Jan Ekström]

On Tue, Aug 22, 2023 at 12:59 AM Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> wrote:
>
> Jan Ekström:
> > On Sun, Aug 20, 2023 at 10:10 AM Andreas Rheinhardt
> > <andreas.rheinhardt at 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