It's crashing in libav: av_lzo1x_decode()

Can you please make the file that caused the crash available?

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.

Created attachment 237168 [details]
track of totem in gdb with the crashing file pasted into it

the crashing file works ok in VLC, but make crash with same error Banshee

Also crashes with gst-launch-1.0. Looks like a libav bug.

+ Trace 231545

Does not work with git master + internal libav 9.1 either.

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.

Did anybody reproduce this with avplay or any other software using libav? Also did anybody forward this to the libav bugtracker?

The video I posted above indeed crashes with avplay (0.8.5). I'll try with 0.9

Crashes with latest git master (using libav 9.4) but doesn't crash with avplay from the Debian 9.4 packages.

*** Bug 700965 has been marked as a duplicate of this bug. ***

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*)

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

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.

Unfortunately this makes other codecs crash...

Comment on attachment 249433 [details] [review]
avviddec: match avplay to compute plane offsets for picture

seems to need work accodring to comment #15.

Where is the relevant code for that in libav?

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.

Created attachment 249476 [details] [review]
print output frame info in libav

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 :)

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

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

Are you planning to write a patch for calculating the UV plane offsets correctly?

Yes I'm working on it

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.

What exactly does libav require there? Some padding after each plane or each plane start aligned?

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.

Yes, let's just do that then

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.

Created attachment 249515 [details] [review]
avviddec: increase bottom padding for output frames

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 ?

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