[Ffmpeg-devel] Re: [Ffmpeg-cvslog] r8420 - trunk/libavcodec/dv.c

[John Dalgliesh]

On Sat, 24 Mar 2007, Roman Shaposhnik wrote:
> On Fri, 2007-03-16 at 00:45 +0100, gpoirier wrote:
>> Modified: trunk/libavcodec/dv.c
>> ==============================================================================
>> --- trunk/libavcodec/dv.c	(original)
>> +++ trunk/libavcodec/dv.c	Fri Mar 16 00:45:20 2007
>> @@ -838,7 +838,7 @@ static inline void dv_encode_video_segme
>>      uint8_t*  data;
>>      uint8_t*  ptr;
>>      int       do_edge_wrap;
>> -    DECLARE_ALIGNED_8(DCTELEM, block[64]);
>> +    DECLARE_ALIGNED_16(DCTELEM, block[64]);
>>      EncBlockInfo  enc_blks[5*6];
>>      PutBitContext pbs[5*6];
>>      PutBitContext* pb;
>> @@ -846,7 +846,7 @@ static inline void dv_encode_video_segme
>>      int       vs_bit_size = 0;
>>      int       qnos[5];
>>
>> -    assert((((int)block) & 7) == 0);
>> +    assert((((int)block) & 15) == 0);
>
>  Are you sure this works?

It works for me on my platform (x86, Mac OS X, gcc 4.0.1 (Apple)). The 
problem does not seem to occur on linux. The reporter of the bug was on 
Mac OS X too IIRC.

> I see no way of aligning anything on the
> x86 stack for more than 8.

What do you mean by 'no way'? The code above should work - the compiler 
should defintiely be able to align something on the stack even when the 
function is called with less alignment. If it hadn't worked I would have 
increased the size of the array to cope with any misalignment and set a 
pointer to the appropriate place... but happily I didn't need to.

Also, where do you get that 8 from? In the man page for (my) gcc it claims 
that the default value for -mpreferred-stack-boundary is 4, i.e. 16 bytes.

> And even that is kind of iffy, given
> that the x86 ABI mandates an alignment of 4.

True.

Anyway, the code should ask for the alignment it actually requires, even 
if the compiler isn't up to it. So IMHO this change is correct. If it 
doesn't work in some places (resulting in bug reports), then we can 
investigate workarounds then.

> Thanks,
> Roman.

{P^/