[MPlayer-advusers] mplayer crash when geometry changes

[David Hollister]

On 12/ 3/11 02:15 PM, Reimar Döffinger wrote:
> On Sat, Dec 03, 2011 at 01:44:34PM -0700, David Hollister wrote:
>> Hi Reimar,
>>
>> Thanks for your quick response.
>>
>> On 12/ 3/11 11:21 AM, Reimar Döffinger wrote:
>>> On Sat, Dec 03, 2011 at 10:50:16AM -0700, David Hollister wrote:
>>>> First off, the system is running Solaris 11.  I always have to make
>>>> a handful of modifications in order to get it to compile, but I've
>>>> never had problems before.  (I can provide all the diffs I have to
>>>> make whenever I update the source if anybody really wants to know).
>>>
>>> Unless they are due to not having switched Solaris to the POSIX
>>> environment we would want to fix those issues, yes/
>>
>> I am attaching another diff containing the changes I make to get
>> things to work for Solaris.  They are actually much smaller now than
>> they were a few months ago :)
>>
>> A quick description of why:
>>
>> mp3lib/dct64_sse.c: As is, I'll get the following errors:
>>
>> Assembler: dct64_sse.c
>> 	"<stdin>", line 250 : Illegal mnemonic
>> 	Near line: "	fistps 512(%esi)"
>> 	"<stdin>", line 250 : Syntax error
>> ...
>> 	"<stdin>", line 258 : Illegal mnemonic
>> 	Near line: "	fists  256(%ebx)"
>> 	"<stdin>", line 258 : Syntax error
>> 	Near line: "	fists  256(%ebx)"
>> ...
>>
>> This is with the following gcc: gcc (GCC) 4.6.2 20110826
>> (prerelease) (also happens with gcc 4.3.3 and 4.5.x).
>
> No surprise since it is a binutils issue, you can change compilers
> all you want and it won't make a difference.
> But did you test your change? My understanding is that these should
> be 16-bit stores, and removing the "s" would make them 32 bit.
> Though my tests indicate that "s" seems to be the default here.
> Anyway mp3lib is deprecated so just disabling it might be the better
> solution.

Fair enough.  I'll just disable it.  For the record, though, I've been 
running various builds of mplayer with this change for several months 
now.  I mostly run mencoder, not mplayer, but I've not seen any issues 
with any of the encodings I've done.

>> libmpcodecs/dec_video.c: This change is not necessary to compile...
>> it will compile as-is.  However, it results in the following when I
>> try to run:
>>
>> $ file mplayer
>> mplayer: ELF 32-bit LSB executable, Intel 80386, version 1,
>> dynamically linked (uses shared libs), not stripped, uses FPU TSC
>> CMOV MMX AMD_3DNow SSE SSE2 SSE3 SSSE3 SSE4.1
>>
>> $ ./mplayer
>> ld.so.1: mplayer: fatal: mplayer: hardware capability (CA_SUNW_HW_1)
>> unsupported: 0x100  [ AMD_3DNow ]
>> Killed
>>
>> So, I suppose the Solaris ld.so will refuse to run binaries with
>> hardware capabilities it knows aren't supported by the current
>> processor.  Just FYI, There are a couple of ffmpeg files that
>> require similar changes (libavcodec/x86/cavsdsp_mmx.c and
>> dsputilenc_mmx.c)
>
> That is just broken, instructions in the binary don't generally have
> much to do with instructions needed to execute.
> Also, this:
> http://ffmpeg.org/pipermail/ffmpeg-cvslog/2011-September/040912.html
> seems to indicate this should only happen when compiling with suncc?

I would agree.  I will see what I can find out about this.  However, 
it's really a linker issue, not a compiler issue.  I don't recall 
whether I've tried with the GNU linker, but I haven't had a lot of luck 
with it on Solaris in the past.

>> help/help_mp-en.h: As is, this will result in the following compile error:
>>
>> libao2/ao_sun.c: In function 'realtime_samplecounter_available':
>> libao2/ao_sun.c:161:35: error: 'MSGTR_AO_SUN_RtscWriteFailed'
>> undeclared (first use in this function)
>> libao2/ao_sun.c:161:35: note: each undeclared identifier is reported
>> only once for each function it appears in
>> gmake: *** [libao2/ao_sun.o] Error 1
>
> Strange, I am sure that worked at some point.
>
>>>> During my testing, I found that it's only the frame passed in to
>>>> draw_image/draw_slice immediately following the geometry change that
>>>> has the incorrect width and height.  Subsequent frames were correct.
>>>
>>> Please provide a sample file.
>>> Also say whether this also happens with -noslices and -vo x11 or -vo gl
>>> or such.
>>
>> Unfortunately, the sample file is about 1.8GB in size.  However, I'm
>> pretty sure I can easily generate one that is much smaller.  I'll
>> try to do that and will post a link as soon as I can.
>
> You should be able to just cut out a few MB at the transition point with
> dd.
> I think there's even a command you could bind to a key top make MPlayer
> print the current file position in bytes.
> get_property stream_pos
> I think.
>
>> With "-noslices -vo x11": Still crashes.
>>
>> With "-vo gl": Appears to work fine.
>
> Probably just luck that -vo gl works.
> Would also appreciate if you could test my patch.

I tried your patch, and it appears to work as expected.

As for a segment of that video, if you'd still like it, I'll see what I 
can do to cut out a snippet for you.  Just can't get to it right this 
moment, though.  If you still want it, let me know.

>> Index: libmpcodecs/dec_video.c
>> ===================================================================
>> --- libmpcodecs/dec_video.c	(revision 34378)
>> +++ libmpcodecs/dec_video.c	(working copy)
>> @@ -423,12 +423,16 @@
>>   #if HAVE_MMX
>>       // some codecs are broken, and doesn't restore MMX state :(
>>       // it happens usually with broken/damaged files.
>> +#if HAVE_AMD3DNOW
>>       if (gCpuCaps.has3DNow) {
>>           __asm__ volatile ("femms\n\t":::"memory");
>> -    } else if (gCpuCaps.hasMMX) {
>> +    } else
>> +#else
>> +    if (gCpuCaps.hasMMX) {
>>           __asm__ volatile ("emms\n\t":::"memory");
>>       }
>>   #endif
>> +#endif
>
> That will actually break --enable-runtime-cpudetection builds that
> are run on a CPU without 3DNow - neither emms nor femms will be
> executed in that case.
> Since 3DNow implies MMX the #if should just be split I think.
> Might get a bit ugly though.

If you look closely at the patch, the #if HAVE_AMD3DNOW essentially is 
breaking the if/else into just one "if" in the case where HAVE_AMD3DNOW 
is 0.  I'm not arguing this is the best way to do this, or is even 
correct.  Just wanted to ensure you didn't miss that.

David