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.
You could do the same for the last 4 values of the cinething changes, they
are clearly not enums, but flags. Flags are always unsigned, so if enums
are unsigned and that causes an issue (as it does here), it makes sense to
fix that at its root, i.e. by making the value unsigned and it thus also
being a macro instead of an enum. Again, only the last 4 elements, the
first 5 appear to be genuine enums.
Ronald
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
