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

[Vittorio Giovara]

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.
Thanks
-- 
Vittorio