| Message ID | CABLWnS_qFDjPqUePEsO05HzgNVbTFPL3Q_6ERQB6xZ6+cECtYg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 20 September 2017 at 12:55, Vittorio Giovara <vittorio.giovara@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.
On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 20 September 2017 at 12:55, Vittorio Giovara <vittorio.giovara@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
On 20 September 2017 at 16:05, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov > <atomnuker@gmail.com> wrote: > > On 20 September 2017 at 12:55, Vittorio Giovara < > vittorio.giovara@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? The standard sucks and I see no reason why we need to stick to it.
On 9/20/2017 12:05 PM, Hendrik Leppkes wrote: > On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov > <atomnuker@gmail.com> wrote: >> On 20 September 2017 at 12:55, Vittorio Giovara <vittorio.giovara@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 +1. I very much rather not touch AVMasteringDisplayMetadata at all. The struct size is supposedly not part of the ABI yet the alloc function doesn't report the size in any way whatsoever, resulting in the need to use sizeof() outside of lavu. Adding new values will inevitably lead to backwards compatibility issues. Besides, the frame/packet side data API is extremely easy to expand. You just add a new type, and when needed a struct and accompanying utility functions.
On 9/20/2017 12:47 PM, Rostislav Pehlivanov wrote: > On 20 September 2017 at 16:05, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov >> <atomnuker@gmail.com> wrote: >>> On 20 September 2017 at 12:55, Vittorio Giovara < >> vittorio.giovara@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. > The standard sucks and I see no reason why we need to stick to it. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, Sep 20, 2017 at 5:47 PM, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 20 September 2017 at 16:05, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov >> <atomnuker@gmail.com> wrote: >> > On 20 September 2017 at 12:55, Vittorio Giovara < >> vittorio.giovara@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? > 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
On Wed, 20 Sep 2017 12:58:14 -0300 James Almer <jamrial@gmail.com> wrote: > On 9/20/2017 12:47 PM, Rostislav Pehlivanov wrote: > > On 20 September 2017 at 16:05, Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > >> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov > >> <atomnuker@gmail.com> wrote: > >>> On 20 September 2017 at 12:55, Vittorio Giovara < > >> vittorio.giovara@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).
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;