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

[Hendrik Leppkes]

On Wed, Sep 20, 2017 at 5:47 PM, Rostislav Pehlivanov
<atomnuker at gmail.com> 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?
> The standard sucks and I see no reason why we need to stick to it.

I see no reason for you to get so angry. Just make a new struct for
your custom value already.
The comment above the struct clearly states that its derived from this
standard, and thats that.

- Hendrik