[FFmpeg-devel] [PATCHv2] all: fix enum definition for large values
Ronald S. Bultje
rsbultje at gmail.com
Thu Oct 29 12:57:29 CET 2015
Hi,
On Thu, Oct 29, 2015 at 7:55 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
wrote:
> On Thu, Oct 29, 2015 at 12:29 AM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> > On Wed, Oct 28, 2015 at 07:02:42PM -0400, Ganesh Ajjanagadde wrote:
> >> On Wed, Oct 28, 2015 at 3:05 PM, Ronald S. Bultje <rsbultje at gmail.com>
> wrote:
> >> > Hi,
> >> >
> >> > On Wed, Oct 28, 2015 at 2:46 PM, Ganesh Ajjanagadde <
> gajjanagadde at gmail.com>
> >> > wrote:
> >> >>
> >> >> On Wed, Oct 28, 2015 at 2:39 PM, Ronald S. Bultje <
> rsbultje at gmail.com>
> >> >> wrote:
> >> >> > Hi,
> >> >> >
> >> >> > On Wed, Oct 28, 2015 at 2:31 PM, Ganesh Ajjanagadde
> >> >> > <gajjanagadde at gmail.com>
> >> >> > wrote:
> >> >> >>
> >> >> >> On Wed, Oct 28, 2015 at 11:38 AM, Ronald S. Bultje <
> rsbultje at gmail.com>
> >> >> >> wrote:
> >> >> >> > Hi,
> >> >> >> >
> >> >> >> > On Wed, Oct 28, 2015 at 11:00 AM, Ganesh Ajjanagadde
> >> >> >> > <gajjanagadde at gmail.com> wrote:
> >> >> >> >>
> >> >> >> >> On Wed, Oct 28, 2015 at 10:53 AM, Ronald S. Bultje
> >> >> >> >> <rsbultje at gmail.com>
> >> >> >> >> wrote:
> >> >> >> >> > Hi,
> >> >> >> >> >
> >> >> >> >> > On Wed, Oct 28, 2015 at 10:48 AM, Ganesh Ajjanagadde
> >> >> >> >> > <gajjanagadde at gmail.com> wrote:
> >> >> >> >> >>
> >> >> >> >> >> On Wed, Oct 28, 2015 at 10:34 AM, Ronald S. Bultje
> >> >> >> >> >> <rsbultje at gmail.com>
> >> >> >> >> >> wrote:
> >> >> >> >> >> > Hi,
> >> >> >> >> >> >
> >> >> >> >> >> > On Wed, Oct 28, 2015 at 8:20 AM, Ganesh Ajjanagadde
> >> >> >> >> >> > <gajjanagadde at gmail.com>
> >> >> >> >> >> > wrote:
> >> >> >> >> >> >>
> >> >> >> >> >> >> On Wed, Oct 28, 2015 at 7:00 AM, Ronald S. Bultje
> >> >> >> >> >> >> <rsbultje at gmail.com>
> >> >> >> >> >> >> wrote:
> >> >> >> >> >> >> > Hi,
> >> >> >> >> >> >> >
> >> >> >> >> >> >> > On Tue, Oct 27, 2015 at 10:58 PM, Ganesh Ajjanagadde
> >> >> >> >> >> >> > <gajjanagadde at gmail.com> wrote:
> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> On Tue, Oct 27, 2015 at 10:41 PM, Ronald S. Bultje
> >> >> >> >> >> >> >> <rsbultje at gmail.com>
> >> >> >> >> >> >> >> wrote:
> >> >> >> >> >> >> >> > Hi,
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> > On Tue, Oct 27, 2015 at 8:53 PM, Ganesh Ajjanagadde
> >> >> >> >> >> >> >> > <gajjanagadde at gmail.com>
> >> >> >> >> >> >> >> > wrote:
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> ISO C restricts enumerator values to the range of
> int.
> >> >> >> >> >> >> >> >> Thus
> >> >> >> >> >> >> >> >> (for
> >> >> >> >> >> >> >> >> instance)
> >> >> >> >> >> >> >> >> 0x80000000
> >> >> >> >> >> >> >> >> unfortunately does not work, and throws a warning
> with
> >> >> >> >> >> >> >> >> -Wpedantic
> >> >> >> >> >> >> >> >> on
> >> >> >> >> >> >> >> >> clang 3.7.
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> This fixes it by using alternative expressions that
> >> >> >> >> >> >> >> >> result
> >> >> >> >> >> >> >> >> in
> >> >> >> >> >> >> >> >> identical
> >> >> >> >> >> >> >> >> values but do not have this issue.
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> Tested with FATE.
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> Signed-off-by: Ganesh Ajjanagadde
> >> >> >> >> >> >> >> >> <gajjanagadde at gmail.com>
> >> >> >> >> >> >> >> >> ---
> >> >> >> >> >> >> >> >> libavcodec/dca_syncwords.h | 26
> >> >> >> >> >> >> >> >> ++++++++++++--------------
> >> >> >> >> >> >> >> >> libavformat/cinedec.c | 2 +-
> >> >> >> >> >> >> >> >> libavformat/mov_chan.c | 2 +-
> >> >> >> >> >> >> >> >> 3 files changed, 14 insertions(+), 16 deletions(-)
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> diff --git a/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> b/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> index 3466b6b..6981cb8 100644
> >> >> >> >> >> >> >> >> --- a/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> +++ b/libavcodec/dca_syncwords.h
> >> >> >> >> >> >> >> >> @@ -19,19 +19,17 @@
> >> >> >> >> >> >> >> >> #ifndef AVCODEC_DCA_SYNCWORDS_H
> >> >> >> >> >> >> >> >> #define AVCODEC_DCA_SYNCWORDS_H
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> -enum DCASyncwords {
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_CORE_BE = 0x7FFE8001U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_CORE_LE = 0xFE7F0180U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_CORE_14B_BE = 0x1FFFE800U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_CORE_14B_LE = 0xFF1F00E8U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_XCH = 0x5A5A5A5AU,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_XXCH = 0x47004A03U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_X96 = 0x1D95F262U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_XBR = 0x655E315EU,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_LBR = 0x0A801921U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_XLL = 0x41A29547U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_SUBSTREAM = 0x64582025U,
> >> >> >> >> >> >> >> >> - DCA_SYNCWORD_SUBSTREAM_CORE = 0x02B09261U,
> >> >> >> >> >> >> >> >> -};
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_CORE_BE
> >> >> >> >> >> >> >> >> 0x7FFE8001U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_CORE_LE
> >> >> >> >> >> >> >> >> 0xFE7F0180U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_CORE_14B_BE
> >> >> >> >> >> >> >> >> 0x1FFFE800U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_CORE_14B_LE
> >> >> >> >> >> >> >> >> 0xFF1F00E8U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_XCH
> >> >> >> >> >> >> >> >> 0x5A5A5A5AU
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_XXCH
> >> >> >> >> >> >> >> >> 0x47004A03U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_X96
> >> >> >> >> >> >> >> >> 0x1D95F262U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_XBR
> >> >> >> >> >> >> >> >> 0x655E315EU
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_LBR
> >> >> >> >> >> >> >> >> 0x0A801921U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_XLL
> >> >> >> >> >> >> >> >> 0x41A29547U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_SUBSTREAM
> >> >> >> >> >> >> >> >> 0x64582025U
> >> >> >> >> >> >> >> >> +#define DCA_SYNCWORD_SUBSTREAM_CORE
> >> >> >> >> >> >> >> >> 0x02B09261U
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> > This one is fine.
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> --- a/libavformat/cinedec.c
> >> >> >> >> >> >> >> >> +++ b/libavformat/cinedec.c
> >> >> >> >> >> >> >> >> @@ -50,7 +50,7 @@ enum {
> >> >> >> >> >> >> >> >> CFA_BAYER = 3, /**< GB/RG */
> >> >> >> >> >> >> >> >> CFA_BAYERFLIP = 4, /**< RG/GB */
> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> - CFA_TLGRAY = 0x80000000,
> >> >> >> >> >> >> >> >> + CFA_TLGRAY = INT32_MIN,
> >> >> >> >> >> >> >> >> CFA_TRGRAY = 0x40000000,
> >> >> >> >> >> >> >> >> CFA_BLGRAY = 0x20000000,
> >> >> >> >> >> >> >> >> CFA_BRGRAY = 0x10000000
> >> >> >> >> >> >> >> >> diff --git a/libavformat/mov_chan.c
> >> >> >> >> >> >> >> >> b/libavformat/mov_chan.c
> >> >> >> >> >> >> >> >> index a2fa8d6..f6181e2 100644
> >> >> >> >> >> >> >> >> --- a/libavformat/mov_chan.c
> >> >> >> >> >> >> >> >> +++ b/libavformat/mov_chan.c
> >> >> >> >> >> >> >> >> @@ -45,7 +45,7 @@
> >> >> >> >> >> >> >> >> * do not specify a particular
> ordering of
> >> >> >> >> >> >> >> >> those
> >> >> >> >> >> >> >> >> channels."
> >> >> >> >> >> >> >> >> */
> >> >> >> >> >> >> >> >> enum MovChannelLayoutTag {
> >> >> >> >> >> >> >> >> - MOV_CH_LAYOUT_UNKNOWN =
> 0xFFFF0000,
> >> >> >> >> >> >> >> >> + MOV_CH_LAYOUT_UNKNOWN = -( 1 <<
> 16),
> >> >> >> >> >> >> >> >> MOV_CH_LAYOUT_USE_DESCRIPTIONS = ( 0 <<
> 16) |
> >> >> >> >> >> >> >> >> 0,
> >> >> >> >> >> >> >> >> MOV_CH_LAYOUT_USE_BITMAP = ( 1 <<
> 16) |
> >> >> >> >> >> >> >> >> 0,
> >> >> >> >> >> >> >> >> MOV_CH_LAYOUT_DISCRETEINORDER = (147 <<
> 16) |
> >> >> >> >> >> >> >> >> 0,
> >> >> >> >> >> >> >> >> --
> >> >> >> >> >> >> >> >> 2.6.2
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> > I personally don't really like these... I think both
> >> >> >> >> >> >> >> > obfuscate
> >> >> >> >> >> >> >> > the
> >> >> >> >> >> >> >> > meaning
> >> >> >> >> >> >> >> > of the flag values, particularly the first one.
> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> There is no real solution (recall apedec and the
> INT32_MIN
> >> >> >> >> >> >> >> final
> >> >> >> >> >> >> >> solution), barring adding a comment signifying our
> intent
> >> >> >> >> >> >> >> (ie
> >> >> >> >> >> >> >> the
> >> >> >> >> >> >> >> desired hex mask). I can do this if you think it
> helps.
> >> >> >> >> >> >> >
> >> >> >> >> >> >> >
> >> >> >> >> >> >> > The solution is to not care about ISO C if it doesn't
> fix
> >> >> >> >> >> >> > real
> >> >> >> >> >> >> > issues.
> >> >> >> >> >> >> > :)
> >> >> >> >> >> >>
> >> >> >> >> >> >> This is where we will just have to agree to disagree, I
> >> >> >> >> >> >> consider
> >> >> >> >> >> >> this
> >> >> >> >> >> >> issue "real enough" - it is a violation of the standard,
> and
> >> >> >> >> >> >> POSIX
> >> >> >> >> >> >> says nothing contrariwise unlike the function
> pointer/data
> >> >> >> >> >> >> pointer
> >> >> >> >> >> >> thing.
> >> >> >> >> >> >
> >> >> >> >> >> >
> >> >> >> >> >> > Well, that doesn't really help figuring out a way to do
> this in
> >> >> >> >> >> > a
> >> >> >> >> >> > way
> >> >> >> >> >> > that
> >> >> >> >> >> > we all find acceptable. So let's do that instead.
> >> >> >> >> >> >
> >> >> >> >> >> > For the enum movChannelLayoutTag, I don't think we ever
> rely on
> >> >> >> >> >> > it
> >> >> >> >> >> > being
> >> >> >> >> >> > an
> >> >> >> >> >> > enum do we? In fact, I'd say that the solution you used
> for the
> >> >> >> >> >> > DCA
> >> >> >> >> >> > enums
> >> >> >> >> >> > (use macros instead of enums) would work here also.
> >> >> >> >> >>
> >> >> >> >> >> Well, there are some arrays defined in terms of this. The
> type of
> >> >> >> >> >> the
> >> >> >> >> >> array will need to be changed appropriately. I hence gave
> this as
> >> >> >> >> >> the
> >> >> >> >> >> solution producing the minimal diff while sticking to the
> >> >> >> >> >> standard.
> >> >> >> >> >> This one I thus strongly prefer keeping it as in the above
> patch.
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >> > Right, but it doesn't fix the issue. The individual bits of
> the
> >> >> >> >> > value
> >> >> >> >> > may
> >> >> >> >> > have the same value as currently and you're not causing that
> one
> >> >> >> >> > compiler
> >> >> >> >> > warning. But you're still assigning a negative/signed value
> to a
> >> >> >> >> > field
> >> >> >> >> > that
> >> >> >> >> > is used as unsigned. See this piece of code:
> >> >> >> >> >
> >> >> >> >> > struct MovChannelLayoutMap {
> >> >> >> >> > uint32_t tag; << unsigned
> >> >> >> >> > uint64_t layout;
> >> >> >> >> > };
> >> >> >> >> >
> >> >> >> >> > static const struct MovChannelLayoutMap
> mov_ch_layout_map_misc[] =
> >> >> >> >> > {
> >> >> >> >> > [..]
> >> >> >> >> > { MOV_CH_LAYOUT_UNKNOWN, 0 }, << assigning a
> >> >> >> >> > signed/negative
> >> >> >> >> > value
> >> >> >> >>
> >> >> >> >> So what? This is completely portable, signed to unsigned
> conversion
> >> >> >> >> has well defined semantics (e.g
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> https://stackoverflow.com/questions/50605/signed-to-unsigned-conversion-in-c-is-it-always-safe
> ),
> >> >> >> >> essentially guaranteeing identical bit patterns.
> >> >> >> >
> >> >> >> >
> >> >> >> > Then why "fix" the enum?
> >> >> >>
> >> >> >> Because the hex literal to int conversion is implementation
> defined,
> >> >> >> with no guarantees from the standard. It can in fact raise an
> >> >> >> implementation defined signal.
> >> >> >> The new method at least guarantees identical bit representation
> on 2's
> >> >> >> complement (only thing we care/assume), and has well defined, i.e
> >> >> >> specified semantics as given in the above link.
> >> >> >
> >> >> >
> >> >> > This is getting very fuzzy very quickly. My impression is that you
> care
> >> >> > more
> >> >> > about one spec violation than the other because one raises a
> compiler
> >> >> > warning but the other doesn't...
> >> >>
> >> >> No, I don't. That is simply false, read the point above. I care about
> >> >> well defined semantics vs implementation defined behavior. I can't do
> >> >> anything about "your impressions", over which I don't have much
> >> >> influence.
> >> >>
> >> >> >
> >> >> > But as said before, I like to be solution driven. Why not make enum
> >> >> > MovChannelLayout a series of defines? Doesn't that solve all issues
> >> >> > without
> >> >> > the drawbacks?
> >> >>
> >> >> I am also "solution driven" - the fact that my solution does not
> match
> >> >> yours does not mean in any way that I am less "solution driven".
> >> >
> >> >
> >> > Wait, there's a misunderstanding here. I agree that your solution
> fixes the
> >> > problem you're seeing. But, I raised an objection, so, we have a new
> >> > problem. With "solution driven", I'm trying to help overcome my own
> >> > objection and come up with a new, alternate solution that overcomes my
> >> > objection, while still solving your problem. There may be other ways
> to do
> >> > the same thing, and you're free to propose alternatives. But, like
> mine,
> >> > they need to both fix your problem as well as overcome my objection.
> >>
> >> I proposed a comment as a solution to meet your readability concern.
> >>
> >> >
> >> >> There is the concrete drawback of a larger diff and type change from
> >> >> the enum array, etc and likely greater scope for mistakes as a
> result.
> >> >
> >> >
> >> > Small vs. big diff is a "preference" in FFmpeg, that is, we "prefer"
> smaller
> >> > diffs over bigger diffs, everything all being equal. However, not
> everything
> >> > else is equal in this case. Plus, the diff is not that big.
> >>
> >> I still feel the benefits of the one line diff (with associated
> >> comment) far outweigh the costs of your solution. Michael also raised
> >> the related "prettiness" concern, and I feel your solution scores
> >> negatively on that aspect compared to mine. Of course, that is
> >> subjective.
> >> In fact, while I by no means feel that my solution is close to
> >> optimal, I feel strongly enough about your proposed solution to oppose
> >> it with whatever means I have available, unless amended of course.
> >>
> >> I doubt this opposition amounts to much though, given your senior
> >> state and my recent joining. Bugs get introduced for all kinds of
> >> reasons, even in one liners and review. It is a simple fact that
> >> larger diffs are likely more trouble, especially for something as
> >> minor as this.
> >
> > maybe something like this could be a compromise ?
> >
> > --- a/libavformat/cinedec.c
> > +++ b/libavformat/cinedec.c
> > @@ -50,7 +50,7 @@ enum {
> > CFA_BAYER = 3, /**< GB/RG */
> > CFA_BAYERFLIP = 4, /**< RG/GB */
> >
> > - CFA_TLGRAY = 0x80000000,
> > +#define CFA_TLGRAY 0x80000000
> > CFA_TRGRAY = 0x40000000,
> > CFA_BLGRAY = 0x20000000,
> > CFA_BRGRAY = 0x10000000
> > --- a/libavformat/mov_chan.c
> > +++ b/libavformat/mov_chan.c
> > @@ -45,7 +45,7 @@
> > * do not specify a particular ordering of those channels."
> > */
> > enum MovChannelLayoutTag {
> > - MOV_CH_LAYOUT_UNKNOWN = 0xFFFF0000,
> > +#define MOV_CH_LAYOUT_UNKNOWN 0xFFFF0000
> > MOV_CH_LAYOUT_USE_DESCRIPTIONS = ( 0 << 16) | 0,
> > MOV_CH_LAYOUT_USE_BITMAP = ( 1 << 16) | 0,
> > MOV_CH_LAYOUT_DISCRETEINORDER = (147 << 16) | 0,
> >
>
> Thanks a lot for your effort. I am happy with your solution for
> mov_chan. For cinedec, I am anyway going to change and make all
> CFA_*RAY flags based on a macro.
>
> As long as Ronald (and others) are on board, I will rework and post
> patchv3.
That's good with me.
Ronald
More information about the ffmpeg-devel
mailing list