On Wed, Oct 28, 2015 at 4:00 PM, 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.
>
Since you like pedantic C conformity, this does not actually say that.
It only says that it happens to be like that on a two-complements
system, but C itself does not guarantee it.
- Hendrik
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
