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 790057 - waylandsink: does not respect video alignment from upstream
waylandsink: does not respect video alignment from upstream
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-11-08 12:26 UTC by Arnaud Vrac
Modified: 2018-06-12 05:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
waylandsink: Apply buffer offsets and strides (2.19 KB, patch)
2017-11-14 00:47 UTC, Nicolas Dufresne (ndufresne)
rejected Details | Review
remove SHM bufferpool (6.73 KB, patch)
2017-11-17 16:20 UTC, Arnaud Vrac
none Details | Review
waylandsink: Disable video-alignment from the pool (2.48 KB, patch)
2017-11-21 02:13 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
wlwindow: Only update video info on new render (1.84 KB, patch)
2017-11-23 02:51 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
waylandsink: Only try SHM for single memory buffer (962 bytes, patch)
2017-11-23 02:51 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
waylandsink: Allocate only what's needed when copying (1.10 KB, patch)
2017-11-23 02:51 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
waylandsink: Update video info size to buffer size (1.05 KB, patch)
2017-11-23 02:52 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
waylandsink: Validate strides and offset when using FD as SHM (4.26 KB, patch)
2017-11-23 02:52 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
waylandsink: Rollback video info changes when copying (1.63 KB, patch)
2017-11-23 03:28 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Arnaud Vrac 2017-11-08 12:26:54 UTC
Hi,

waylandsink's own SHM buffer allocator is exposed using a GstVideoBufferPool, which allows upstream to set a video alignment in buffers. However, the alignment of such buffers (for example set by ffmpeg based decoders) is ignored, so the result on screen is wrong.

Support for video alignment could be implemented by allocating SHM buffers that account for padding in the buffer width and height, and then use the wp_viewport extension to crop the buffer to the visible width and height.

Testing with weston might also require the following patch (not yet merged): https://patchwork.freedesktop.org/patch/180767/

To reproduce simply decode a video directly to waylandsink using an ffmpeg based decoder, for example:

gst-launch-1.0 filesrc location=/path/to/video.avi ! avidemux ! avdec_mpeg4 ! waylandsink
Comment 1 Olivier Crête 2017-11-13 19:51:03 UTC
We should probably not use the GstVideoBufferPool then as the wp_viewport extension is basically not implemented outside of Weston.
Comment 2 Olivier Crête 2017-11-13 20:41:19 UTC
This is all lies, wl_shm has all it needs as you can specify the offset and stride. But it does mean that if those parameters change, then the wl_shm's buffer needs to be re-created.
Comment 3 Nicolas Dufresne (ndufresne) 2017-11-13 20:55:21 UTC
To add more details, after:

+  wlbuffer = gst_buffer_get_wl_buffer (buffer);

We need to find a way to validate that the video meta (offsets and strides) used to create that wl_buffer matches the what we have on this buffer. They could in theory have changed. That is rarely the case, but I have plan to make this happen more often ;-P


Then, a second bug in:

+ wbuf = gst_wl_shm_memory_construct_wl_buffer (mem, sink->display,
+          &sink->video_info);

The video_info has not yet been updated to match the GstBuffer video meta. In the copy path it works because gst_video_frame_map() secretly update that structure for us. An extra nice thing to do would be to add a helper in gst_video library that just do that.

In GstVideoBufferPool, setting that alignment will simply call gst_video_info_aling() on a VideoInfo structure cached in the buffer pool. Because it's a opaque copy, we often end up being out-of-sync if we use it as our internal pool.
Comment 4 Nicolas Dufresne (ndufresne) 2017-11-14 00:47:51 UTC
Created attachment 363550 [details] [review]
waylandsink: Apply buffer offsets and strides

I could not test this patch as my weston is too old (does not have working NV12 SHM support). Would be nice to report if this solution works for you.

When we propose a pool it is possible that the pool changes the default
offsets and strides as it may get a GstVideoAlignment in it's
configuration. For this reason, we need to read-back in all cases the
strides and offsets from the GstVideoMeta.

This was already done when copying through gst_video_frame_map(), but was
missing when importing file descriptors as SHM.
Comment 5 Arnaud Vrac 2017-11-17 15:19:55 UTC
@stormer: your patch doesn't help, the code you added is already done a few lines above.

As I said, the only way to support this would be to use the aligned width/height in the SHM buffer, and to crop to the actual size using wp_viewport. This is for two reasons:

 - the left, right, top and bottom padding added by the buffer alignment configured upstream are not included in the memory offsets. Left and right padding could be handled by including those in the SHM buffer offset.

 - the SHM protocol assumes the planes are contiguous (ie no arbitrary space between planes). So there is no way to support top and bottom padding from the buffer alignement, which is applied for each plane.
Comment 6 Nicolas Dufresne (ndufresne) 2017-11-17 15:29:42 UTC
Comment on attachment 363550 [details] [review]
waylandsink: Apply buffer offsets and strides

Ok then, your go on making a patch then.
Comment 7 Nicolas Dufresne (ndufresne) 2017-11-17 15:31:20 UTC
To be fair, SHM is just a terribly interface, which should have never been used for planar formarts, it's as bad as DRM Dumb buffers. Maybe the right solution is go back to the Wayland Protocol and implement a better SHM interface.
Comment 8 Arnaud Vrac 2017-11-17 15:44:24 UTC
I agree, I think I'll focus on enabling DMABUF allocations, even with ffmpeg. The simplest solution would probably to check that the video info matches the buffer meta and fallback to a copy if not. Disabling the video alignment option on the buffer pool could also help.
Comment 9 Arnaud Vrac 2017-11-17 16:20:49 UTC
Created attachment 363935 [details] [review]
remove SHM bufferpool
Comment 10 Nicolas Dufresne (ndufresne) 2017-11-17 16:22:23 UTC
Review of attachment 363935 [details] [review]:

Tempting, but too drastic to my taste.
Comment 11 Nicolas Dufresne (ndufresne) 2017-11-21 02:13:18 UTC
Created attachment 364089 [details] [review]
waylandsink: Disable video-alignment from the pool

The SHM interface does not allow passing arbitrary strides and offsets,
for this reason, we simply disable this feature from the proposed pool.

This fixes video artifact seen when using the FFMPEG based video
decoder.
Comment 12 Nicolas Dufresne (ndufresne) 2017-11-21 02:17:03 UTC
Please review, I have actually built latest weston + YUV fixes and tested this one. Why it's better, well the pool is still offered and some scenario will write directly into SHM, notably with videotestsrc, videoconvert, videoscale, etc.
Comment 13 Nicolas Dufresne (ndufresne) 2017-11-21 02:24:16 UTC
Ok, spoke too fast, while it works at 480p, it fails with some 720 and 1080p samples, I need to find out why.
Comment 14 Nicolas Dufresne (ndufresne) 2017-11-21 02:54:46 UTC
Ok, the problem is that libav will still use the SHM allocator we have proposed. It's a bit unfortunate, since we can't use that memory, but because we offer it generically, we'll need to handle the case where we have FD memory but the offsets introduce padding between planes.

For that, we'll need to extrapolate offsets and strides from the first strides (so there is no padding) and compare against the provided array. Then, only if that matches we'll be able to use it as SHM (exactly like DRM Dumb buffers, which share the same poorly defined API, hence wastn't meant to be used with planar formats). Overall, this will make our SHM support more robust. Arnaud's patch didn't protect against an upstream element, that has it's own FdMemory allocator to pass unsupported strides/offsets (I'm thinking about pipewire here).
Comment 15 Arnaud Vrac 2017-11-21 08:35:17 UTC
Is there a way to check that upstream would not use arbitrary plane offsets during the allocation negotiation ? If not, we will more than often get into a state where upstream uses the SHM allocator, and waylandsink will just have to still fallback to a copy to another SHM buffer.
Comment 16 Nicolas Dufresne (ndufresne) 2017-11-21 14:57:41 UTC
You could drop VideoMeta support, and then upstream will have to copy, but you will likely never get any DMABuf again.
Comment 17 Nicolas Dufresne (ndufresne) 2017-11-21 15:06:03 UTC
Also, the definition of arbitrary plane offsets is pretty vague. GStreamer have default rules that may not match your stack default rules for what is standard strides and offsets.

In general, we implement the common sense that was shared with V4L2 UVC devices and XV. Common sense that seems to have been replicated by DRM Dump and SHM, but nothing of this is documented.
Comment 18 Nicolas Dufresne (ndufresne) 2017-11-23 02:51:49 UTC
Created attachment 364249 [details] [review]
wlwindow: Only update video info on new render

The sink->video_info might not reflect the current buffer
when expose is being called.
Comment 19 Nicolas Dufresne (ndufresne) 2017-11-23 02:51:54 UTC
Created attachment 364250 [details] [review]
waylandsink: Only try SHM for single memory buffer
Comment 20 Nicolas Dufresne (ndufresne) 2017-11-23 02:51:58 UTC
Created attachment 364251 [details] [review]
waylandsink: Allocate only what's needed when copying

There was this regression that we'd be using the updated video
info size instead of default size when initializing the pool.
Comment 21 Nicolas Dufresne (ndufresne) 2017-11-23 02:52:03 UTC
Created attachment 364252 [details] [review]
waylandsink: Update video info size to buffer size

We where setting the size to the first memory size, this may be
too small in case we received a buffer with multiple memory.
Comment 22 Nicolas Dufresne (ndufresne) 2017-11-23 02:52:08 UTC
Created attachment 364253 [details] [review]
waylandsink: Validate strides and offset when using FD as SHM

As SHM interface only support 1 stride, and 1 offset, we need to
make sure that there is no padding between planes for planar formats.
Comment 23 Nicolas Dufresne (ndufresne) 2017-11-23 03:07:59 UTC
I'm almost there, but not quite yet. There is still a problem with the video info when we copy to our internal pool. That's because we updated sink->video_info from input buffer. It then has two problems, fails validation, and fails at map because of the update size.
Comment 24 Nicolas Dufresne (ndufresne) 2017-11-23 03:12:53 UTC
Review of attachment 364251 [details] [review]:

The problem I face is related to this patch not being sufficient. It works for the first frame, but then fail for the following. Also, I don't want to run info_from_caps() for every frame.
Comment 25 Nicolas Dufresne (ndufresne) 2017-11-23 03:28:21 UTC
Created attachment 364254 [details] [review]
waylandsink: Rollback video info changes when copying

We change the video info base on the received buffer. We need to
rollback these changes whenever we want to copy into our internal
pool of buffers.
Comment 26 Nicolas Dufresne (ndufresne) 2017-11-23 03:34:05 UTC
Ok, just solved with one more patch, at this state one more won't make much difference.

Some extra notes, I was wrong, avdec is not using our shm allocator, because it ended up pushing from it's initial pool (long story, unrelated). So basically, the copy path was now broken. Many other things were not robust, including resize and redraw, which I covered here.
Comment 27 Nicolas Dufresne (ndufresne) 2017-11-25 19:59:27 UTC
Attachment 364089 [details] pushed as b06a8bf - waylandsink: Disable video-alignment from the pool
Attachment 364249 [details] pushed as 51bb235 - wlwindow: Only update video info on new render
Attachment 364250 [details] pushed as 9bbd5ef - waylandsink: Only try SHM for single memory buffer
Attachment 364251 [details] pushed as 02df3a4 - waylandsink: Allocate only what's needed when copying
Attachment 364252 [details] pushed as cc03335 - waylandsink: Update video info size to buffer size
Attachment 364253 [details] pushed as 1a7363e - waylandsink: Validate strides and offset when using FD as SHM
Attachment 364254 [details] pushed as 2db81d6 - waylandsink: Rollback video info changes when copying
Comment 28 Haihua Hu 2018-06-11 08:00:56 UTC
(In reply to Nicolas Dufresne (ndufresne) from comment #27)
> Attachment 364089 [details] pushed as b06a8bf - waylandsink: Disable
> video-alignment from the pool
> Attachment 364249 [details] pushed as 51bb235 - wlwindow: Only update video
> info on new render
> Attachment 364250 [details] pushed as 9bbd5ef - waylandsink: Only try SHM
> for single memory buffer
> Attachment 364251 [details] pushed as 02df3a4 - waylandsink: Allocate only
> what's needed when copying
> Attachment 364252 [details] pushed as cc03335 - waylandsink: Update video
> info size to buffer size
> Attachment 364253 [details] pushed as 1a7363e - waylandsink: Validate
> strides and offset when using FD as SHM
> Attachment 364254 [details] pushed as 2db81d6 - waylandsink: Rollback video
> info changes when copying

Hi Nicolas Dufresne,

The below patch break 348*240 resolution video playback. How to fix this issue.

1a7363e - waylandsink: Validate strides and offset when using FD as SHM

My added log as below:
wlshmallocator.c:173:gst_wl_shm_validate_video_info: estride = 174 stride = 176
Comment 29 Nicolas Dufresne (ndufresne) 2018-06-11 14:10:36 UTC
Have you notice that this works on gnome-shell ?

gst-launch-1.0 videotestsrc ! video/x-raw,width=348,height=240 ! waylandsink

Looks like some corner case with how Weston is doing things. Can you file a bug specific to that please ?
Comment 30 Nicolas Dufresne (ndufresne) 2018-06-11 14:27:33 UTC
Obviously, gnome-shell does not support planar formats.
Comment 31 Nicolas Dufresne (ndufresne) 2018-06-11 15:20:42 UTC
Interesting, in I420, video-info does this weird thing, the second and third strides are roundup 4. so ROUNDUP4(348/2) == 176. GStreamer has been doing this for years now. Maybe there is or there was software code assuming that lines are 32bit word aligned.

Now, the extrapolate function does not do this spurious rounding. Again, this is a total guess, but I think compositors supporting planar over shm interface are unlike to do this roundup. SHM for planar format is pretty bad idea clearly, as we are never sure we agree on the layout with the compositor. As of now, it's good thing it fails, because what we think the compositor need is not what the buffer pool we offer does.
Comment 32 Nicolas Dufresne (ndufresne) 2018-06-11 15:59:17 UTC
Looks like as of today, the GStreamer wayland pool produces buffer with second U and V planes that have an incompatible pitch. The gl renderer in weston simply divides by two.

    glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT,
	          gs->pitch / gs->hsub[j]);
Comment 33 Tim-Philipp Müller 2018-06-11 18:26:47 UTC
I believe all these things came from "what the X server expected" at the time (as we couldn't express other strides or offsets in <= 0.10).
Comment 34 Nicolas Dufresne (ndufresne) 2018-06-11 19:59:02 UTC
Very likely, and glamour driver is changing the game also. I'll file the other bugs, but the fix I just work does not work, and seems like a mess to me.
Comment 35 Nicolas Dufresne (ndufresne) 2018-06-11 21:52:04 UTC
Please refer to this bug, https://bugzilla.gnome.org/show_bug.cgi?id=796565 . I have tried fixing it, but something is not right, not sure what yet. I've also tested removing the check, it does not render correct, and I also tried before my changes, and still, it does not render correctly. It seems like this resolution never worked in I420.
Comment 36 Haihua Hu 2018-06-12 05:24:53 UTC
(In reply to Nicolas Dufresne (ndufresne) from comment #35)
> Please refer to this bug, https://bugzilla.gnome.org/show_bug.cgi?id=796565
> . I have tried fixing it, but something is not right, not sure what yet.
> I've also tested removing the check, it does not render correct, and I also
> tried before my changes, and still, it does not render correctly. It seems
> like this resolution never worked in I420.

Thanks Nicolas,

Your analysis is correct. Hope we can find some way to fix it.