Reproduced on Fedora 27 with gstreamer1-plugins-base-1.12.4-1.fc27.x86_64

0:00:00.745133977 14704       0x80e2d0 DEBUG                basesrc gstbasesrc.c:3146:gst_base_src_prepare_allocation:<videotestsrc0> ALLOCATION (1) params: allocation query: 0x7fffec003190, GstQueryAllocation, caps=(GstCaps)"video/x-raw\,\ format\=\(string\)YV12\,\ width\=\(int\)905\,\ height\=\(int\)720\,\ framerate\=\(fraction\)30/1\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ interlace-mode\=\(string\)progressive", need-pool=(boolean)true, pool=(GArray)NULL, meta=(GArray)NULL, allocator=(GArray)NULL;
0:00:00.745187131 14704       0x80e2d0 DEBUG                basesrc gstbasesrc.c:2971:gst_base_src_set_allocation:<videotestsrc0> activate pool
0:00:00.745194672 14704       0x80e2d0 LOG               bufferpool gstbufferpool.c:473:gst_buffer_pool_set_active:<xvimagebufferpool1> active 1
0:00:00.745202290 14704       0x80e2d0 LOG               bufferpool gstbufferpool.c:349:do_start:<xvimagebufferpool1> starting
0:00:00.745209179 14704       0x80e2d0 LOG               GST_BUFFER gstbuffer.c:727:gst_buffer_new: new 0x80f680
0:00:00.745221075 14704       0x80e2d0 DEBUG       xvimageallocator xvimageallocator.c:371:gst_xvimage_allocator_alloc:<xvimageallocator0> creating image 0x7fffec010000 (905x720) cropped 0x0-905x720
0:00:00.745285108 14704       0x80e2d0 LOG         xvimageallocator xvimageallocator.c:406:gst_xvimage_allocator_alloc:<xvimageallocator0> XShm image size is 979200
0:00:00.745294230 14704       0x80e2d0 DEBUG       xvimageallocator xvimageallocator.c:430:gst_xvimage_allocator_alloc:<xvimageallocator0> Plane 0 has a expected pitch of 908 bytes, offset of 0
0:00:00.745303200 14704       0x80e2d0 DEBUG       xvimageallocator xvimageallocator.c:430:gst_xvimage_allocator_alloc:<xvimageallocator0> Plane 1 has a expected pitch of 454 bytes, offset of 653760
0:00:00.745311829 14704       0x80e2d0 DEBUG       xvimageallocator xvimageallocator.c:430:gst_xvimage_allocator_alloc:<xvimageallocator0> Plane 2 has a expected pitch of 454 bytes, offset of 817200
error size

Thread 3 "videotestsrc0:s" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff37f4700 (LWP 14709)]
gst_xvimage_allocator_alloc (allocator=0x63e350 [GstXvImageAllocator], im_format=<optimised out>, info=info@entry=0x7fffec00e370, padded_width=<optimised out>, padded_height=<optimised out>, crop=crop@entry=0x7fffec00e35c, error=0x7ffff37f3c90)
    at xvimageallocator.c:535
535	    *(int *)0 = 0;
(gdb) p mem->xvimage->data_size 
$1 = 979200
(gdb) p mem->xvimage->offsets[0]
$2 = 0
(gdb) p mem->xvimage->offsets[1]
$3 = 653760
(gdb) p mem->xvimage->offsets[2]
$4 = 816480
(gdb) p mem->xvimage->pitches[0]
$5 = 908
(gdb) p mem->xvimage->pitches[1]
$6 = 452
(gdb) p mem->xvimage->pitches[2]
$7 = 452
(gdb)

The conflicting information ultimately, in my two test cases, comes from intel_video_query_image_attributes():

https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/uxa/intel_video.c#n769

	case FOURCC_YV12:
	case FOURCC_I420:
		*h = (*h + 1) & ~1;
		size = (*w + 3) & ~3;
		if (pitches)
			pitches[0] = size;
		size *= *h;
		if (offsets)
			offsets[1] = size;
		tmp = ((*w >> 1) + 3) & ~3;
		if (pitches)
			pitches[1] = pitches[2] = tmp;
		tmp *= (*h >> 1);
		size += tmp;
		if (offsets)
			offsets[2] = size;
		size += tmp;

FWIW if I bypass the sanity check and force it to display anyway, it looks wrong. I think that the xorg-video-intel driver does have it right. Even though 452 is less than half of 905.

Created attachment 371069 [details]
905x720 output with sanity check disabled

This is what "looks wrong" looks like. It happens at width=905 and again at width=913 etc. — every width which is one greater than a multiple of 8.

I was mistaken; my test boxes are one i915, one Radeon. They both show exactly the same problem — widths of 905, 913, etc. are failing.

This appears to fix it. It doesn't make me happy.

--- a/sys/xvimage/xvimagepool.c
+++ b/sys/xvimage/xvimagepool.c
@@ -122,6 +122,16 @@ xvimage_buffer_pool_set_config (GstBufferPool * pool, GstStructure * config)
       GST_VIDEO_INFO_HEIGHT (&info) + xvpool->align.padding_top +
       xvpool->align.padding_bottom;
 
+  /*
+   * Work around the fact that all the Xv drivers seem to do weird
+   * things for widths which are one greater than a multiple of 8.
+   * https://bugzilla.gnome.org/show_bug.cgi?id=795235
+   */
+  if ((xvpool->padded_width & 7) == 1) {
+         xvpool->padded_width++;
+         xvpool->align.padding_right++;
+  }
+
   xvpool->info = info;
   xvpool->crop.x = xvpool->align.padding_left;
   xvpool->crop.y = xvpool->align.padding_top;

Actually, I think radeon_drv and intel_drv are both fine. The problem in both my test cases is glamorgl.

Fixed by https://lists.x.org/archives/xorg-devel/2018-April/056701.html

I can reproduce this on i915. I'll have a look at how we calculate the padding.

This seems like a generic issue, gst_video_set_format() will add padding implicitly to avoid burst of unaligned read/write. So as an example, in 905x720 I420 the stride will be set as:

  info->stride[0] = GST_ROUND_UP_4 (width);

So through the stride, we already add a padding_right of 3, but the allocator is not aware of that. Same thing can happen with height. What I think is that the code need a tad of refactoring, instead of trying to guess what the driver should pick, we should just allocated one buffer to read what the driver is doing. This will likely fixed many issues with our XV implementation.

Ok, I thought, hey, let's the driver choose, but I notice a Gst ends up reading too far. That's because the Intel driver math is wrong for odd width:

Bug is quite evident:
https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/uxa/intel_video.c#n814
"tmp = ((*w >> 1) + 3) & ~3;"

This will do (905 >> 1) ->452, which is a multiple of 4. But if you reverse this,
you'll get 452 * 2 == 904, so there is no UV value for the 905th Y of each lines.

The correct math should have been:

"tmp = (((*w + 1) >> 1) + 3) & ~3;"

Created attachment 371444 [details] [review]
xvimagepool: Workaround Intel XV driver bug with odd with

The code failes to round up a division by two for the U and V planes in
I420. As a side effect, there is a missing U/V value. We can workaround
this by padding to the right.

I don't like this patch, but I know there is a lot of copy-paste from Intel driver plus this is X11, which most dev are trying to move away from ... Maybe I should round up everything to 16 pixels, would probably hide the other bugs we see on random drivers ...

(In reply to Nicolas Dufresne (ndufresne) from comment #10)
> Ok, I thought, hey, let's the driver choose, but I notice a Gst ends up
> reading too far. That's because the Intel driver math is wrong for odd width:
> 
> Bug is quite evident:
> https://cgit.freedesktop.org/xorg/driver/xf86-video-intel/tree/src/uxa/
> intel_video.c#n814
> "tmp = ((*w >> 1) + 3) & ~3;"

But see #n793 a few lines above:

	*w = (*w + 1) & ~1;

I'm not sure the Intel driver *is* at fault here. Are you sure you're not using the glamor_gl version, which I already fixed (comment 7)?

Although it doesn't really matter much, except to get the blame right in your comments. Some driver(s) get it wrong and thus we should probably work around it by padding.

One would need to blame that line to see if it's included or not in Fedora shipped driver (the one I've testing against). I have no idea if it's glamor or something else, but I know it's broken as the strides values I read from X11 are not usable, pitches is [ 908, 452, 452 ] is invalid for I420 905x720.

Ok, you're right, this is glamor, found out running xvinfo:
  Adaptor #0: "glamor textured video"

Is that the right code ?
https://cgit.freedesktop.org/xorg/xserver/tree/glamor/glamor_xv.c#n202

David, you're patch won't be sufficient, see glamor_xv_put_image line 459:

  srcPitch2 = ALIGN(width >> 1, 4);

This function should call glamor_xv_query_image_attributes() instead of redoing the math. Anyway, closing as there this is not GStramer.

Review of attachment 371444 [details] [review]:

.

Hm, interesting observation. Nevertheless it *is* working with that patch. Perhaps because my patch doesn't only affect the calculation within the  glamor_xv_query_image_attributes() function — the height and width are passed in by reference each as 'int *' and they are rounded up to a multiple of two.

So I suspect the original odd width value is never going to reach glamor_xv_query_image_attributes() and that's why it's actually working?

That would require tracing the value that reaches, but yeah, I only got a narrow view of all this, the value could get rounded up somewhere up the stack. At the same time, there all source of problem with cropping resulting in green lines leaking in, so this driver needs some maintenance in general.

On GStreamer side, the code is pretty much what was done in 0.10 prior to having GstVideoMeta, so it's not ideal. It could fail easily if the drivers didn't always ended up with the same pitches. Though, this is XV which is getting low maintenance. So I don't really see when / if I find the time to rewrite this properly to query the driver in the pool set_config() function.