On Wed, 20 Sep 2017 12:58:14 -0300
James Almer <jamr...@gmail.com> wrote:
> On 9/20/2017 12:47 PM, Rostislav Pehlivanov wrote:
> > On 20 September 2017 at 16:05, Hendrik Leppkes <h.lepp...@gmail.com> wrote:
> >
> >> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
> >> <atomnu...@gmail.com> wrote:
> >>> On 20 September 2017 at 12:55, Vittorio Giovara <
> >> vittorio.giov...@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@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@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).
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
