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

Ganesh Ajjanagadde gajjanagadde at gmail.com
Wed Oct 28 19:46:43 CET 2015


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

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.

>
> Ronald


More information about the ffmpeg-devel mailing list