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

Vittorio Giovara vittorio.giovara at gmail.com
Wed Feb 26 07:32:58 EET 2020


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

> On 25/02/2020 04:32, Vittorio Giovara wrote:
> > On Mon, Feb 24, 2020 at 5:18 PM Mark Thompson <sw at jkqxz.net> wrote:
> >> 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.
> >>
> >
> > 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.
-- 
Vittorio


More information about the ffmpeg-devel mailing list