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 779790 - waylandsink: add offset setting for multiplane dmabuf
waylandsink: add offset setting for multiplane dmabuf
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.11.1
Other Linux
: Normal normal
: 1.11.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-09 07:50 UTC by Shinya Saito
Modified: 2017-03-14 15:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add offset setting for multiplane dmabuf (1.31 KB, patch)
2017-03-09 07:50 UTC, Shinya Saito
needs-work Details | Review
waylandsink: fix memory offset calculation for multi-plane dmabuf buffers (2.28 KB, patch)
2017-03-13 10:52 UTC, George Kiagiadakis
none Details | Review
waylandsink: fix memory offset calculation for dmabuf buffers (2.71 KB, patch)
2017-03-14 12:18 UTC, George Kiagiadakis
committed Details | Review
glupload: adjust memory offset calculation for dmabuf buffers (1.14 KB, patch)
2017-03-14 12:20 UTC, George Kiagiadakis
committed Details | Review
kmssink: adjust memory offset calculation for dmabuf buffers (1.03 KB, patch)
2017-03-14 12:21 UTC, George Kiagiadakis
committed Details | Review

Description Shinya Saito 2017-03-09 07:50:02 UTC
Created attachment 347528 [details] [review]
add offset setting for multiplane dmabuf

By default, the offset value of each plane is taken from GstVideoInfo, which is corresponding to the color format. There are cases where this value is different from the actual memory offset.

When the memory is allocated for each plane separately,
The value of the offset of each memory block (from GstMem) should be used.
Comment 1 Nicolas Dufresne (ndufresne) 2017-03-09 15:18:27 UTC
Review of attachment 347528 [details] [review]:

You definitely spotted a bug here, good catch. The solution isn't quite right. In fact, the memory offset in GStreamer is currently always 0, but we should eventually account for it. That needs to be implement in v4l2 (were such offset is signalled) and accounted in glupload, kmssink, vl42sink and possibly vaapisink. Considering the spread, a helper would be nice.

Better approach is to use gst_buffer_find_memory (buffer, offset, ...). The offset is what you get from the video meta. This is generic, and will work regardless if you have 1 or more DMABuf. The length parameter is tricky, and is unimplemented (using 1) in GstVideoMeta::default_map(). The returned skip is the actual offset on the memory. The final offset would then be mem->offset + skip (regardless the plane). I think in most correct implementation, we simply use skip, which is always correct for now, but won't always be.
Comment 2 George Kiagiadakis 2017-03-13 10:52:41 UTC
Created attachment 347818 [details] [review]
waylandsink: fix memory offset calculation for multi-plane dmabuf buffers

The offset calculation was indeed wrong. In my older implementations I was using 0 for the offset, which worked at least (while the current implementation doesn't work for sure). The correct solution is indeed mem->offset + skip, which works regardless of how many file descriptors & planes there are.

I think the attached patch is doing this correctly now. Nicolas?
Comment 3 Nicolas Dufresne (ndufresne) 2017-03-13 14:08:46 UTC
Review of attachment 347818 [details] [review]:

Yes, this is the right method I believe. This probably fixes YUV handling with latest Weston. On PC, the best way to test is to use vivid capture driver, loaded with mplane=2. And then:

  v4l2src io-mode=dmabuf ! video/x-raw,format... ! waylandsink

As mention, I would error-out if a plane is not found though.

::: ext/wayland/wllinuxdmabuf.c
@@ +113,3 @@
+      zwp_linux_buffer_params_v1_add (params, fd, i, m->offset + skip,
+          stride, 0, 0);
+    }

I think we should handle the else/error case here.
Comment 4 George Kiagiadakis 2017-03-14 12:18:04 UTC
Created attachment 347914 [details] [review]
waylandsink: fix memory offset calculation for dmabuf buffers

Added error handling.
Comment 5 George Kiagiadakis 2017-03-14 12:20:38 UTC
Created attachment 347915 [details] [review]
glupload: adjust memory offset calculation for dmabuf buffers

This applies the same logic to glupload. The code there already uses gst_buffer_find_memory, but the memory offset is not being accounted for.
Comment 6 George Kiagiadakis 2017-03-14 12:21:17 UTC
Created attachment 347916 [details] [review]
kmssink: adjust memory offset calculation for dmabuf buffers

Same for kmssink.
Comment 7 Víctor Manuel Jáquez Leal 2017-03-14 12:40:23 UTC
Comment on attachment 347916 [details] [review]
kmssink: adjust memory offset calculation for dmabuf buffers

lgtm
Comment 8 Nicolas Dufresne (ndufresne) 2017-03-14 14:31:20 UTC
Review of attachment 347914 [details] [review]:

Looks good now.
Comment 9 George Kiagiadakis 2017-03-14 15:08:57 UTC
commit 4757ec88604835b5e3b6ce32ba6ddb5e6b6611b4
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Mon Mar 13 16:06:53 2017 +0200

    kmssink: adjust memory offset calculation for dmabuf buffers
    
    The data in the dmabuf fd may not start from byte 0, therefore
    we need to inform DRM about this additional offset.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=779790

commit 20d4aca0d44b83347c12c24e770a5b578afcfc20
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Mon Mar 13 15:48:33 2017 +0200

    glupload: adjust memory offset calculation for dmabuf buffers
    
    The data in the dmabuf fd may not start from byte 0, therefore
    we need to inform EGL about this additional offset.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=779790

commit 5e9ce3313f4acd26b6672e7729f2a3e4f95ff798
Author: George Kiagiadakis <george.kiagiadakis@collabora.com>
Date:   Mon Mar 13 12:46:47 2017 +0200

    waylandsink: fix memory offset calculation for dmabuf buffers
    
    https://bugzilla.gnome.org/show_bug.cgi?id=779790