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

2017-09-20 Thread wm4
On Wed, 20 Sep 2017 15:28:26 -0300
James Almer  wrote:

> On 9/20/2017 3:18 PM, wm4 wrote:
> > On Wed, 20 Sep 2017 04:00:27 +0100
> > Rostislav Pehlivanov  wrote:
> >   
> >> PNG exposes it and its required in order to correctly display some images,
> >> particularly images crafted to contain 2 different images which appear
> >> differently depending on whether the gamma has been taken into account.
> >>
> >> Signed-off-by: Rostislav Pehlivanov 
> >> ---
> >>  libavutil/mastering_display_metadata.h | 10 ++
> >>  libavutil/version.h|  2 +-
> >>  2 files changed, 11 insertions(+), 1 deletion(-)
> >>
> >> 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;  
> > 
> > Why have the last field, instead of making gamma={0,0} mean unset?  
> 
> Didn't check the spec about it, but 0 could very well be a valid value.
> 
> In any case this will go in a separate struct, so there will be no need
> for a has_gama field. The mere fact the side data exists will mean the
> gamma value was filled.

A valid value of 0 would be {0,1}. {0,0} is akin to NAN.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread James Almer
On 9/20/2017 3:18 PM, wm4 wrote:
> On Wed, 20 Sep 2017 04:00:27 +0100
> Rostislav Pehlivanov  wrote:
> 
>> PNG exposes it and its required in order to correctly display some images,
>> particularly images crafted to contain 2 different images which appear
>> differently depending on whether the gamma has been taken into account.
>>
>> Signed-off-by: Rostislav Pehlivanov 
>> ---
>>  libavutil/mastering_display_metadata.h | 10 ++
>>  libavutil/version.h|  2 +-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> 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;
> 
> Why have the last field, instead of making gamma={0,0} mean unset?

Didn't check the spec about it, but 0 could very well be a valid value.

In any case this will go in a separate struct, so there will be no need
for a has_gama field. The mere fact the side data exists will mean the
gamma value was filled.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread wm4
On Wed, 20 Sep 2017 12:58:14 -0300
James Almer  wrote:

> On 9/20/2017 12:47 PM, Rostislav Pehlivanov wrote:
> > On 20 September 2017 at 16:05, Hendrik Leppkes  wrote:
> >   
> >> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
> >>  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


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

2017-09-20 Thread wm4
On Wed, 20 Sep 2017 04:00:27 +0100
Rostislav Pehlivanov  wrote:

> PNG exposes it and its required in order to correctly display some images,
> particularly images crafted to contain 2 different images which appear
> differently depending on whether the gamma has been taken into account.
> 
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  libavutil/mastering_display_metadata.h | 10 ++
>  libavutil/version.h|  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> 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;

Why have the last field, instead of making gamma={0,0} mean unset?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread Hendrik Leppkes
On Wed, Sep 20, 2017 at 5:47 PM, Rostislav Pehlivanov
 wrote:
> On 20 September 2017 at 16:05, Hendrik Leppkes  wrote:
>
>> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
>>  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?
> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread James Almer
On 9/20/2017 12:58 PM, Vittorio Giovara wrote:
>> * 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.
> 
> 
> "mastering display" is a specification defined in several standards,
> including h264 and h265. It groups two kind of properties, the mastering
> display primaries (with its whitepoint) and the screen min and max
> luminance. AVMasteringDisplayMetadata is modelled after this specification
> and designed to expose the same kind of information; setters and getter
> will expect to use all the information contained in it.
> 
> Adding a field that is missing from the original specification to that
> structure is, in my opinion, semantically wrong, not only because
> AVMasteringDisplayMetadata won't be strictly mapped to a the "mastering
> display" any more, but also because the setters and getters of the
> "mastering display" users will not need the new field at all.
> 
> There is a precedent to this: AVColorVolume. Its field are, in practice,

Did you mean AVContentLightMetadata? I can't find any reference to
AVColorVolume in the tree.

> tied to mastering display, but they are stored in a separate structure, and
> not squashed in the main AVMasteringDisplayMetadata structure. I think we
> should continue this trend so that we can easily map structures with known
> specifications and not overload them with unnecessary fields.
> 

PS: Please fix your mail client. You're creating new threads with each
reply as they don't have an in-reply-to header field.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread Vittorio Giovara
>* 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.


"mastering display" is a specification defined in several standards,
including h264 and h265. It groups two kind of properties, the mastering
display primaries (with its whitepoint) and the screen min and max
luminance. AVMasteringDisplayMetadata is modelled after this specification
and designed to expose the same kind of information; setters and getter
will expect to use all the information contained in it.

Adding a field that is missing from the original specification to that
structure is, in my opinion, semantically wrong, not only because
AVMasteringDisplayMetadata won't be strictly mapped to a the "mastering
display" any more, but also because the setters and getters of the
"mastering display" users will not need the new field at all.

There is a precedent to this: AVColorVolume. Its field are, in practice,
tied to mastering display, but they are stored in a separate structure, and
not squashed in the main AVMasteringDisplayMetadata structure. I think we
should continue this trend so that we can easily map structures with known
specifications and not overload them with unnecessary fields.
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread James Almer
On 9/20/2017 12:05 PM, Hendrik Leppkes wrote:
> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
>  wrote:
>> On 20 September 2017 at 12:55, Vittorio Giovara 
>> 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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread James Almer
On 9/20/2017 12:47 PM, Rostislav Pehlivanov wrote:
> On 20 September 2017 at 16:05, Hendrik Leppkes  wrote:
> 
>> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
>>  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.

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

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread Rostislav Pehlivanov
On 20 September 2017 at 16:05, Hendrik Leppkes  wrote:

> On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
>  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?
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


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

2017-09-20 Thread Hendrik Leppkes
On Wed, Sep 20, 2017 at 4:34 PM, Rostislav Pehlivanov
 wrote:
> On 20 September 2017 at 12:55, Vittorio Giovara 
> 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


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

2017-09-20 Thread Rostislav Pehlivanov
On 20 September 2017 at 12:55, Vittorio Giovara 
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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-09-20 Thread Vittorio Giovara
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


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

2017-09-19 Thread Rostislav Pehlivanov
PNG exposes it and its required in order to correctly display some images,
particularly images crafted to contain 2 different images which appear
differently depending on whether the gamma has been taken into account.

Signed-off-by: Rostislav Pehlivanov 
---
 libavutil/mastering_display_metadata.h | 10 ++
 libavutil/version.h|  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

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;
 
 /**
diff --git a/libavutil/version.h b/libavutil/version.h
index d99eff5d15..8ac41f49f5 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -80,7 +80,7 @@
 
 
 #define LIBAVUTIL_VERSION_MAJOR  55
-#define LIBAVUTIL_VERSION_MINOR  75
+#define LIBAVUTIL_VERSION_MINOR  76
 #define LIBAVUTIL_VERSION_MICRO 100
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
-- 
2.14.1.821.g8fa685d3b7

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel