[FFmpeg-devel] r9017 breaks WMA decoding on Intel Macs

[Zuxy Meng]

Hi,

2007/5/29, Guillaume Poirier <gpoirier at mplayerhq.hu>:
> Hi,
>
> Michael Niedermayer wrote:
> > On Tue, May 29, 2007 at 03:13:16PM +0200, Guillaume Poirier wrote:
> >> Le 29 mai 07 ? 11:58, Guillaume POIRIER a ?crit :
> >>> On 5/29/07, Zuxy Meng <zuxy.meng at gmail.com> wrote:
> >>>> 2007/5/29, Guillaume Poirier <gpoirier at mplayerhq.hu>:
> >>>>> Le 28 mai 07 ? 01:26, Trent Piepho a ?crit :
> >>>>>
> >>>>>> On Mon, 28 May 2007, Guillaume POIRIER wrote:
> >>>>>>> On 5/28/07, Trent Piepho <xyzzy at speakeasy.org> wrote:
> >>>>>> I'm attaching a patch showing the right way to do.
> >>>>> Ok, I get the idea. Inline asm is pretty much black magic to me.
> >>>>>
> >>>>> Your patch doesn't work, as it doesn't even assemble, but attached
> >>>>> corrected patch does fix the problem.
> >>>>>
> >>>>> Zuxy, Michael, what do you think about it?
> >>>>> Note that I did try to putting all asm blocks of fft_sse.c as "asm
> >>>>> volatile" but that didn't fix the problem alone.
> >>>> It looks good to me, but is the "asm" -> "asm volatile" part
> >>>> absolutely necessary?
> >>> Don't know. This sure can be tested. I'd like to point out that every
> >>> ASM block in lavc is declared as "asm volatile", except in fft_sse.c.
> >>> Why is that? Why would you want them to be left as non-volatile?
> >>>
> >>> Won't GCC try to outsmart us sooner or later and break your beautiful
> >>> little ASM blocks? ;-)
> >> Ok, I just tested it: adding "volatile" keyword doesn't help fixing
> >> the bug, so I removed it, hence attached patch.
> >
> > id like to understand precissely what caused the problem and how before
> > some patch is applied
>
> I agree completely.
>
> >
> > someonw should use gcc -S and gdb to look at what gets
> > misscompiled/missassembled exactly
>
> I'll try to see what's wrong. In the meantime, I'd like to point out
> the message (which I had failed to see before) that I'm getting when
> compiling vanilla sources:
>
>
> cc -I../libswscale -I../libavcodec  -DHAVE_AV_CONFIG_H
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_ISOC9X_SOURCE -I.. -I..
> -I../libavutil -Wdeclaration-after-statement -I. -I.. -I../libavutil
> -O4 -march=pentium-m -mtune=pentium-m -pipe -ffast-math
> -fomit-frame-pointer -mdynamic-no-pic -falign-loops=16 -DSYS_DARWIN
> -DCONFIG_DARWIN -shared-libgcc -D_LARGEFILE_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE
> -I/Users/guillaume/Prgm/x264 -I/usr/local/include
> -I/opt/local/include/freetype2 -I/opt/local/include  -c -o
> i386/fft_sse.o i386/fft_sse.c
> In file included from ../libavcodec/dsputil.h:33,
>                 from i386/fft_sse.c:21:
> ../libavcodec/avcodec.h:2501: warning: 'ImgReSampleContext' is deprecated
> ../libavcodec/avcodec.h:2507: warning: 'ImgReSampleContext' is deprecated
> {standard input}:190:Missing operand value assumed absolute 0.
> {standard input}:191:Missing operand value assumed absolute 0.
> {standard input}:194:Missing operand value assumed absolute 0.
> {standard input}:195:Missing operand value assumed absolute 0.
> {standard input}:347:Missing operand value assumed absolute 0.
> {standard input}:349:Missing operand value assumed absolute 0.
> {standard input}:353:Missing operand value assumed absolute 0.
> {standard input}:373:Missing operand value assumed absolute 0.
>
>
> The patched version doesn't have these message:
>
> cc -I../libswscale -I../libavcodec  -DHAVE_AV_CONFIG_H
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_ISOC9X_SOURCE -I.. -I..
> -I../libavutil -Wdeclaration-after-statement -I. -I.. -I../libavutil
> -O4 -march=pentium-m -mtune=pentium-m -pipe -ffast-math
> -fomit-frame-pointer -mdynamic-no-pic -falign-loops=16 -DSYS_DARWIN
> -DCONFIG_DARWIN -shared-libgcc -D_LARGEFILE_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE64_SOURCE
> -I/Users/guillaume/Prgm/x264 -I/usr/local/include
> -I/opt/local/include/freetype2 -I/opt/local/include  -c -o
> i386/fft_sse.o i386/fft_sse.c
> In file included from ../libavcodec/dsputil.h:33,
>                 from i386/fft_sse.c:21:
> ../libavcodec/avcodec.h:2501: warning: 'ImgReSampleContext' is deprecated
> ../libavcodec/avcodec.h:2507: warning: 'ImgReSampleContext' is deprecated

These warnings comes from the assembler not the compiler about cases
like 16+(%esi). The FSF as treats this as equivalent to 16+0(esi) ==
16(esi) (therefore the assumed 0). If the Apple as treats it
differently without even a warning then the result is catastrophic...

> I'm attaching both version (the one with -vanilla postfix is the one
> from trunk, that is getting misscompiled).
>
> I'll have a look at it tonight, but in the meantime, if someone wants
> to look at it....
>
> Guillaume
>
>        .const
>        .align 4
[...]
>L32:
>       addl    $4, %ebx
>       movaps          (%eax), %xmm0
>       movaps       16+(%eax), %xmm4
>       movlps          (%ecx), %xmm1
>       movlps        8+(%ecx), %xmm5

Actually gcc -S can't help diagnostic here: we don't know how
"16+(%eax)" will be assembled so you should have done an 'objdump -d'
instead.

I'm sorry I don't access to any OS X boxes so I can't really help you
much in shooting down the problem although it's me who introduced this
regression. I hope you don't feel annoyed.
-- 
Zuxy
Beauty is truth,
While truth is beauty.
PGP KeyID: E8555ED6