[FFmpeg-devel] [PATCHv2] all: fix enum definition for large values
Ganesh Ajjanagadde
gajjanagadde at gmail.com
Thu Oct 29 00:02:42 CET 2015
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.
>
> Ronald
More information about the ffmpeg-devel
mailing list