After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 680796 - [0.11] Crash because of misaligned frame data
[0.11] Crash because of misaligned frame data
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-libav
git master
Other Linux
: Normal normal
: 0.11.x
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 681454
 
 
Reported: 2012-07-29 17:07 UTC by Arnaud Vrac
Modified: 2012-09-11 13:03 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Arnaud Vrac 2012-07-29 17:07:10 UTC
I get a crash when using a libav decoder in my pipeline and when direct rendering is enabled. In this case the output frame buffers are allocated by a downstream buffer pool.

The crash happens in the put_pixels16_sse2() function in libavcodec/x86/dsputil_mmx.c:455. It happens because this function assumes that the output data is aligned on 16 bytes boundaries, and it is not the case. I can confirm that changing the movdqa instructions in this function to movdqu fixes the problem.

The problem should be fixed in gst-ffmepg by forcing the plane data of the output buffers allocated from the buffer pool to be 16-bytes aligned.
Comment 1 Tim-Philipp Müller 2012-08-03 14:20:41 UTC
0.10 or git master (0.11/1.0) ? Should be fixed in 0.11.
Comment 2 Arnaud Vrac 2012-08-03 14:45:17 UTC
This happens with git 0.11. The alignment meta adds padding to the frame buffer (top 16, right 16, left 16, bottom 32), however the buffer address passed to put_pixels16_sse2 is not 16 bits aligned.

This happens on x86 BTW, not amd64.
Comment 3 Sebastian Dröge (slomo) 2012-08-08 10:15:04 UTC
Loosely related to bug #680386
Comment 4 Sebastian Dröge (slomo) 2012-08-08 14:03:15 UTC
commit fe0a8d005d837703cf9b6649a183c75511f2f2bb
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Wed Aug 8 15:59:59 2012 +0200

    avviddec: Properly align and pad buffers for libav
    
    https://bugzilla.gnome.org/show_bug.cgi?id=680796
Comment 5 Arnaud Vrac 2012-08-10 09:16:54 UTC
Still crashes for me, I guess the buffer is now aligned however the start of the lines in the frames might not be.
Comment 6 Sebastian Dröge (slomo) 2012-08-10 10:56:46 UTC
The start of the lines should be, that's what GstVideoAlign is used for. The width and height of the frames is set to what libav wants, but maybe it tells us something wrong here :)

What exactly is your testcase that crashes?
Comment 7 Wim Taymans 2012-08-10 11:04:02 UTC
GstVideoAlign is not used yet AFAIK
Comment 8 Sebastian Dröge (slomo) 2012-08-10 11:11:52 UTC
It is and it's also implemented properly in the x*imagesink buffer pools and GstVideoBufferPool (bug #680386 happened because GstVideoAlign wasn't properly handled in vp8enc).
Comment 9 Wim Taymans 2012-08-10 11:16:04 UTC
(In reply to comment #8)
> It is and it's also implemented properly in the x*imagesink buffer pools and
> GstVideoBufferPool (bug #680386 happened because GstVideoAlign wasn't properly
> handled in vp8enc).

The stride alignment is simply ignored in x*imagesink.
Comment 10 Sebastian Dröge (slomo) 2012-08-10 11:21:46 UTC
Actually you're right, seems to be ignored in GstVideoBufferPool too. But all other fields are used correctly
Comment 11 Wim Taymans 2012-08-10 11:22:02 UTC
Also ignored in GstVideoPool, I'll see if I can fix it.
Comment 12 Arnaud Vrac 2012-08-10 11:27:07 UTC
There might also be some other problems, from the logs I see the following alignment values:


  libav gstffmpegviddec.c:1497:gst_ffmpegviddec_decide_allocation:<avdec_mpeg4-0> aligned dimension 640x480 -> 672x512 padding t:16 l:16 r:16 b:16, stride_align 7:7:7:7

default gstvideopool.c:136:gst_video_info_align: plane 0: hedge 16 vedge 16 align 7 stride 672
default gstvideopool.c:136:gst_video_info_align: plane 1: hedge 8 vedge 8 align 7 stride 336                                                                                                                                                                                                                                                                                
default gstvideopool.c:136:gst_video_info_align: plane 2: hedge 8 vedge 8 align 7 stride 336

The 7 stride align should be 15 for sse2 instructions to work. Also on plane 1 and 2 hedge is 8 so that probably is where the misaligned access is. The start address of plane 1 will be 672x512+8 in this case, which is wrong.
Comment 13 Wim Taymans 2012-08-10 15:06:09 UTC
put_pixels16_sse2() is only used on the Y plane. The only place where It could be misaligned would be in the imagesink when XSHM is disabled. Please test with the following patch:

commit 9b3849db1ccf4f6b985d8fd1eeb0913ae44bc6be
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Aug 10 16:58:47 2012 +0200

    x11: fix alignment in non-XSHM case
    
    Align the allocated memory to 16 bytes. When doing XSHM we are already aligned
    to a page boundary but without, we use plain g_malloc, which could allocate
    aligned on 8 bytes only.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=680796
Comment 14 Arnaud Vrac 2012-08-10 15:22:09 UTC
This patch won't help since I'm using a custom sink for my platform. The buffers that make libav crash in put_pixels16_sse2 are allocated by the video pool from the videoconvert element.

I will provide more info as soon as I can.
Comment 15 Arnaud Vrac 2012-08-13 12:34:18 UTC
This patch seems to fix the problem, I haven't been able to reproduce the crash since I applied it:

commit 4b62101f44843643a4b371c35e699fa06288de7d
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Fri Aug 10 17:03:26 2012

    viddec: use the right pointers
    
    Use the plane pointers and strides.
    Improve some debug
Comment 16 Arnaud Vrac 2012-08-13 15:58:52 UTC
Sorry I was wrong, this commit does not help. I've investigated and found out that the misaligned frame buffer is allocated by _default_mem_copy, which apparently does not respect alignment of the source memory.

I am still trying to find out why the buffer is copied in the first place. The pipeline is allocated by playbin but the relevant part is the following:

avdev_mpeg4 ! streamsynchronizer ! queue ! videoconvert

The buffer allocated from the videoconvert buffer pool is already locked when libav tries to map the frame, and I don't understand how it is possible.

The stack trace to the _default_mem_copy call is the following:

  • #0 _default_mem_copy
    at gstallocator.c line 458
  • #1 gst_memory_copy
    at gstmemory.c line 331
  • #2 gst_memory_make_mapped
    at gstmemory.c line 210
  • #3 gst_buffer_map_range
    at gstbuffer.c line 1348
  • #4 default_map
    at gstvideometa.c line 133
  • #5 gst_video_meta_map
    at gstvideometa.c line 269
  • #6 gst_video_frame_map_id
    at video-frame.c line 80
  • #7 gst_video_frame_map
    at video-frame.c line 165
  • #8 gst_ffmpegviddec_get_buffer
    from /srv/nfs/fbx6hd_master/usr/lib/gstreamer-1.0/libgstlibav.so
  • #9 ff_thread_get_buffer
    at libavcodec/pthread.c line 916
  • #10 alloc_frame_buffer
    at libavcodec/mpegvideo.c line 262
  • #11 ff_alloc_picture
    at libavcodec/mpegvideo.c line 314
  • #12 MPV_frame_start
    at libavcodec/mpegvideo.c line 1221
  • #13 ff_h263_decode_frame
    at libavcodec/h263dec.c line 628
  • #14 avcodec_decode_video2
    at libavcodec/utils.c line 1152
  • #15 gst_ffmpegviddec_video_frame
    from /srv/nfs/fbx6hd_master/usr/lib/gstreamer-1.0/libgstlibav.so
  • #16 gst_ffmpegviddec_frame
    from /srv/nfs/fbx6hd_master/usr/lib/gstreamer-1.0/libgstlibav.so
  • #17 gst_ffmpegviddec_handle_frame
    from /srv/nfs/fbx6hd_master/usr/lib/gstreamer-1.0/libgstlibav.so
  • #18 gst_video_decoder_decode_frame
    at gstvideodecoder.c line 2513
  • #19 gst_video_decoder_chain_forward
    at gstvideodecoder.c line 1642
  • #20 gst_video_decoder_chain
    at gstvideodecoder.c line 1898
  • #21 gst_pad_chain_data_unchecked
    at gstpad.c line 3587
  • #22 gst_pad_push_data
    at gstpad.c line 3800
  • #23 gst_single_queue_push_one
    at gstmultiqueue.c line 1057
  • #24 gst_multi_queue_loop
    at gstmultiqueue.c line 1303
  • #25 gst_task_func
    at gsttask.c line 316
  • #26 default_func
    at gsttaskpool.c line 70
  • #27 g_thread_pool_thread_proxy
    from /srv/nfs/fbx6hd_master/lib/libglib-2.0.so.0
  • #28 g_thread_proxy
    from /srv/nfs/fbx6hd_master/lib/libglib-2.0.so.0
  • #29 start_thread
    from /srv/nfs/fbx6hd_master/lib/libpthread.so.0
  • #30 clone
    from /srv/nfs/fbx6hd_master/lib/libc.so.6

Do you have any idea why the frame buffer is being copied ? Thanks
Comment 17 Wim Taymans 2012-08-20 09:26:16 UTC
Some more commits:

commit f56e1222dae16efe0ee6f60c86d18d117c0df28c
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Mon Aug 20 10:16:59 2012 +0200

    videopool: improve alignment
    
    Check the alignment of the strides in gst_video_info_align and increase the
    padding on the frame until the strides are aligned.
Comment 18 Wim Taymans 2012-08-20 09:36:20 UTC
commit 0e4d2814fdc8b4370749af107a08c25466427440
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Mon Aug 20 11:31:51 2012 +0200

    allocator: make a copy with the same alignment
    
    When making a copy of the memory allocated from the default memory allocator,
    make sure the new copy has the same alignment as the original memory.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=680796
Comment 19 Wim Taymans 2012-08-20 09:37:37 UTC
I think the copy is made in streamsynchronizer when it tries to make the buffer writable to unset the DISCONT flag.
Comment 20 Sebastian Dröge (slomo) 2012-08-20 10:11:43 UTC
(In reply to comment #19)
> I think the copy is made in streamsynchronizer when it tries to make the buffer
> writable to unset the DISCONT flag.

This (gst_buffer_copy()) shouldn't copy the GstMemory though if I'm not mistaken.
Comment 21 Wim Taymans 2012-08-20 10:18:42 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > I think the copy is made in streamsynchronizer when it tries to make the buffer
> > writable to unset the DISCONT flag.
> 
> This (gst_buffer_copy()) shouldn't copy the GstMemory though if I'm not
> mistaken.

The memory is marked as NO_SHARE and is thus copied. We should likely put the X info together with the GstMemory instead of setting it as buffer metadata to fix this.
Comment 22 Arnaud Vrac 2012-08-20 12:12:09 UTC
Aren't the buffers allocated by the videoconvert element ? I don't know if X image sink supports I420.
Comment 23 Arnaud Vrac 2012-08-20 17:25:40 UTC
At least the crash does not happen anymore now.
Comment 24 Tim-Philipp Müller 2012-09-11 12:47:08 UTC
What's up with this? Is anything missing?
Comment 25 Arnaud Vrac 2012-09-11 12:55:06 UTC
It works fine for me now, thanks.