[FFmpeg-devel] [PATCH] ac3_parser type punning fix

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Sat, Oct 18, 2008 at 10:44:16PM -0700, David DeHaven wrote:
>> 
>> > Nothing major, I just applied the same fix that was applied to  
>> > aac_parser.c recently...
>> >
>> > -DrD-
>> > <ac3_parser.patch>
>> 
>> I think there was confusion about the whole point of this patch...
>> 
>> On the four x86 based machine/OS combos I built and tested on (i686-pc- 
>> mingw32, 2x i686-pc-linux, i686-apple-darwin), gcc with optimizations  
>> at more than -O0 would produce machine code that would *only* byteswap  
>> 32 of the 64 bits in the "state" variable passed to both aac_sync and  
>> ac3_sync leading to complete and utter misinterpretation of the header  
>> information. The result: at the *least* would be that it reported the  
>> wrong information to stdout, at worst it would crash due to having the  
>> wrong codec parameters. I can't honestly believe that noone else has  
>> seen this problem, especially considering aac_sync has been already  
>> fixed unless that was simply a "compiler warning" fix.
>> 
>> I can provide disassembly snippets if you need further convincing...
>
> I have no doubt that gcc generates wrong code i also have no doubt that
> the code breaks strict aliassing rules.
> The doubt i have is the relation, so to convince me first tell us if the
> code works with all identical but -fno-strict-aliasing added. If not
> the bug is elsewhere, and this patch is hiding it.

The C code is the same as in aac_parser, and I confirmed it was broken
myself.  IIRC, it is only gcc 4.3 that optimises it "incorrectly".
Scare quotes because the C code is what is incorrect, breaking strict
aliasing rules.  You accepted the same patch for the aac parser
without questions, so why the hesitation now?

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