Looks good here with xvimagesink and vaapisink, so I would suspect a bug in vaapisink. Can you get a backtrace of the assertion to see where it comes from? Maybe something broken with the GstVideoAlignment handling

** (gst-launch-1.0:26342): CRITICAL **: gst_video_frame_copy: assertion 'dinfo->width == sinfo->width && dinfo->height == sinfo->height' failed

Program received signal SIGTRAP, Trace/breakpoint trap.

+ Trace 235749

$1 = {info = {finfo = 0x7ffff5686328 <formats+488>, interlace_mode = GST_VIDEO_INTERLACE_MODE_PROGRESSIVE, flags = GST_VIDEO_FLAG_NONE, 
    width = 1920, height = 1080, size = 3110400, views = 1, chroma_site = GST_VIDEO_CHROMA_SITE_H_COSITED, colorimetry = {
      range = GST_VIDEO_COLOR_RANGE_16_235, matrix = GST_VIDEO_COLOR_MATRIX_BT709, transfer = GST_VIDEO_TRANSFER_BT709, 
      primaries = GST_VIDEO_COLOR_PRIMARIES_BT709}, par_n = 1, par_d = 1, fps_n = 19200, fps_d = 1, offset = {0, 2088960, 2611200, 0}, stride = {
      1920, 960, 960, 0}, ABI = {abi = {multiview_mode = GST_VIDEO_MULTIVIEW_MODE_NONE, multiview_flags = GST_VIDEO_MULTIVIEW_FLAGS_NONE}, 
      _gst_reserved = {0xffffffff, 0x0, 0x0, 0x0}}}, flags = GST_VIDEO_FRAME_FLAG_NONE, buffer = 0x7fffe40037e0, meta = 0x7fffe4008818, id = 0, 
  data = {0x7fffe8489000, 0x7fffe8687000, 0x7fffe8706800, 0x8422c4}, map = {{memory = 0x1000, flags = GST_MAP_WRITE, 
      data = 0x7fff00000001 <error: Cannot access memory at address 0x7fff00000001>, size = 2, maxsize = 140737182964929, user_data = {
        0x7fffec626210, 0xffffffffffffffff, 0x0, 0x83e0e0}, _gst_reserved = {0x9f00000002, 0x100000004, 0x4, 0x60ff20}}, {memory = 0x0, 
      flags = GST_MAP_WRITE, data = 0x60e2d0 "\320G\210\367\377\177", size = 140737346268367, maxsize = 140737159258840, user_data = {
        0x7ffff6d3ca89 <write+57>, 0x0, 0x649ae0, 0x7fffe4002080}, _gst_reserved = {0x649ae0, 0x7fffe4002080, 0x7ffff7afb275 <gst_bus_post+933>, 
        0x0}}, {memory = 0x0, flags = GST_MAP_WRITE, 
      data = 0x7ffff787eccf <g_object_unref+95> "\215U\377\211\301\211\350\360\017\261\023u\344\203\375\002\017\205\r\001", size = 6593248, 
      maxsize = 1, user_data = {0x649ae0, 0x7fffe4002080, 0x83e0f8, 0x7ffff7b0f652 <gst_element_post_message_default+162>}, _gst_reserved = {0x2, 
        0x7fffe4002080, 0x7fffe4002080, 0x83e0e0}}, {memory = 0x1, flags = (GST_MAP_WRITE | unknown: 4155438156), data = 0x83e0e0 "", 
      size = 140737348968813, maxsize = 8642784, user_data = {0x7ffff7dda3d0 <_gst_debug_min>, 0x7fffe4002080, 
        0x7ffff7aef769 <gst_bin_handle_message_func+281>, 0x3}, _gst_reserved = {0x7ffff7b490a4 <gst_segment_to_running_time+228>, 
        0x7ffff7ddb420 <_priv_gst_quark_table>, 0x1fca055, 0x3}}}, _gst_reserved = {0x7ffff5d79429 <gst_base_sink_get_sync_times+1177>, 0x0, 
    0x1fca055, 0x1fca055}}
(gdb) print src_frame
$2 = {info = {finfo = 0x7ffff5686328 <formats+488>, interlace_mode = GST_VIDEO_INTERLACE_MODE_PROGRESSIVE, flags = GST_VIDEO_FLAG_NONE, 
    width = 1920, height = 1088, size = 3110400, views = 1, chroma_site = GST_VIDEO_CHROMA_SITE_H_COSITED, colorimetry = {
      range = GST_VIDEO_COLOR_RANGE_16_235, matrix = GST_VIDEO_COLOR_MATRIX_BT709, transfer = GST_VIDEO_TRANSFER_BT709, 
      primaries = GST_VIDEO_COLOR_PRIMARIES_BT709}, par_n = 1, par_d = 1, fps_n = 19200, fps_d = 1, offset = {32832, 2310176, 2885664, 0}, 
    stride = {2048, 1024, 1024, 0}, ABI = {abi = {multiview_mode = GST_VIDEO_MULTIVIEW_MODE_NONE, 
        multiview_flags = GST_VIDEO_MULTIVIEW_FLAGS_NONE}, _gst_reserved = {0xffffffff, 0x0, 0x0, 0x0}}}, flags = GST_VIDEO_FRAME_FLAG_NONE, 
  buffer = 0x7fffe4003070, meta = 0x7ea5f8, id = 0, data = {0x7fffe92de0e0, 0x7fffe950a0c0, 0x7fffe95968c0, 0x7fffec6262d8}, map = {{
      memory = 0x7fffe92d6010, flags = GST_MAP_READ, data = 0x7fffe92d60a0 "", size = 3452928, maxsize = 3452959, user_data = {0x7fffec6260c0, 
        0x7ffff787a839 <g_cclosure_marshal_generic+505>, 0x7fffec626298, 0x7fffec6262b0}, _gst_reserved = {0x7fffec6262c8, 0x6334a0, 0x1, 
        0x7fffe4000020}}, {memory = 0x7fffe92d6010, flags = GST_MAP_READ, data = 0x7fffe92d60a0 "", size = 3452928, maxsize = 3452959, user_data = {
        0x40, 0x0, 0xffffffff00000030, 0x0}, _gst_reserved = {0x10000005b, 0x0, 0x0, 0x770000006e}}, {memory = 0x7fffe92d6010, 
      flags = GST_MAP_READ, data = 0x7fffe92d60a0 "", size = 3452928, maxsize = 3452959, user_data = {0x633490, 0x7fffec626290, 0x4e401f9f0, 
        0x7fffec625f60}, _gst_reserved = {0x7fffec625f90, 0x7fffec626054, 0xe401f9f0, 0x7fffec625fc0}}, {memory = 0x400000001, 
      flags = (unknown: 3965869968), data = 0x7ffff64813a0 <ffi_type_void> "\001", size = 0, maxsize = 144, user_data = {0x869873a0d6ad9f00, 
        0x8432e0, 0x633490, 0x0}, _gst_reserved = {0x3, 0x7fffec626290, 0x7fffec626210, 0x0}}}, _gst_reserved = {
    0x7ffff787a015 <g_closure_invoke+325>, 0x7fffec6260f0, 0x0, 0x2}}
(gdb)

Isn't this the same thing we saw on FreeBSD ?

OpenBSD, and wasn't there the problem that allocation just failed (from xvimagesink)?

Ok, if I remember there was a good indication in the trace. So if we could have a trace of that failing run, with GST_DEBUG="libav:7" we'll be able to sort out if this is a new bug, or that same. Was there some patches on xvimagesink to fix the issue ?

Why don't you just look it up in Bugzilla? ;)

There was no fix and the bug is still open, but what was fixed (by you) is that we don't go into the fallback allocation (which doesn't work for us) in gst-libav anymore but just fail.


But a new debug log would be helpful, yes. The problem is different though than the OpenBSD one: there the allocation in xvimagesink failed, here it seems like it succeeds?

(In reply to Sebastian Dröge (slomo) from comment #6)
> Why don't you just look it up in Bugzilla? ;)

I would have notice it was in bugzilla, but you told me once there was a fix coming, so I assumed you where taking care.

> 
> There was no fix and the bug is still open, but what was fixed (by you) is
> that we don't go into the fallback allocation (which doesn't work for us) in
> gst-libav anymore but just fail.

And I still think this is the right thing to do, we can't make mandatory that all element to have fallback in case a pool randomly stop working, so better not have a fallback and make sure our pool can keep working.

> 
> But a new debug log would be helpful, yes. The problem is different though
> than the OpenBSD one: there the allocation in xvimagesink failed, here it
> seems like it succeeds?

I don't know, that's why I'd like the trace (or a clue on how to reproduce).

The OpenBSD bug is bug #756029 btw. And yes, the gst-libav fix you did is correct :)

The OpenBSD bug is something I can't fix, I don't run OpenBSD. Someone running that will have to debug it further first.

Ok, I just tested this one, here's my test results:

OK: gst-play-1.0 test.mov --videosink="imagefreeze ! xvimagesink"
OK: gst-play-1.0 test.mov --videosink="imagefreeze ! glimagesink"
KO: gst-play-1.0 test.mov --videosink="imagefreeze ! vaapisink"

The difference is that vaapisink is the only one that does not offer a pool (hence the copy). Now, interesting fact, avviddec does not set the video_meta, it relies on the pool to do so and the pool in this case is GstVideoBufferPool. Moving to gst-plugins-base (we can forget the discussion we had ;-P).

Ok, back to libav. The width/height is taking form the pool config caps, which is 1920x1088.

Interesting, vaapi propose a pool, and pretend to support the GstVideoAlignment, while in fact it completely ignores it ;-P Anyway, that's why the pool get rejected. But later, it would seem we endup pushing from the internal pool, which requires fixing the GstVideoMeta before pushing (as we don't know the appropriate width/height when we create the internal pool). This is a small oops, I might not have tested that particular case.

Yeah, changing the GstVideoMeta chosen by the pool would be a hack. Maybe we should just create a new GstVideoBufferPool with the known width/height. Set the direct rendering alignment, and let it copy until the transition from internal to external pool is done ? (this way we don't have two code path)

Created attachment 316331 [details] [review]
avviddec: Fix VideoMeta from internal pool

Our internal pool might have been configured early, before we know
the actual width and height. There strides and offsets remains valid,
so simply update the width and height before pushing.

This is the simpliest fix I could find, works nicely here. I'll report the vaapisink alignment bug.

Created attachment 316392 [details]
new test video

I've tested the patch. Now playback starts, but I get this error multiple times, when I play the new (longer) video file:

 ** (gst-launch-1.0:1688): CRITICAL **: gst_video_frame_map_id: assertion 'info->height <= meta->height' failed

I started gst-launch in gdb with G_DEBUG=fatal_criticals and I get this:

(gdb) bt

+ Trace 235768

$1 = {finfo = 0x7ffff5687328 <formats+488>, interlace_mode = GST_VIDEO_INTERLACE_MODE_PROGRESSIVE, flags = GST_VIDEO_FLAG_NONE, width = 1920, 
  height = 1088, size = 3133440, views = 1, chroma_site = GST_VIDEO_CHROMA_SITE_H_COSITED, colorimetry = {range = GST_VIDEO_COLOR_RANGE_16_235, 
    matrix = GST_VIDEO_COLOR_MATRIX_BT709, transfer = GST_VIDEO_TRANSFER_BT709, primaries = GST_VIDEO_COLOR_PRIMARIES_BT709}, par_n = 1, par_d = 1, 
  fps_n = 0, fps_d = 1, offset = {0, 2088960, 2611200, 0}, stride = {1920, 960, 960, 0}, ABI = {abi = {
      multiview_mode = GST_VIDEO_MULTIVIEW_MODE_NONE, multiview_flags = GST_VIDEO_MULTIVIEW_FLAGS_NONE}, _gst_reserved = {0xffffffff, 0x0, 0x0, 
      0x0}}}
(gdb) print *meta
$2 = {meta = {flags = (GST_META_FLAG_POOLED | GST_META_FLAG_LOCKED), info = 0x7fffe40051e0}, buffer = 0x7fffe4003070, 
  flags = GST_VIDEO_FRAME_FLAG_NONE, format = GST_VIDEO_FORMAT_I420, id = 0, width = 1920, height = 1080, n_planes = 3, offset = {32832, 2310176, 
    2885664, 0}, stride = {2048, 1024, 1024, 0}, map = 0x7ffff544ffd0 <default_map>, unmap = 0x7ffff544ffb0 <default_unmap>}
(gdb)

That could be when the buffer is being reused later. Can you provide a stream that reproduce this please ?

(never mind, just notice)

The same bug also happens with d3dvideosink, and there it complains also about "The memory allocated from downstream pool could not be mapped for read and write".

And I mean this happens with the patch. Without the patch it just crashes. Adding a tee in front of d3dvideosink makes it work as usual with these kind of problems.

Oh and d3dvideosink does not provide a pool but proposes the VIDEO and VIDEO_CROP meta in the allocation query.

Can be reproduced with bbb_sunflower_480p_30fps_normal.mov and xvimagesink after patching xvimagesink to not provide a pool (but to propose the metas).

Yeah, I know this already. It applies to sink with VideoMeta where the downstream pool is unusable. I've been too quick on the first patch, and I forgot about the side effect of just changing the video meta (internal viddec map are then done with the wrong size, so it simply moves the assertion into the decoder). Note it's an assertion, an induced crash as this that particular case, ignoring those value would work.

Created attachment 316545 [details] [review]
avviddec: Make sure to use a buffer pool with the correct width/height configured on it for pushing buffers downstream

If downstream does not provide a (usable) pool, we would use our internal
pool. But the internal pool might be configured with a different width/height
because of padding, which then will cause problems if we push buffers from it
directly downstream.
Instead create a new pool if the width/height is different.

This prevents crashes with vaapisink and d3dvideosink for example.

Created attachment 316546 [details] [review]
avviddec: Make sure to use a buffer pool with the correct width/height configured on it for pushing buffers downstream

If downstream does not provide a (usable) pool, we would use our internal
pool. But the internal pool might be configured with a different width/height
because of padding, which then will cause problems if we push buffers from it
directly downstream.
Instead create a new pool if the width/height is different.

This prevents crashes with vaapisink and d3dvideosink for example.

Based on the debugging results and discussions with
Nicolas Dufresne <nicolas.dufresne@collabora.com>

Review of attachment 316546 [details] [review]:

::: ext/libav/gstavviddec.c
@@ +1420,3 @@
+  else {
+    /* Our pool might have been configured with preliminatry width and height,
+     * so fix it */

This comment can be removed ;-P

@@ +1901,3 @@
+  if (have_videometa && ffmpegdec->internal_pool
+      && ffmpegdec->pool_width == state->info.width
+      && ffmpegdec->pool_height == state->info.height) {

Good.

The new patch looks good to me. It fixes the problem with all my test-cases.

Attachment 316546 [details] pushed as 5df8cc5 - avviddec: Make sure to use a buffer pool with the correct width/height configured on it for pushing buffers downstream

Thanks for testing Michael. I'll backport this to 1.6 together with some other things in a bit.