On Wed, Oct 28, 2015 at 2:39 PM, Ronald S. Bultje <rsbul...@gmail.com> wrote: > Hi, > > On Wed, Oct 28, 2015 at 2:31 PM, Ganesh Ajjanagadde <gajjanaga...@gmail.com> > wrote: >> >> On Wed, Oct 28, 2015 at 11:38 AM, Ronald S. Bultje <rsbul...@gmail.com> >> wrote: >> > Hi, >> > >> > On Wed, Oct 28, 2015 at 11:00 AM, Ganesh Ajjanagadde >> > <gajjanaga...@gmail.com> wrote: >> >> >> >> On Wed, Oct 28, 2015 at 10:53 AM, Ronald S. Bultje <rsbul...@gmail.com> >> >> wrote: >> >> > Hi, >> >> > >> >> > On Wed, Oct 28, 2015 at 10:48 AM, Ganesh Ajjanagadde >> >> > <gajjanaga...@gmail.com> wrote: >> >> >> >> >> >> On Wed, Oct 28, 2015 at 10:34 AM, Ronald S. Bultje >> >> >> <rsbul...@gmail.com> >> >> >> wrote: >> >> >> > Hi, >> >> >> > >> >> >> > On Wed, Oct 28, 2015 at 8:20 AM, Ganesh Ajjanagadde >> >> >> > <gajjanaga...@gmail.com> >> >> >> > wrote: >> >> >> >> >> >> >> >> On Wed, Oct 28, 2015 at 7:00 AM, Ronald S. Bultje >> >> >> >> <rsbul...@gmail.com> >> >> >> >> wrote: >> >> >> >> > Hi, >> >> >> >> > >> >> >> >> > On Tue, Oct 27, 2015 at 10:58 PM, Ganesh Ajjanagadde >> >> >> >> > <gajjanaga...@gmail.com> wrote: >> >> >> >> >> >> >> >> >> >> On Tue, Oct 27, 2015 at 10:41 PM, Ronald S. Bultje >> >> >> >> >> <rsbul...@gmail.com> >> >> >> >> >> wrote: >> >> >> >> >> > Hi, >> >> >> >> >> > >> >> >> >> >> > On Tue, Oct 27, 2015 at 8:53 PM, Ganesh Ajjanagadde >> >> >> >> >> > <gajjanaga...@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 <gajjanaga...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel