[FFmpeg-devel] [PATCH 1/2] avcodec/vda_h264: use multichar literal portably

Ganesh Ajjanagadde gajjanag at mit.edu
Sun Sep 20 20:10:21 CEST 2015


On Sun, Sep 20, 2015 at 1:39 PM, wm4 <nfxjfg at googlemail.com> wrote:
> On Sun, 20 Sep 2015 11:18:46 -0400
> Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
>
>> On Sun, Sep 20, 2015 at 10:41 AM, wm4 <nfxjfg at googlemail.com> wrote:
>> > On Fri, 18 Sep 2015 18:16:18 -0400
>> > Ganesh Ajjanagadde <gajjanagadde at gmail.com> wrote:
>> >
>> >> Multichar literals are implementation defined, and thus trigger -Wmultichar:
>> >> http://fate.ffmpeg.org/log.cgi?time=20150918202532&log=compile&slot=x86_64-darwin-gcc-5.
>> >> http://www.zipcon.net/~swhite/docs/computers/languages/c_multi-char_const.html
>> >> gives a good summary of how to deal with them; in particular this patch
>> >> results in behavior identical to that generated by GCC (which I assume is correct this case).
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> ---
>> >>  libavcodec/vda_h264.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
>> >> index 8c526c0..3279afa 100644
>> >> --- a/libavcodec/vda_h264.c
>> >> +++ b/libavcodec/vda_h264.c
>> >> @@ -339,7 +339,9 @@ int ff_vda_default_init(AVCodecContext *avctx)
>> >>      CFMutableDictionaryRef buffer_attributes;
>> >>      CFMutableDictionaryRef io_surface_properties;
>> >>      CFNumberRef cv_pix_fmt;
>> >> -    int32_t fmt = 'avc1', pix_fmt = vda_ctx->cv_pix_fmt_type;
>> >> +#define LE_CHR(a,b,c,d) ( ((a)<<24) | ((b)<<16) | ((c)<<8) | (d) )
>> >> +    int32_t fmt = LE_CHR( 'a', 'v', 'c', '1');
>> >> +    int32_t pix_fmt = vda_ctx->cv_pix_fmt_type;
>> >>
>> >>      // kCVPixelFormatType_420YpCbCr8Planar;
>> >>
>> >
>> > Sorry, but this is probably completely pointless. Apple and code using
>> > Apple APIs use this in a lot of places. Also, since this is Apple
>> > specific anyway (and thus runs on only 1 compiler and 1 platform), the
>> > portability argument doesn't make sense either.
>>
>> please see patchv2.
>>
>
> Nothing changed about this point, though.

>
> I'm not terribly opposed to your patch, but I'm wondering if it doesn't
> make the code harder to read (not adhering to Apple's conventions when
> using their API), and could potentially break things for whatever
> reason.

As for harder to read, I doubt AV_RB32 is way worse than multichar
literals. Furthermore, it is used all over the codebase for creating
such tags.
I do not understand what you mean by "not adhering to Apple's
convention": https://developer.apple.com/library/prerelease/ios/documentation/CoreFoundation/Reference/CFNumberRef/index.html#//apple_ref/c/tdef/CFNumberType.
All we pass in is a pointer to a valid int32_t. The only real question
is endianness/bit representation, but multichar literals are no
solution to that since the way they are converted to int is inherently
implementation defined (and thus triggers the warning). At least now
we "freeze" the creation of the int32_t to something well defined and
in our control (AV_RB32). Furthermore, this coincides with the
implementation of multichar literal on gcc (and likely clang due to
their similarities/compatibilities). Thus, I do not believe there will
be breakage. Unfortunately, I lack Apple stuff to actually test this,
so perhaps someone with the environment should do so.

>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


More information about the ffmpeg-devel mailing list