[FFmpeg-devel] [PATCH 1/2] lavu: add a gamma field to AVMasteringDisplayMetadata

[James Almer]

On 9/20/2017 12:47 PM, Rostislav Pehlivanov wrote:
> On 20 September 2017 at 16:05, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> 
>> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
>> <atomnuker at gmail.com> wrote:
>>> On 20 September 2017 at 12:55, Vittorio Giovara <
>> vittorio.giovara at gmail.com>
>>> wrote:
>>>
>>>> diff --git a/libavutil/mastering_display_metadata.h
>>>> b/libavutil/mastering_display_metadata.h
>>>> index 847b0b62c6..3de58bf468 100644
>>>> --- a/libavutil/mastering_display_metadata.h
>>>> +++ b/libavutil/mastering_display_metadata.h
>>>> @@ -66,6 +66,16 @@ typedef struct AVMasteringDisplayMetadata {
>>>>       */
>>>>      int has_luminance;
>>>>
>>>> +    /**
>>>> +     * The power-law response exponent needed to compensate for
>>>> nonlinearity.
>>>> +     */
>>>> +    AVRational gamma;
>>>> +
>>>> +    /**
>>>> +     * Flag indicating whether the gamma has been set.
>>>> +     */
>>>> +    int has_gamma;
>>>> +
>>>>  } AVMasteringDisplayMetadata;
>>>>
>>>>
>>>> In my opinion we should not add new fields to structures that map 1:1 to
>>>> something defined elsewhere (with the exception of booleans).
>>>> I think this should be a separate side data and type, ie
>> AVGammaResponse,
>>>> that can of course reside in the same header.
>>>> --
>>>> Vittorio
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel at ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>
>>> Why? I don't see anything special about the struct. And this value fits
>> in
>>> exactly to what the struct is all about.
>>
>> I basically agree with Vittorio, the struct basically represents the
>> HDR metadata as specified for SMPTE ST 2086 (and as signaled as such
>> in HEVC and various container formats). Jumbling other values in which
>> are not part of that standard doesn't seem ideal.
>>
>> - Hendrik
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> 
> So why not name the whole struct with some hevc prefix and add a field
> saying: "New values are forbidden because SMPTE said so!", rather than a
> warning saying not to use the size of the struct as a public API because
> new values might be added?

Because the standard could always get new values, i presume.

In any case, as i said, the AVMasteringDisplayMetadata is currently
kinda fucked because of the lack of a proper allocation function.
A new one needs to be added in any case, and no new fields whatsoever
should be added to the struct until a major bump takes place.

> The standard sucks and I see no reason why we need to stick to it.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>