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

[Andreas Rheinhardt]

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.

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