[FFmpeg-devel,1/2] lavu: Add JEDEC P22 color primaries

Submitted by Vittorio Giovara on Nov. 30, 2016, 6:16 p.m.

Details

Message ID 20161130181625.34192-1-vittorio.giovara@gmail.com
State New
Headers show

Commit Message

Vittorio Giovara Nov. 30, 2016, 6:16 p.m.
Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
---
Please CC.
Vittorio

 libavutil/pixdesc.c | 1 +
 libavutil/pixfmt.h  | 1 +
 2 files changed, 2 insertions(+)

Comments

Nicolas George Nov. 30, 2016, 6:21 p.m.
Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> ---

> Please CC.

You can achieve the same result more reliably by setting the reply-to
header.

> Vittorio
> 
>  libavutil/pixdesc.c | 1 +
>  libavutil/pixfmt.h  | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index 3b9c45d..04eab0b 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -2183,6 +2183,7 @@ static const char *color_primaries_names[AVCOL_PRI_NB] = {
>      [AVCOL_PRI_SMPTE428] = "smpte428",
>      [AVCOL_PRI_SMPTE431] = "smpte431",
>      [AVCOL_PRI_SMPTE432] = "smpte432",
> +    [AVCOL_PRI_JEDEC_P22] = "jedec-p22",
>  };
>  
>  static const char *color_transfer_names[] = {
> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index dfb1b11..d6c5a57 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -413,6 +413,7 @@ enum AVColorPrimaries {
>      AVCOL_PRI_SMPTEST428_1 = AVCOL_PRI_SMPTE428,
>      AVCOL_PRI_SMPTE431    = 11, ///< SMPTE ST 431-2 (2011)
>      AVCOL_PRI_SMPTE432    = 12, ///< SMPTE ST 432-1 D65 (2010)
> +    AVCOL_PRI_JEDEC_P22   = 22, ///< JEDEC P22 phosphors
>      AVCOL_PRI_NB                ///< Not part of ABI
>  };

Are you leaving a gap on purpose? There was no gap until now.

(Also, why are the values specified? And despite being "not part of the
ABI", AVCOL_PRI_NB is used outside lavu.)

Regards,
Vittorio Giovara Nov. 30, 2016, 7:28 p.m.
On Wed, Nov 30, 2016 at 1:21 PM, Nicolas George <george@nsup.org> wrote:
> Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>> ---
>
>> Please CC.
>
> You can achieve the same result more reliably by setting the reply-to
> header.

Oh, nice trick, I'll try it out next time.

>> Vittorio
>>
>>  libavutil/pixdesc.c | 1 +
>>  libavutil/pixfmt.h  | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
>> index 3b9c45d..04eab0b 100644
>> --- a/libavutil/pixdesc.c
>> +++ b/libavutil/pixdesc.c
>> @@ -2183,6 +2183,7 @@ static const char *color_primaries_names[AVCOL_PRI_NB] = {
>>      [AVCOL_PRI_SMPTE428] = "smpte428",
>>      [AVCOL_PRI_SMPTE431] = "smpte431",
>>      [AVCOL_PRI_SMPTE432] = "smpte432",
>> +    [AVCOL_PRI_JEDEC_P22] = "jedec-p22",
>>  };
>>
>>  static const char *color_transfer_names[] = {
>> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
>> index dfb1b11..d6c5a57 100644
>> --- a/libavutil/pixfmt.h
>> +++ b/libavutil/pixfmt.h
>> @@ -413,6 +413,7 @@ enum AVColorPrimaries {
>>      AVCOL_PRI_SMPTEST428_1 = AVCOL_PRI_SMPTE428,
>>      AVCOL_PRI_SMPTE431    = 11, ///< SMPTE ST 431-2 (2011)
>>      AVCOL_PRI_SMPTE432    = 12, ///< SMPTE ST 432-1 D65 (2010)
>> +    AVCOL_PRI_JEDEC_P22   = 22, ///< JEDEC P22 phosphors
>>      AVCOL_PRI_NB                ///< Not part of ABI
>>  };
>
> Are you leaving a gap on purpose? There was no gap until now.

This is the value specified in the 23001-8_2013 document.

> (Also, why are the values specified? And despite being "not part of the
> ABI", AVCOL_PRI_NB is used outside lavu.)

Hm those should probably be addressed too, the decoder and filters
ones are simple, while static options might be tricky.
Nicolas George Nov. 30, 2016, 7:51 p.m.
Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
> This is the value specified in the 23001-8_2013 document.

This looks paywalled. Please give links to public versions of specs in
that kind of case.

But I am pretty sure this document does not specify the values of enums
in FFmpeg's API. If the rest of the code requires that the values of the
enum match values in files or protocol, then I think a comment must say
so clearly. And if not, then the gap is not needed.

> Hm those should probably be addressed too, the decoder and filters
> ones are simple, while static options might be tricky.

My opinion on the matter is that any such kind of enumeration should
include not only the enum, but also a value->string function (this was
done), a string->value function and an AV_OPT type. That requires a
little work, though.

Regards,
Vittorio Giovara Nov. 30, 2016, 8 p.m.
On Wed, Nov 30, 2016 at 2:51 PM, Nicolas George <george@nsup.org> wrote:
> Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
>> This is the value specified in the 23001-8_2013 document.
>
> This looks paywalled. Please give links to public versions of specs in
> that kind of case.

I am sorry, I don't have any public link I can share.

> But I am pretty sure this document does not specify the values of enums
> in FFmpeg's API. If the rest of the code requires that the values of the
> enum match values in files or protocol, then I think a comment must say
> so clearly. And if not, then the gap is not needed.

It does.
James Almer Nov. 30, 2016, 8:02 p.m.
On 11/30/2016 4:51 PM, Nicolas George wrote:
> Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
>> This is the value specified in the 23001-8_2013 document.
> 
> This looks paywalled. Please give links to public versions of specs in
> that kind of case.

http://standards.iso.org/ittf/PubliclyAvailableStandards/c062088_ISO_IEC_23001-8_2013.zip
Nicolas George Nov. 30, 2016, 8:16 p.m.
Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
> I am sorry, I don't have any public link I can share.

Ok. Thanks for James for his gratis-but-sell-your-soul link.

> > But I am pretty sure this document does not specify the values of enums
> > in FFmpeg's API. If the rest of the code requires that the values of the
> > enum match values in files or protocol, then I think a comment must say
> > so clearly. And if not, then the gap is not needed.
> It does.

Who does what? Your answer is ambiguous.

And can you explain where it does? I looked at the code and did not see
it.

What happens if we need to support a system of primaries that does not
come from the same organization?

Regards,
James Almer Nov. 30, 2016, 8:58 p.m.
On 11/30/2016 5:16 PM, Nicolas George wrote:
> Le decadi 10 frimaire, an CCXXV, Vittorio Giovara a écrit :
>> I am sorry, I don't have any public link I can share.
> 
> Ok. Thanks for James for his gratis-but-sell-your-soul link.
> 
>>> But I am pretty sure this document does not specify the values of enums
>>> in FFmpeg's API. If the rest of the code requires that the values of the
>>> enum match values in files or protocol, then I think a comment must say
>>> so clearly. And if not, then the gap is not needed.
>> It does.
> 
> Who does what? Your answer is ambiguous.

It is needed like this. Decoders and encoders (h264, hevc, vc1, mpeg2, prores,
libx264, libx265, nvenc, etc), demuxers and muxers (mov, matroka), all expect
the enum values to follow those described by this spec.

> 
> And can you explain where it does? I looked at the code and did not see
> it.
> 
> What happens if we need to support a system of primaries that does not
> come from the same organization?

Custom mapping functions would be written for such unusual cases, i guess.

> 
> Regards,
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Andreas Cadhalpun Nov. 30, 2016, 9:51 p.m.
On 30.11.2016 19:16, Vittorio Giovara wrote:
> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> ---
> Please CC.
> Vittorio
> 
>  libavutil/pixdesc.c | 1 +
>  libavutil/pixfmt.h  | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> index 3b9c45d..04eab0b 100644
> --- a/libavutil/pixdesc.c
> +++ b/libavutil/pixdesc.c
> @@ -2183,6 +2183,7 @@ static const char *color_primaries_names[AVCOL_PRI_NB] = {
>      [AVCOL_PRI_SMPTE428] = "smpte428",
>      [AVCOL_PRI_SMPTE431] = "smpte431",
>      [AVCOL_PRI_SMPTE432] = "smpte432",
> +    [AVCOL_PRI_JEDEC_P22] = "jedec-p22",
>  };
>  
>  static const char *color_transfer_names[] = {
> diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> index dfb1b11..d6c5a57 100644
> --- a/libavutil/pixfmt.h
> +++ b/libavutil/pixfmt.h
> @@ -413,6 +413,7 @@ enum AVColorPrimaries {
>      AVCOL_PRI_SMPTEST428_1 = AVCOL_PRI_SMPTE428,
>      AVCOL_PRI_SMPTE431    = 11, ///< SMPTE ST 431-2 (2011)
>      AVCOL_PRI_SMPTE432    = 12, ///< SMPTE ST 432-1 D65 (2010)
> +    AVCOL_PRI_JEDEC_P22   = 22, ///< JEDEC P22 phosphors
>      AVCOL_PRI_NB                ///< Not part of ABI
>  };

You can't just add a gap like that.
The current code assumes that the numbers are consecutive, like e.g. the
naming of AVCOL_PRI_NB suggests.
For example AVCOL_PRI_NB is used in libavcodec/options_table.h to validate
the color_primaries option, but after this patch would accept values like 15.
Also I think av_color_primaries_name would just return uninitialized memory
for such a value.

If you really need to add the gap here, you'd have to carefully look at
every occurrence of AVCOL_PRI_NB in the code base and make sure the
code still works with a gap here.

Best regards,
Andreas
Ronald S. Bultje Nov. 30, 2016, 9:55 p.m.
Hi,

On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 30.11.2016 19:16, Vittorio Giovara wrote:
> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> > ---
> > Please CC.
> > Vittorio
> >
> >  libavutil/pixdesc.c | 1 +
> >  libavutil/pixfmt.h  | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
> > index 3b9c45d..04eab0b 100644
> > --- a/libavutil/pixdesc.c
> > +++ b/libavutil/pixdesc.c
> > @@ -2183,6 +2183,7 @@ static const char *color_primaries_names[AVCOL_PRI_NB]
> = {
> >      [AVCOL_PRI_SMPTE428] = "smpte428",
> >      [AVCOL_PRI_SMPTE431] = "smpte431",
> >      [AVCOL_PRI_SMPTE432] = "smpte432",
> > +    [AVCOL_PRI_JEDEC_P22] = "jedec-p22",
> >  };
> >
> >  static const char *color_transfer_names[] = {
> > diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
> > index dfb1b11..d6c5a57 100644
> > --- a/libavutil/pixfmt.h
> > +++ b/libavutil/pixfmt.h
> > @@ -413,6 +413,7 @@ enum AVColorPrimaries {
> >      AVCOL_PRI_SMPTEST428_1 = AVCOL_PRI_SMPTE428,
> >      AVCOL_PRI_SMPTE431    = 11, ///< SMPTE ST 431-2 (2011)
> >      AVCOL_PRI_SMPTE432    = 12, ///< SMPTE ST 432-1 D65 (2010)
> > +    AVCOL_PRI_JEDEC_P22   = 22, ///< JEDEC P22 phosphors
> >      AVCOL_PRI_NB                ///< Not part of ABI
> >  };
>
> You can't just add a gap like that.
> The current code assumes that the numbers are consecutive, like e.g. the
> naming of AVCOL_PRI_NB suggests.


No, we've had gaps in these before.

Whether all code works correctly with gaps is a separate thing, but gaps
are intended and have existed.

Ronald
Andreas Cadhalpun Nov. 30, 2016, 10:26 p.m.
On 30.11.2016 22:55, Ronald S. Bultje wrote:
> On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
> andreas.cadhalpun@googlemail.com> wrote:
> 
>> On 30.11.2016 19:16, Vittorio Giovara wrote:
>> You can't just add a gap like that.
>> The current code assumes that the numbers are consecutive, like e.g. the
>> naming of AVCOL_PRI_NB suggests.
> 
> 
> No, we've had gaps in these before.

In AVColorPrimaries?

> Whether all code works correctly with gaps is a separate thing, but gaps
> are intended and have existed.

Without this patch there is no need for code to take gaps in AVColorPrimaries
into account, so it's no bug if it doesn't.
As such this patch would (indirectly) introduce those bugs, which is why
the code should be changed to take gaps into account before this patch
is applied.

Best regards,
Andreas
James Almer Nov. 30, 2016, 10:38 p.m.
On 11/30/2016 7:26 PM, Andreas Cadhalpun wrote:
> On 30.11.2016 22:55, Ronald S. Bultje wrote:
>> On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
>> andreas.cadhalpun@googlemail.com> wrote:
>>
>>> On 30.11.2016 19:16, Vittorio Giovara wrote:
>>> You can't just add a gap like that.
>>> The current code assumes that the numbers are consecutive, like e.g. the
>>> naming of AVCOL_PRI_NB suggests.
>>
>>
>> No, we've had gaps in these before.
> 
> In AVColorPrimaries?
> 
>> Whether all code works correctly with gaps is a separate thing, but gaps
>> are intended and have existed.
> 
> Without this patch there is no need for code to take gaps in AVColorPrimaries
> into account, so it's no bug if it doesn't.
> As such this patch would (indirectly) introduce those bugs, which is why
> the code should be changed to take gaps into account before this patch
> is applied.

Agree.

Vittorio sent a couple patches that do this to libav, for that matter. I guess
we can expect them sent here as well.
Ronald S. Bultje Nov. 30, 2016, 10:48 p.m.
Hi,

On Wed, Nov 30, 2016 at 5:26 PM, Andreas Cadhalpun <
andreas.cadhalpun@googlemail.com> wrote:

> On 30.11.2016 22:55, Ronald S. Bultje wrote:
> > On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
> > andreas.cadhalpun@googlemail.com> wrote:
> >
> >> On 30.11.2016 19:16, Vittorio Giovara wrote:
> >> You can't just add a gap like that.
> >> The current code assumes that the numbers are consecutive, like e.g. the
> >> naming of AVCOL_PRI_NB suggests.
> >
> >
> > No, we've had gaps in these before.
>
> In AVColorPrimaries?


I seemed to remember, looked it up and appeared to be wrong, so scrap that
:)

Ronald
Vittorio Giovara Dec. 1, 2016, 5:25 p.m.
On Wed, Nov 30, 2016 at 5:48 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Wed, Nov 30, 2016 at 5:26 PM, Andreas Cadhalpun
> <andreas.cadhalpun@googlemail.com> wrote:
>>
>> On 30.11.2016 22:55, Ronald S. Bultje wrote:
>> > On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
>> > andreas.cadhalpun@googlemail.com> wrote:
>> >
>> >> On 30.11.2016 19:16, Vittorio Giovara wrote:
>> >> You can't just add a gap like that.
>> >> The current code assumes that the numbers are consecutive, like e.g.
>> >> the
>> >> naming of AVCOL_PRI_NB suggests.
>> >
>> >
>> > No, we've had gaps in these before.
>>
>> In AVColorPrimaries?
>
>
> I seemed to remember, looked it up and appeared to be wrong, so scrap that
> :)

Ok, so I can take care of the "holes" this patch would bring for lavc,
would anybody help me with the remaining occurrences (namely in lavf
and lavfi)?
Rostislav Pehlivanov Dec. 1, 2016, 7 p.m.
On 1 December 2016 at 17:25, Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Wed, Nov 30, 2016 at 5:48 PM, Ronald S. Bultje <rsbultje@gmail.com>
> wrote:
> > Hi,
> >
> > On Wed, Nov 30, 2016 at 5:26 PM, Andreas Cadhalpun
> > <andreas.cadhalpun@googlemail.com> wrote:
> >>
> >> On 30.11.2016 22:55, Ronald S. Bultje wrote:
> >> > On Wed, Nov 30, 2016 at 4:51 PM, Andreas Cadhalpun <
> >> > andreas.cadhalpun@googlemail.com> wrote:
> >> >
> >> >> On 30.11.2016 19:16, Vittorio Giovara wrote:
> >> >> You can't just add a gap like that.
> >> >> The current code assumes that the numbers are consecutive, like e.g.
> >> >> the
> >> >> naming of AVCOL_PRI_NB suggests.
> >> >
> >> >
> >> > No, we've had gaps in these before.
> >>
> >> In AVColorPrimaries?
> >
> >
> > I seemed to remember, looked it up and appeared to be wrong, so scrap
> that
> > :)
>
> Ok, so I can take care of the "holes" this patch would bring for lavc,
> would anybody help me with the remaining occurrences (namely in lavf
> and lavfi)?
> --


Why do you insist on having a hole in the first place?
Nicolas George Dec. 1, 2016, 7:22 p.m.
Le primidi 11 frimaire, an CCXXV, Rostislav Pehlivanov a écrit :
> Why do you insist on having a hole in the first place?

Apparently, the values of the enum are used encode and decode
standardized values in standardized formats like MPEG.

I think this is a terrible design decision, and I would like to suggest
to change it before it is too late.

It will lead to all kinds of trouble if we want to support primaries
from other standards with conflicting values, or if the standards
introduce huge gaps in their allocated values.

The correct way of dealing with that kind of situation is the same as
what we have always done for 4CCs: have tables (or other kinds of
conversion functions) matching the value of the enum in our API with the
value for a corresponding standard. The name of the table must include
the name of the standard.

Regards,
Vittorio Giovara Dec. 1, 2016, 7:58 p.m.
On Thu, Dec 1, 2016 at 2:22 PM, Nicolas George <george@nsup.org> wrote:
> Le primidi 11 frimaire, an CCXXV, Rostislav Pehlivanov a écrit :
>> Why do you insist on having a hole in the first place?
>
> Apparently, the values of the enum are used encode and decode
> standardized values in standardized formats like MPEG.
>
> I think this is a terrible design decision, and I would like to suggest
> to change it before it is too late.
>
> It will lead to all kinds of trouble if we want to support primaries
> from other standards with conflicting values, or if the standards
> introduce huge gaps in their allocated values.

Actually we already do, theora, vp8, vp9 and a few others have
completely different values for most color properties, and we just
check before initializing the context or frame fields. Since iso/itu
codecs are the ones that more consistently use all the properties
everywhere it makes sense that the enum values and field values match
and imho it's simpler to use.
Nicolas George Dec. 1, 2016, 8:01 p.m.
Le primidi 11 frimaire, an CCXXV, Vittorio Giovara a écrit :
> Actually we already do, theora, vp8, vp9 and a few others have
> completely different values for most color properties, and we just
> check before initializing the context or frame fields. Since iso/itu
> codecs are the ones that more consistently use all the properties
> everywhere it makes sense that the enum values and field values match
> and imho it's simpler to use.

It made sense until now, but now they have added a gap, and it becomes
obvious it was not a very good idea to begin with.

Fortunately, since the architecture is already there for the other
codecs, fixing it should not be much work. Probably about the same
amount as fixing the issues with the gap.

Regards,
Vittorio Giovara Dec. 1, 2016, 8:25 p.m.
On Thu, Dec 1, 2016 at 3:01 PM, Nicolas George <george@nsup.org> wrote:
> Le primidi 11 frimaire, an CCXXV, Vittorio Giovara a écrit :
>> Actually we already do, theora, vp8, vp9 and a few others have
>> completely different values for most color properties, and we just
>> check before initializing the context or frame fields. Since iso/itu
>> codecs are the ones that more consistently use all the properties
>> everywhere it makes sense that the enum values and field values match
>> and imho it's simpler to use.
>
> It made sense until now, but now they have added a gap, and it becomes
> obvious it was not a very good idea to begin with.
>
> Fortunately, since the architecture is already there for the other
> codecs, fixing it should not be much work. Probably about the same
> amount as fixing the issues with the gap.

I'm not sure about that, right now a gap would just require adding
bound checks in exactly 5 different places, whereas restructuring this
well-established system will need careful modification for each of the
codecs involved, which are quite a few. The real problem to me seems
the fact that _NB elements are used outside the library, despite being
documented against, so I think it would be more productive to fix
those occurrences (especially since this is a user-facing API).

Patch hide | download patch | download mbox

diff --git a/libavutil/pixdesc.c b/libavutil/pixdesc.c
index 3b9c45d..04eab0b 100644
--- a/libavutil/pixdesc.c
+++ b/libavutil/pixdesc.c
@@ -2183,6 +2183,7 @@  static const char *color_primaries_names[AVCOL_PRI_NB] = {
     [AVCOL_PRI_SMPTE428] = "smpte428",
     [AVCOL_PRI_SMPTE431] = "smpte431",
     [AVCOL_PRI_SMPTE432] = "smpte432",
+    [AVCOL_PRI_JEDEC_P22] = "jedec-p22",
 };
 
 static const char *color_transfer_names[] = {
diff --git a/libavutil/pixfmt.h b/libavutil/pixfmt.h
index dfb1b11..d6c5a57 100644
--- a/libavutil/pixfmt.h
+++ b/libavutil/pixfmt.h
@@ -413,6 +413,7 @@  enum AVColorPrimaries {
     AVCOL_PRI_SMPTEST428_1 = AVCOL_PRI_SMPTE428,
     AVCOL_PRI_SMPTE431    = 11, ///< SMPTE ST 431-2 (2011)
     AVCOL_PRI_SMPTE432    = 12, ///< SMPTE ST 432-1 D65 (2010)
+    AVCOL_PRI_JEDEC_P22   = 22, ///< JEDEC P22 phosphors
     AVCOL_PRI_NB                ///< Not part of ABI
 };