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

[wm4]

On Wed, 20 Sep 2017 12:58:14 -0300
James Almer <jamrial at gmail.com> wrote:

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

Side data is broken in general. Nobody should have to care about the
size of the side data struct.

As an API user, I expect that the side data always has the struct's
side, and if there are any security issues, I will blame FFmpeg (or
anyone who patches out version checks).