[FFmpeg-devel] [PATCH v4 18/21] cbs_h265: Add functions to turn HDR metadata into SEI

[Mark Thompson]

On 24/02/2020 21:28, Vittorio Giovara wrote:
> On Sun, Feb 23, 2020 at 6:41 PM Mark Thompson <sw at jkqxz.net> wrote:
> 
>> ---
>>  libavcodec/Makefile   |  2 +-
>>  libavcodec/cbs_h265.c | 99 +++++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/cbs_h265.h | 18 ++++++++
>>  3 files changed, 118 insertions(+), 1 deletion(-)
>>  create mode 100644 libavcodec/cbs_h265.c
>>
>> ...
>> +void
>> ff_cbs_h265_fill_sei_mastering_display(H265RawSEIMasteringDisplayColourVolume
>> *mdcv,
>> +                                            const
>> AVMasteringDisplayMetadata *mdm)
>> +{
>> +    memset(mdcv, 0, sizeof(*mdcv));
>> +
>> +    if (mdm->has_primaries) {
>> +        // The values in the metadata structure are fractions between 0
>> and 1,
>> +        // while the SEI message contains fixed-point values with an
>> increment
>> +        // of 0.00002.  So, scale up by 50000 to convert between them.
>> +
>> +        for (int a = 0; a < 3; a++) {
>> +            // The metadata structure stores this in RGB order, but the
>> SEI
>> +            // wants it in GBR order.
>> +            int b = (a + 1) % 3;
>>
> 
> this is a pretty minor comment, but do you think you could use the more
> legible way present in other parts of the codebase?
>         const int mapping[3] = {2, 0, 1};
> rather than (a + 1) % 3;

Ok.

Is there a specific reason to make it on the stack rather than static?  I see it's there in hevcdec.

>> +            mdcv->display_primaries_x[a] =
>> +                rescale_fraction(mdm->display_primaries[b][0], 50000);
>> +            mdcv->display_primaries_y[a] =
>> +                rescale_fraction(mdm->display_primaries[b][1], 50000);
>> +        }
>> +
>> +        mdcv->white_point_x = rescale_fraction(mdm->white_point[0],
>> 50000);
>> +        mdcv->white_point_y = rescale_fraction(mdm->white_point[1],
>> 50000);
>> +    }
>> +
>> +    if (mdm->has_luminance) {
>> +        // Metadata are rational values in candelas per square metre, SEI
>> +        // contains fixed point in units of 0.0001 candelas per square
>> +        // metre.  So scale up by 10000 to convert between them, and clip
>> to
>> +        // ensure that we don't overflow.
>> +
>> +        mdcv->max_display_mastering_luminance =
>> +            rescale_clip(mdm->max_luminance, 10000, UINT32_MAX);
>> +        mdcv->min_display_mastering_luminance =
>> +            rescale_clip(mdm->min_luminance, 10000, UINT32_MAX);
>> +
>> +        // The spec has a hard requirement that min is less than the max,
>> +        // and the SEI-writing code enforces that.
>> +        if (!(mdcv->min_display_mastering_luminance <
>> +              mdcv->max_display_mastering_luminance)) {
>> +            if (mdcv->max_display_mastering_luminance == UINT32_MAX)
>> +                mdcv->min_display_mastering_luminance =
>> +                    mdcv->max_display_mastering_luminance - 1;
>> +            else
>> +                mdcv->max_display_mastering_luminance =
>> +                    mdcv->min_display_mastering_luminance + 1;
>> +        }
>> +    } else {
>> +        mdcv->max_display_mastering_luminance = 1;
>> +        mdcv->min_display_mastering_luminance = 0;
>> +    }
>> +}
>> ...

- Mark