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

[Michael Niedermayer]

On Wed, Jan 07, 2009 at 04:13:40PM +0530, Jai Menon wrote:
> Hi,
> 
> On Thu, Dec 11, 2008 at 3:05 PM, M?ns Rullg?rd <mans at mansr.com> wrote:
> > "Jai Menon" <jmenon86 at gmail.com> writes:
> 
> [...]
> 
> >>>>>              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.
> >>>
> >>
> >> done.
> >>
> >>> 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.
> >>
> >> Also done.
> >
> > With this variant the cast is wrong.  Think about what happens if int
> > is wider than 32 bits.
> 
> Is the attached patch okay?
> Maybe this will help fix the regression failure on 64 bit systems.

i think you should read the C standard, i think you have tried every way
except the 2 correct ones.

the problem is ONLY the right shift, the left operand has some number of bits
these are 32 if its int32_t and sizeof(int)*CHAR_BIT if its int

currently it is int and you use 32, this can be fixed by casting to int32_t or
using sizeof(int)*CHAR_BIT but not both at the same time

your patch casts the wrong value.
try:

main(){
printf("%d %d %d\n",
    ((int16_t)0x4000 << 1) >> 1,
    (int16_t)(0x4000 << 1) >> 1,
    (0x4000 << 1) >> 1
    );
}

on a (normal) 32bit system to see the difference

Also, iam sorry but i do not see how this could fix any bug in x86_64 as int
is 32bits there normally.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090111/47eb0c33/attachment.pgp>