[FFmpeg-devel] [PATCH] Issue 636 : ALAC encoding sometimes fails

[Måns Rullgård]

Michael Niedermayer wrote:
> On Wed, Dec 10, 2008 at 10:49:10AM +0530, Jai Menon wrote:
>> Hi,
>>
>> -------- Original-Nachricht --------
>> > Datum: Fri, 5 Dec 2008 13:16:59 +0100
>> > Von: Michael Niedermayer
>> <michaelni at gmx.at<http://service.gmx.net/de/cgi/g.fcgi/mail/new?CUSTOMERNO=12720674&t=uk602561993.1228886151.33ee9c8f&to=michaelni%40gmx.at>
>> >
>> > An: FFmpeg development discussions and patches
>> <ffmpeg-devel at mplayerhq.hu<http://service.gmx.net/de/cgi/g.fcgi/mail/new?CUSTOMERNO=12720674&t=uk602561993.1228886151.33ee9c8f&to=ffmpeg-devel%40mplayerhq.hu>
>> >
>> > Betreff: Re: [FFmpeg-devel] [PATCH] Issue 636 : ALAC encoding
>> sometimes    fails
>>
>> > On Fri, Dec 05, 2008 at 10:09:16AM +0530, Jai Menon wrote:
>> > > Hi,
>> > >
>> > > Attached patch fixes issue 636 and a few other fringe cases where the
>> > > alac encoder produces
>> > > lossy output. I'm behind a NAT and svn doesn't work, so could someone
>> > > please apply. Thanks.
>> > >
>> > > Regards,
>> > >
>> > > Jai
>> >
>> > > Index: libavcodec/alacenc.c
>> > > ===================================================================
>> > > --- libavcodec/alacenc.c    (revision 15797)
>> > > +++ libavcodec/alacenc.c    (working copy)
>> > > @@<http://service.gmx.net/de/cgi/g.fcgi/mail/new?CUSTOMERNO=12720674&t=uk602561993.1228886151.33ee9c8f&to=>-253,7
>> +253,8
>> @@<http://service.gmx.net/de/cgi/g.fcgi/mail/new?CUSTOMERNO=12720674&t=uk602561993.1228886151.33ee9c8f&to=>
>> > >
>> > >              sum >>= lpc.lpc_quant;
>> > >              sum += samples[0];
>> > > -            residual[i] = samples[lpc.lpc_order+1] - sum;
>> > > +            residual[i] = (samples[lpc.lpc_order+1] - sum) << (32 -
>> > s->write_sample_size) >>
>> > > +                          (32 - s->write_sample_size);
>> > >              res_val = residual[i];
>> >
>> > you are missing a int32_t cast in there, without it this wont work on
>> > systems where int is not exactly 32bit
>> >
>>
>> Oversight on my part. 16 bit systems are a nightmare from which I'm trying
>> to awake.
>
> int could be 64bit, it cannot be 16bit on a POSIX system as its required
> by POSIX to be >= 32

36-bit words used to be commonplace once upon a time...

>> @@ -253,7 +253,7 @@
>>
>>              sum >>= lpc.lpc_quant;
>>              sum += samples[0];
>> -            residual[i] = (samples[lpc.lpc_order+1] - sum) << (32 -
>> s->write_sample_size) >>
>> +            residual[i] = (int32_t)(samples[lpc.lpc_order+1] - sum) << (32
>> - s->write_sample_size) >>
>>                            (32 - s->write_sample_size);
>
> id do the cast after the << because its the >> that differes from where
> the sign bit is

Put differently, if int is wider than 32 bits the left-hand argument
to >> must be sign-extended from 32 bits to this size for the >> to
work as intended.

It would be more efficient if 32 were replaced with CHAR_BIT*sizeof(int)
instead.  This could be put in a nicely named macro, making it even more
obvious what the intent of the code is.

-- 
M?ns Rullg?rd
mans at mansr.com