GNOME Bugzilla – Bug 327350
[mpeg2dec] altivec crashes on misaligned buffers
Last modified: 2006-11-23 15:22:48 UTC
Hi! When trying to play an mpg video, gstreamer crashes with a double free memory corruption. A reasonably small (4MB) test file is at http://people.ubuntu.com/~pitti/shots/test.mpg (in case it doesn't happen with any mpg file). $ totem test.mpg (totem:22843): Gtk-CRITICAL **: gtk_accel_label_set_accel_closure: assertion `gtk_accel_group_from_accel_closure (accel_closure) != NULL' failed (totem:22843): Gtk-CRITICAL **: gtk_accel_label_set_accel_closure: assertion `gtk_accel_group_from_accel_closure (accel_closure) != NULL' failed (totem:22843): GdkPixbuf-CRITICAL **: gdk_pixbuf_new_from_data: assertion `width > 0' failed (totem:22843): GdkPixbuf-CRITICAL **: gdk_pixbuf_scale: assertion `src != NULL' failed (totem:22843): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (totem:22843): GdkPixbuf-CRITICAL **: gdk_pixbuf_new_from_data: assertion `width > 0' failed (totem:22843): GdkPixbuf-CRITICAL **: gdk_pixbuf_scale: assertion `src != NULL' failed (totem:22843): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed *** glibc detected *** free(): invalid next size (normal): 0x106d9b10 *** I extracted a stack trace: Program received signal SIGSEGV, Segmentation fault.
+ Trace 65290
Thread 852767984 (LWP 22451)
This happens on powerpc, but not on amd64 (but I guess that's just a coincidence). Please tell me if you need any further information. Thanks!
Martin, this sounds like #154431 back again, can you confirm or deny?
Thanks for the pointer. The odd thing is that gst-launch-0.10 filesrc location=Desktop/test.mpg ! mpegdemux ! mpeg2dec ! autovideosink works just fine, whereas totem still crashes. Maybe totem constructs a different pipeline (e. g. I don't get the myriad of assertion errors with the gst-launch command that I get with totem). So after all it seems to be a different bug.
Or you might get different random behaviour because the memory layout is being set up differently :) Perhaps valgrind gst-launch-0.10 filesrc location=Desktop/test.mpg ! mpegdemux ! mpeg2dec ! autovideosink can tell us more, or try gst-launch-0.10 playbin uri=file:///full/path/to/file
> gst-launch-0.10 playbin uri=file:///full/path/to/file Indeed: $ gst-launch-0.10 playbin uri=file:///home/martin/Desktop/test.mpg [...] *** glibc detected *** free(): invalid next size (normal): 0x10320840 *** it also happens with the test video mentioned in #154431: $ gst-launch-0.10 playbin uri=file:///home/martin/a-mpeg2.mpg [...] Segmentation fault I'll attach a valgrind output soon.
Created attachment 57597 [details] valgrind output An awful lot of output, but I don't think it caught the actual crash. :( attaching nevertheless
New stack trace without totem overhead and all gst* packages rebuilt with debugging info and without otpmimization: $ gdb --args gst-launch-0.10 playbin uri=file:///home/martin/a-mpeg2.mpg Pipeline is PREROLLING ... [New Thread 842355952 (LWP 8707)] Pipeline is PREROLLED ... Setting pipeline to PLAYING ... [New Thread 858838256 (LWP 8708)] New clock: GstSystemClock Program received signal SIGSEGV, Segmentation fault.
+ Trace 65340
Thread 833967344 (LWP 8706)
I can't get much out of that valgrind trace either, except that it seems to want to be recompiled. I notice that there is a thread inside libmpeg2 that is performing altivec stuff. Part of the report in bug 154431 mentions that the crash went away when libmpeg2 was recompiled without altivec support, so you probably want to try that.
Confirmed - With altivec support disabled in libmpeg2, the crash doesn't occur any more. OTOH mpeg2dec test.mpg (and any other mpg video I tested) does not crash even with altivec enabled. OK, so please feel free to reopen bug 154431 and close this as a dup if you want to. Thank you for your help so far!
BTW, James Morrison's alternative solution of building libmpeg2 with gcc 4.0 (i. e. with the default compiler, which is also used to build totem and the rest of gnome), doesn't work, it causes the same crash.
the altivec code needs special alignment of the video buffers. One quick option would be to disable the altivec optimisations until this is fixed.
Created attachment 75796 [details] [review] gstmpeg2dec-ppc.diff The attached patch fixes the bug for me. libmpeg2 requires it's output buffers to start at a 16byte aligned address and the altivec optimizations explode when this is not guaranteed.
Created attachment 75798 [details] [review] gstmpeg2dec-ppc.diff hrm, s/+ 8/+ 15/g
Created attachment 75803 [details] [review] gstmpeg2dec-ppc.diff ok now it's really correct...
Created attachment 75804 [details] [review] gstmpeg2dec-ppc.diff
I tested Sebastian's patch, works like charm. Thanks a lot!
Thanks! Patch by: Sebastian Droege <slomo at ubuntu dot com> * ext/mpeg2dec/gstmpeg2dec.c: (gst_mpeg2dec_finalize), (gst_mpeg2dec_alloc_buffer), (init_dummybuf), (handle_slice): * ext/mpeg2dec/gstmpeg2dec.h: libmpeg2 requires its output buffers to start at a 16byte aligned address or the altivec optimizations will explode.
Don't think this is quite right yet, since downstream converter elements will complain about wrong unit sizes due to the +16 in pad_alloc_buffer(): $ gst-launch-0.10 filesrc location=foo.mpg ! decodebin ! ffmpegcolorspace ! videoscale ! ximagesink Setting pipeline to PAUSED ... Pipeline is PREROLLING ... Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstSystemClock ** (gst-launch-0.10:2004): WARNING **: Size 115216 is not a multiple of unit size 115200 ** (gst-launch-0.10:2004): WARNING **: Size 115216 is not a multiple of unit size 115200 ** (gst-launch-0.10:2004): WARNING **: Size 115216 is not a multiple of unit size 115200 ...
Created attachment 75927 [details] [review] mpeg2dec-3.diff new patch that also fixes the bug tim discovered
But this patch misses correct alignment for non-8n*8m movies... will fix that. Another thing that I noticed is that you almost never get a correctly aligned buffer, would it make sense to just call and test pad_alloc_buffer until it returned a misaligned buffer the first time and from then on just get our own buffers? Allocating twice all the time is probably not the best performance-wise...
Created attachment 75946 [details] [review] mpeg2dec-4.diff ok, there we go... this should be fine now and work in all situations...
Patch looks fine to me in principle. What I wonder though is whether there's a way to automatically detect or hard-code the cases where we actually need aligned buffers, so that we don't go the expensive route on platforms/systems where unaligned buffers work just fine.
libmpeg2 always requires 16byte aligned buffers by convention. It works just fine with unaligned buffers with the C/MMX/SSE implementations at least but that's in no way guaranteed unfortunately :(
Wim, any thoughts on this? Do you think it would be the best to force alignment only on PPC for now and wait until libmpeg2 really requires it on !PPC?
Patch by: Sebastian Droege <slomo@circular-chaos.org> * ext/mpeg2dec/gstmpeg2dec.c: (gst_mpeg2dec_init), (gst_mpeg2dec_reset), (gst_mpeg2dec_alloc_sized_buf), (gst_mpeg2dec_alloc_buffer), (init_dummybuf), (handle_slice): * ext/mpeg2dec/gstmpeg2dec.h: Align buffers to a 16 byte boundary so the altivec optimisations don't crash. Fixes #327350.
There's something about this bug that I don't understand. Are people saying that they've actually seen gst_pad_alloc_buffer return a non-16-byte aligned buffer? I'm expecting that would only ever happen when allocating a normal memory buffer, but the standard scenario should be that x[v]imagesink is allocating a shared memory buffer, which should be paged aligned and therefore 16-byte aligned implicitly.
My statement that one almost never gets 16byte aligned buffers was based on a test with basetransform elements between mpeg2dec and xvimagesink. When linking mpeg2dec and xvimagesink directly I never got a non-16byte aligned buffer.
OK, that makes sense - although our standard elements (ffmpegcolorspace, videoscale) should be doing passthrough for the standard xvimagesink scenario. ximagesink though, videoscale often steps in to correct pixel-aspect-ratios. Even so, I dislike the way mpeg2dec allocates and discards a buffer like that - it ought to hang onto it in a local pointer, for copying the output video into when it gets around to it. Setting a global var to indicate that downstream doesn't support 16-byte aligned allocs is misleading - it might change at any moment if the window gets resized, or the pipeline gets replugged, resulting in a non-optimal codepath being taken that can fairly easily be avoided.