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

[Mark Thompson]

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.

- Mark