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 694299 - crash in put_pixels16_sse2() with SVQ1 video
crash in put_pixels16_sse2() with SVQ1 video
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-libav
unspecified
Other Linux
: Normal normal
: 1.1.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 700965 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-02-20 18:42 UTC by Fabio
Modified: 2013-07-18 14:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
track of totem in gdb with the crashing file pasted into it (52.38 KB, text/plain)
2013-02-22 12:18 UTC, Fabio
  Details
avviddec: match avplay to compute plane offsets for picture (1.18 KB, patch)
2013-07-17 18:59 UTC, Arnaud Vrac
needs-work Details | Review
print output frame info in libav (1.75 KB, patch)
2013-07-18 09:11 UTC, Arnaud Vrac
rejected Details | Review
video: respect stride alignment when calculating planes offsets (1.94 KB, patch)
2013-07-18 13:21 UTC, Arnaud Vrac
committed Details | Review
avviddec: increase bottom padding for output frames (1.19 KB, patch)
2013-07-18 14:17 UTC, Arnaud Vrac
committed Details | Review

Comment 1 Bastien Nocera 2013-02-20 23:50:29 UTC
It's crashing in libav: av_lzo1x_decode()

Can you please make the file that caused the crash available?
Comment 2 leagris 2013-02-21 22:22:59 UTC
I am the original submitter @launchpad/Ubuntu

Here: http://www.noiraude.net/bug/levent_t1.mov
Is the file that was processed (according to the ThreadStackTrace.txt) during the crash (as you requested):

Regards.
Comment 3 Fabio 2013-02-22 12:18:23 UTC
Created attachment 237168 [details]
track of totem in gdb with the crashing file pasted into it
Comment 4 Fabio 2013-02-22 12:22:10 UTC
the crashing file works ok in VLC, but make crash with same error Banshee
Comment 5 Bastien Nocera 2013-02-22 12:30:52 UTC
Also crashes with gst-launch-1.0. Looks like a libav bug.

  • #0 nanosleep
    from /lib64/libpthread.so.0
  • #1 g_usleep
    at gtimer.c line 261
  • #2 fault_spin
    at gst-launch.c line 149
  • #3 fault_handler_sigaction
    at gst-launch.c line 129
  • #4 <signal handler called>
  • #5 put_pixels16_sse2
    at libavcodec/x86/dsputil_mmx.c line 455
  • #6 svq1_motion_inter_block
    at libavcodec/svq1dec.c line 384
  • #7 svq1_decode_delta_block
    at libavcodec/svq1dec.c line 498
  • #8 svq1_decode_frame
    at libavcodec/svq1dec.c line 721
  • #9 avcodec_decode_video2
    at libavcodec/utils.c line 1152
  • #10 gst_ffmpegviddec_video_frame
    at gstavviddec.c line 1135
  • #11 gst_ffmpegviddec_frame
    at gstavviddec.c line 1262
  • #12 gst_ffmpegviddec_handle_frame
    at gstavviddec.c line 1379
  • #13 gst_video_decoder_decode_frame
    at gstvideodecoder.c line 2666
  • #14 gst_video_decoder_chain_forward
    at gstvideodecoder.c line 1698
  • #15 gst_video_decoder_chain
    at gstvideodecoder.c line 1958
  • #5 gst_pad_push
    at gstpad.c line 3974
  • #6 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #7 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #8 gst_pad_push_data
    at gstpad.c line 3871
  • #9 gst_pad_push
    at gstpad.c line 3974
  • #10 gst_stream_synchronizer_sink_chain
    at gststreamsynchronizer.c line 547
  • #11 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #12 gst_pad_push_data
    at gstpad.c line 3871
  • #13 gst_pad_push
    at gstpad.c line 3974
  • #14 gst_tee_handle_data
    at gsttee.c line 601
  • #15 gst_tee_chain
    at gsttee.c line 699
  • #16 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #17 gst_pad_push_data
    at gstpad.c line 3871
  • #18 gst_pad_push
    at gstpad.c line 3974
  • #19 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #20 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #21 gst_pad_push_data
    at gstpad.c line 3871
  • #22 gst_pad_push
    at gstpad.c line 3974
  • #23 gst_selector_pad_chain
    at gstinputselector.c line 1047
  • #24 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #25 gst_pad_push_data
    at gstpad.c line 3871
  • #26 gst_pad_push
    at gstpad.c line 3974
  • #27 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #28 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #29 gst_pad_push_data
    at gstpad.c line 3871
  • #30 gst_pad_push
    at gstpad.c line 3974
  • #31 gst_proxy_pad_chain_default
    at gstghostpad.c line 128
  • #32 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #33 gst_pad_push_data
    at gstpad.c line 3871
  • #34 gst_pad_push
    at gstpad.c line 3974
  • #35 gst_ffmpegauddec_frame
  • #36 gst_ffmpegauddec_chain
    at gstavdec.c line 1220
  • #37 gst_pad_chain_data_unchecked
    at gstpad.c line 3654
  • #38 gst_pad_push_data
    at gstpad.c line 3871
  • #39 gst_pad_push
    at gstpad.c line 3974
  • #40 gst_single_queue_push_one
    at gstmultiqueue.c line 1057
  • #41 gst_multi_queue_loop
    at gstmultiqueue.c line 1303
  • #42 gst_task_func
    at gsttask.c line 316
  • #43 default_func
    at gsttaskpool.c line 70
  • #44 g_thread_pool_thread_proxy
    at gthreadpool.c line 309
  • #45 g_thread_proxy
    at gthread.c line 798
  • #46 start_thread
    from /lib64/libpthread.so.0
  • #47 clone
    from /lib64/libc.so.6

Comment 6 Tim-Philipp Müller 2013-02-22 13:46:34 UTC
Does not work with git master + internal libav 9.1 either.
Comment 7 Arnaud Vrac 2013-03-14 15:44:54 UTC
I have the same problem for this video: http://absolut.zogzog.org/share/samples/mov/Stereophonics%20-%20Lying.mov

It crashes in put_pixels16_sse2 with a general protection ip error, so it probably is an alignment bug.
Comment 8 Sebastian Dröge (slomo) 2013-03-21 14:50:41 UTC
Did anybody reproduce this with avplay or any other software using libav? Also did anybody forward this to the libav bugtracker?
Comment 9 Arnaud Vrac 2013-03-21 14:58:49 UTC
The video I posted above indeed crashes with avplay (0.8.5). I'll try with 0.9
Comment 10 Sebastian Dröge (slomo) 2013-03-25 09:03:39 UTC
Crashes with latest git master (using libav 9.4) but doesn't crash with avplay from the Debian 9.4 packages.
Comment 11 Tim-Philipp Müller 2013-05-24 18:05:23 UTC
*** Bug 700965 has been marked as a duplicate of this bug. ***
Comment 12 Edward Hervey 2013-07-17 12:54:54 UTC
This does not seem to be libav's fault:

This works fine:
gst-launch-1.0 file:///media/data/contents/levent_t1.mov ! qtdemux ! avdec_svq1 ! fakesink

avdec_svq1-0.GstPad:src: caps = video/x-raw, format=(string)YUV9, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)24/1


This segfaults
gst-launch-1.0 file:///media/data/contents/levent_t1.mov ! qtdemux ! avdec_svq1 ! videoconvert ! xvimagesink

avdec_svq1-0.GstPad:src: caps = video/x-raw, format=(string)YUV9, width=(int)320, height=(int)240, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive, colorimetry=(string)bt601, framerate=(fraction)24/1


(obviously running it in valgrind doesn't reproduce the issue .. .*sigh*)
Comment 13 Arnaud Vrac 2013-07-17 17:21:53 UTC
This seems to be a bug in the direct rendering path.

Works:
gst-launch file:///media/data/contents/levent_t1.mov ! qtdemux ! avdec_svq1 direct-rendering=false ! videoconvert ! 'video/x-raw, format=NV12' ! fakesink

Crashes:
gst-launch file:///media/data/contents/levent_t1.mov ! qtdemux ! avdec_svq1 direct-rendering=true ! videoconvert ! 'video/x-raw, format=NV12' ! fakesink
Comment 14 Arnaud Vrac 2013-07-17 18:59:21 UTC
Created attachment 249433 [details] [review]
avviddec: match avplay to compute plane offsets for picture

This patch fixes the issue for me. The new padding values make the video frame info match avplay exactly (plane offsets and sizes).

I'm not sure if's libav or videoconvert that does not cope with the non-0 left and top padding values properly.
Comment 15 Arnaud Vrac 2013-07-17 19:01:26 UTC
Unfortunately this makes other codecs crash...
Comment 16 Tim-Philipp Müller 2013-07-17 20:10:23 UTC
Comment on attachment 249433 [details] [review]
avviddec: match avplay to compute plane offsets for picture

seems to need work accodring to comment #15.
Comment 17 Sebastian Dröge (slomo) 2013-07-18 08:55:52 UTC
Where is the relevant code for that in libav?
Comment 18 Arnaud Vrac 2013-07-18 09:07:56 UTC
Libav allocates output buffer in update_frame_pool in libavcodec/utils.c. I've attached a small patch that prints out the plane offsets and sizes for reference.
Comment 19 Arnaud Vrac 2013-07-18 09:11:08 UTC
Created attachment 249476 [details] [review]
print output frame info in libav
Comment 20 Sebastian Dröge (slomo) 2013-07-18 09:42:27 UTC
Ok, so in any case it's probably a good idea to completely replicate that code (after your patch that's not the case yet, right?). If it still crashes then, we'll have to understand why :)
Comment 21 Arnaud Vrac 2013-07-18 10:27:39 UTC
The new offsets do match the libav values, so I'm not sure why some other codecs crash now.

Here are some examples:

* h264 yuv420p, 848x480 video:

libav:
frame 848x480
aligned 848x482 stride align 16-16-16
with edge 880x514
strides for width 880: 880-440-440
strides for width 896: 896-448-448
data[0] = 0         size=460544
data[1] = 460544    size=115136
data[2] = 575680    size=115136

gstlibav:
<avdec_h264-0> aligned dimension 848x480 -> 880x514 padding t:0 l:0 r:32 b:34, stride_align 15:15:15:15
gst_buffer_add_video_meta_full: plane 0, offset 0, stride 896
gst_buffer_add_video_meta_full: plane 1, offset 460544, stride 448
gst_buffer_add_video_meta_full: plane 2, offset 575680, stride 448

* mpeg4, yuv420p, 640x480 video:

libav:
frame 640x480
aligned 640x480 stride align 16-16-16
with edge 672x512
strides for width 672: 672-336-336
data[0] = 0         size=344064
data[1] = 344064    size=86016
data[2] = 430080    size=86016

gstlibav:
<avdec_mpeg4-0> aligned dimension 640x480 -> 672x512 padding t:0 l:0 r:32 b:32, stride_align 15:15:15:15
gst_buffer_add_video_meta_full: plane 0, offset 0, stride 672
gst_buffer_add_video_meta_full: plane 1, offset 344064, stride 336
gst_buffer_add_video_meta_full: plane 2, offset 430080, stride 336

* svq1, yuv410p, 320x240 video:

libav:
frame 320x240
aligned 320x256 stride align 16-16-16
with edge 352x288
strides for width 352: 352-88-88
strides for width 384: 384-96-96
data[0] = 0         size=110592
data[1] = 110592    size=6912
data[2] = 117504    size=6912

gstlibav:
<avdec_svq1-0> aligned dimension 320x240 -> 352x288 padding t:0 l:0 r:32 b:48, stride_align 15:15:15:15
gst_buffer_add_video_meta_full: plane 0, offset 0, stride 384
gst_buffer_add_video_meta_full: plane 1, offset 110592, stride 96
gst_buffer_add_video_meta_full: plane 2, offset 117504, stride 96
Comment 22 Arnaud Vrac 2013-07-18 11:21:25 UTC
After some more investigation it turns out the gstlibav code is right, libav does need padding on top and left of the planes. The padding offset are computed in video_get_buffer in libavcodec/utils.c, and in the crash is due to the fact that the UV plane offsets are not computed correctly in gst_video_info_align. The offsets are not aligned to the stride_align values, which is why the SIMD instructions crash with a general protection error.

For the levent_1.mov video, libav uses the following offsets for each plane:

offset[0] = 6160
offset[1] = 400
offset[2] = 400

Whereas gstreamer uses the following values:

plane 0: comp: 0, hedge 16 vedge 16 align 15 stride 384 offset 6160
plane 1: comp: 1, hedge 4 vedge 4 align 15 stride 96 offset 388
plane 2: comp: 2, hedge 4 vedge 4 align 15 stride 96 offset 388
Comment 23 Sebastian Dröge (slomo) 2013-07-18 12:19:38 UTC
Are you planning to write a patch for calculating the UV plane offsets correctly?
Comment 24 Arnaud Vrac 2013-07-18 12:20:14 UTC
Yes I'm working on it
Comment 25 Arnaud Vrac 2013-07-18 13:21:06 UTC
Created attachment 249501 [details] [review]
video: respect stride alignment when calculating planes offsets

This patch for gst-plugins-base makes all plane start offsets aligned on the plane stride align value.

The patch by itself is right, however it makes some videos crash when decoding them with libav. This is because libav needs even more padding at the very end of each plane, in some SIMD optimized functions. See https://trac.ffmpeg.org/ticket/2645 and http://git.videolan.org/?p=ffmpeg.git;a=commit;h=175e916fa20b7887bdb29809817985e481ae0888. This is a patch from ffmpeg 2.0, even in the latest libav there is an out-of-bounds write in some cases because the padding is not big enough.

I'm not sure how to add this small padding at the end of each plane with the current API in gstreamer.
Comment 26 Sebastian Dröge (slomo) 2013-07-18 13:28:51 UTC
What exactly does libav require there? Some padding after each plane or each plane start aligned?
Comment 27 Arnaud Vrac 2013-07-18 13:39:04 UTC
Both. The plane size should have 16 + stride_align bytes padding added at the end for some SIMD optimized functions to work, from the links I've posted above.

The plane start offset must then be aligned otherwise some SIMD instructions will trigger a general protection error.

The simplest way to work around this would be to increase the align.padding_bottom value by 1 in gstavviddec.c. This would alloc one more line than necessary in each plane.
Comment 28 Sebastian Dröge (slomo) 2013-07-18 13:41:30 UTC
Yes, let's just do that then
Comment 29 Wim Taymans 2013-07-18 13:58:36 UTC
Yes, this is correct, after vertical and horizontal padding, it should add some more to align to the requested alignment, like what libav does.

for the extra-extra padding, I would indeed just add more padding at the bottom. You could theoretically also configure the extra padding in the params of the allocator that you set with gst_buffer_pool_config_set_allocator() but that seems
more trouble.
Comment 30 Arnaud Vrac 2013-07-18 14:17:31 UTC
Created attachment 249515 [details] [review]
avviddec: increase bottom padding for output frames
Comment 31 Edward Hervey 2013-07-18 14:24:20 UTC
Review of attachment 249501 [details] [review]:

Needs work

::: gst-libs/gst/video/video-info.c
@@ +722,3 @@
   gint width, height;
   gint padded_width, padded_height;
+  gint i, j, n_planes;

I can't see where j is used.

@@ +785,3 @@
         (hedge * GST_VIDEO_FORMAT_INFO_PSTRIDE (vinfo, comp));
+
+    offset = (offset + align->stride_align[i]) & ~align->stride_align[i];

you're overwriting offset again. Which one do you want ?
Comment 32 Sebastian Dröge (slomo) 2013-07-18 14:25:11 UTC
commit 28e1dadbfaa403679e69f8173d1aa2c7500fd556
Author: Arnaud Vrac <avrac@freebox.fr>
Date:   Thu Jul 18 14:13:33 2013 +0200

    video: respect stride alignment when calculating planes offsets
    
    https://bugzilla.gnome.org/show_bug.cgi?id=694299


commit 4a2054c6aad18db3c1e9bc359d4ba4de855b036d
Author: Arnaud Vrac <avrac@freebox.fr>
Date:   Thu Jul 18 16:11:16 2013 +0200

    avviddec: increase bottom padding for output frames
    
    libav can write slightly after the plane end in some SIMD optimized
    functions. The extra padding value needs to be at least 16+stride_align
    for each plane, so just increase the bottom padding value for the output
    frame.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=694299