[FFmpeg-cvslog] r22937 - in trunk: libavcodec/amrnbdec.c libavcodec/atrac1.c libavcodec/audioconvert.c libavcodec/qcelpdata.h libavcodec/qcelpdec.c libavcodec/ra288.c libavcodec/sipr.c libavcodec/sipr16k.c libavco...

[Ronald S. Bultje]

Hi,

On Sat, Apr 24, 2010 at 12:33 PM, Andreas ?man <andreas at lonelycoder.com> wrote:
> rbultje wrote:
>> Author: rbultje
>> Date: Wed Apr 21 19:57:48 2010
>> New Revision: 22937
>>
>> Log:
>> Move clipping of audio samples (for those codecs outputting float) from
>> decoder
>> to the audio conversion routines.
>>
>> Modified: trunk/libavutil/common.h
>>
>> ==============================================================================
>> --- trunk/libavutil/common.h ? ?Wed Apr 21 19:51:37 2010 ? ? ? ?(r22936)
>> +++ trunk/libavutil/common.h ? ?Wed Apr 21 19:57:48 2010 ? ? ? ?(r22937)
>> @@ -145,6 +145,17 @@ static inline av_const int16_t av_clip_i
>> ?}
>> ?/**
>> + * Clips a signed 64-bit integer value into the -2147483648,2147483647
>> range.
>> + * @param a value to clip
>> + * @return clipped value
>> + */
>> +static inline av_const int32_t av_clipl_int32(int64_t a)
>> +{
>> + ? ?if ((a+2147483648) & ~2147483647) return (a>>63) ^ 2147483647;
>> + ? ?else ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?return a;
>> +}
[..]
> This trips a warning if not compiled with c99 (for example an
> application that happens to include libavutil/common.h). Not only
> annoying but for applications compiling with -Werror this results in
> build fail.
>
> common.h:154: error: this decimal constant is unsigned only in ISO C90
>
> I'm not sure but do we require that apps pulling include files from
> FFmpeg to be compiled in c99 mode? I think not, but I don't really know.
>
> I guess this could be easily fixed by adding the correct LL or ULL
> postfixes to the constants.
>
> Opinions?

The code is broken, no idea how I got to think it was OK. See attached
for a proper version, tested.

Consider this code:

int main(int argc, char **argv)
{
    int64_t x = atoll(argv[1]);
    printf("0x%llx: 0x%x\n", x, av_clipl_int32(x));
    return 0;
}

Expands to the following ASM, which seems approximately intended:

[..]
0x00001f0e <main+30>:	call   0x1f64 <dyld_stub_atoll>
0x00001f13 <main+35>:	mov    %eax,%esi
0x00001f15 <main+37>:	mov    %edx,%edi
(OK gcc is being stupid in the above 2 lines)
0x00001f17 <main+39>:	add    $0x80000000,%eax
0x00001f1c <main+44>:	adc    $0x0,%edx
0x00001f1f <main+47>:	xor    %eax,%eax
0x00001f21 <main+49>:	mov    %esi,-0x1c(%ebp)
0x00001f24 <main+52>:	mov    %edx,%ecx
0x00001f26 <main+54>:	or     %eax,%ecx
0x00001f28 <main+56>:	je     0x1f39 <main+73>
0x00001f2a <main+58>:	mov    %edi,%edx
0x00001f2c <main+60>:	sar    $0x1f,%edx
0x00001f2f <main+63>:	mov    %edx,%eax
0x00001f31 <main+65>:	xor    $0x7fffffff,%eax
0x00001f36 <main+70>:	mov    %eax,-0x1c(%ebp)
0x00001f39 <main+73>:	mov    -0x1c(%ebp),%eax
0x00001f3c <main+76>:	mov    %eax,0xc(%esp)
0x00001f40 <main+80>:	mov    %esi,0x4(%esp)
0x00001f44 <main+84>:	mov    %edi,0x8(%esp)
0x00001f48 <main+88>:	lea    0x9c(%ebx),%eax
0x00001f4e <main+94>:	mov    %eax,(%esp)
0x00001f51 <main+97>:	call   0x1f70 <dyld_stub_printf>
[..]

And gives the correct output:

bash-3.2$ ./test 0
0x0: 0x0
bash-3.2$ ./test -1
0xffffffffffffffff: 0xffffffff
bash-3.2$ ./test -2147483648
0xffffffff80000000: 0x80000000
bash-3.2$ ./test -2147483649
0xffffffff7fffffff: 0x80000000
bash-3.2$ ./test -2147483647
0xffffffff80000001: 0x80000001
bash-3.2$ ./test 2147483647
0x7fffffff: 0x7fffffff
bash-3.2$ ./test 2147483640
0x7ffffff8: 0x7ffffff8
bash-3.2$ ./test 2147483649
0x80000001: 0x7fffffff

OK to apply (along with cosmetic patch to align the returns)?

Ronald
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clip.patch
Type: application/octet-stream
Size: 457 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-cvslog/attachments/20100424/8c0009d4/attachment.obj>