| Message ID | 20161130181625.34192-1-vittorio.giovara@gmail.com |
|---|---|
| State | New |
| Headers | show |
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,
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.
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,
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.
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
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,
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 >
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
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
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
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.
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
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)?
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?
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,
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.
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,
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).
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 };
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(+)