[FFmpeg-devel] [PATCHv2] all: fix enum definition for large values

Ronald S. Bultje rsbultje at gmail.com
Wed Oct 28 19:39:31 CET 2015


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

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?

Ronald


More information about the ffmpeg-devel mailing list