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

---
 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

On Sun, Feb 23, 2020 at 6:41 PM Mark Thompson <sw@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
>
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 0c4547f3a1..1ce079687b 100644
> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -65,7 +65,7 @@ OBJS-$(CONFIG_CABAC)                   += cabac.o
>  OBJS-$(CONFIG_CBS)                     += cbs.o
>  OBJS-$(CONFIG_CBS_AV1)                 += cbs_av1.o
>  OBJS-$(CONFIG_CBS_H264)                += cbs_h2645.o cbs_h264.o
> h2645_parse.o
> -OBJS-$(CONFIG_CBS_H265)                += cbs_h2645.o h2645_parse.o
> +OBJS-$(CONFIG_CBS_H265)                += cbs_h2645.o cbs_h265.o
> h2645_parse.o
>  OBJS-$(CONFIG_CBS_JPEG)                += cbs_jpeg.o
>  OBJS-$(CONFIG_CBS_MPEG2)               += cbs_mpeg2.o
>  OBJS-$(CONFIG_CBS_VP9)                 += cbs_vp9.o
> diff --git a/libavcodec/cbs_h265.c b/libavcodec/cbs_h265.c
> new file mode 100644
> index 0000000000..590977cf00
> --- /dev/null
> +++ b/libavcodec/cbs_h265.c
> @@ -0,0 +1,99 @@
> +/*
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> + */
> +
> +#include "libavutil/mathematics.h"
> +#include "libavutil/mastering_display_metadata.h"
> +
> +#include "cbs_h265.h"
> +
> +
> +static uint32_t rescale_clip(AVRational value, uint32_t scale, uint32_t
> max)
> +{
> +    int64_t scaled = av_rescale(scale, value.num, value.den);
> +    return av_clip64(scaled, 0, max);
> +}
> +
> +static uint32_t rescale_fraction(AVRational value, uint32_t max)
> +{
> +    return rescale_clip(value, max, max);
> +}
> +
> +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;

Vittorio

+            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;
> +    }
> +}
> +
> +void
> ff_cbs_h265_fill_sei_content_light_level(H265RawSEIContentLightLevelInfo
> *cll,
> +                                              const
> AVContentLightMetadata *clm)
> +{
> +    memset(cll, 0, sizeof(*cll));
> +
> +    // Both the metadata and the SEI are in units of candelas per square
> +    // metre, so we only need to clip to ensure that they are in the valid
> +    // range.
> +
> +    cll->max_content_light_level     = av_clip_uintp2(clm->MaxCLL,  16);
> +    cll->max_pic_average_light_level = av_clip_uintp2(clm->MaxFALL, 16);
> +}
> diff --git a/libavcodec/cbs_h265.h b/libavcodec/cbs_h265.h
> index f5eb5af5b2..793675ad41 100644
> --- a/libavcodec/cbs_h265.h
> +++ b/libavcodec/cbs_h265.h
> @@ -746,4 +746,22 @@ typedef struct CodedBitstreamH265Context {
>  } CodedBitstreamH265Context;
>
>
> +struct AVMasteringDisplayMetadata;
> +struct AVContentLightMetadata;
> +
> +/**
> + * Fill an SEI Mastering Display Colour Volume structure with values
> derived
> + * from the AVMasteringDisplayMetadata side-data structure.
> + */
> +void
> ff_cbs_h265_fill_sei_mastering_display(H265RawSEIMasteringDisplayColourVolume
> *mdcv,
> +                                            const struct
> AVMasteringDisplayMetadata *mdm);
> +
> +/**
> + * Fill an SEI Content Light Level Info structure with values derived from
> + * the AVContentLightMetadata side-data structure.
> + */
> +void
> ff_cbs_h265_fill_sei_content_light_level(H265RawSEIContentLightLevelInfo
> *cll,
> +                                              const struct
> AVContentLightMetadata *clm);
> +
> +
>  #endif /* AVCODEC_CBS_H265_H */
> --
> 2.25.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

On 24/02/2020 21:28, Vittorio Giovara wrote:
> On Sun, Feb 23, 2020 at 6:41 PM Mark Thompson <sw@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

On Mon, Feb 24, 2020 at 5:18 PM Mark Thompson <sw@jkqxz.net> wrote:

> On 24/02/2020 21:28, Vittorio Giovara wrote:
> > On Sun, Feb 23, 2020 at 6:41 PM Mark Thompson <sw@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.
>

No particular reason, I just find it more readable, if you think it's a
really bad practice then you could keep the code as is.
Thanks

On 25/02/2020 04:32, Vittorio Giovara wrote:
> On Mon, Feb 24, 2020 at 5:18 PM Mark Thompson <sw@jkqxz.net> wrote:
>> On 24/02/2020 21:28, Vittorio Giovara wrote:
>>> On Sun, Feb 23, 2020 at 6:41 PM Mark Thompson <sw@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.
>>
> 
> No particular reason, I just find it more readable, if you think it's a
> really bad practice then you could keep the code as is.

Sorry, my question wasn't very clear.  I don't mind the change.  But:

Is there a reason why the array in hevcdec (and your suggestion) is not static?  (Some sort of compiler optimisation effect I'm missing, maybe.)  Intuitively it feels like it should be static const rather than being constructed on the stack every time the function is called.

- Mark

On Tue, Feb 25, 2020 at 6:03 PM Mark Thompson <sw@jkqxz.net> wrote:

> On 25/02/2020 04:32, Vittorio Giovara wrote:
> > On Mon, Feb 24, 2020 at 5:18 PM Mark Thompson <sw@jkqxz.net> wrote:
> >> On 24/02/2020 21:28, Vittorio Giovara wrote:
> >>> On Sun, Feb 23, 2020 at 6:41 PM Mark Thompson <sw@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.
> >>
> >
> > No particular reason, I just find it more readable, if you think it's a
> > really bad practice then you could keep the code as is.
>
> Sorry, my question wasn't very clear.  I don't mind the change.  But:
>
> Is there a reason why the array in hevcdec (and your suggestion) is not
> static?  (Some sort of compiler optimisation effect I'm missing, maybe.)
> Intuitively it feels like it should be static const rather than being
> constructed on the stack every time the function is called.
>

Oops no, there isn't any to my knowledge, feel free to add it though.