GNOME Bugzilla – Bug 790057
waylandsink: does not respect video alignment from upstream
Last modified: 2018-06-12 05:24:53 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
We should probably not use the GstVideoBufferPool then as the wp_viewport extension is basically not implemented outside of Weston.
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.
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.
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.
@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 on attachment 363550 [details] [review] waylandsink: Apply buffer offsets and strides Ok then, your go on making a patch then.
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.
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.
Created attachment 363935 [details] [review] remove SHM bufferpool
Review of attachment 363935 [details] [review]: Tempting, but too drastic to my taste.
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.
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.
Ok, spoke too fast, while it works at 480p, it fails with some 720 and 1080p samples, I need to find out why.
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).
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.
You could drop VideoMeta support, and then upstream will have to copy, but you will likely never get any DMABuf again.
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.
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.
Created attachment 364250 [details] [review] waylandsink: Only try SHM for single memory buffer
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.
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.
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.
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.
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.
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.
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.
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
(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
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 ?
Obviously, gnome-shell does not support planar formats.
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.
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]);
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).
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.
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.
(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.