GNOME Bugzilla – Bug 740900
glbufferpool: Add support for GstVideoAlignement
Last modified: 2014-12-19 18:20:11 UTC
In order to remove 1 copy when doing playback with libav video decoder, we need to support GstVideoAlignement. libav will request from top and left padding, most likely to enable some optimization in the decoder code.
Created attachment 291803 [details] [review] [PATCH] glmemory: Handle upload/download flags from map Problem was that if buffer was mapped READWRITE (state of buffers from libav right now), mapping it READ/GL will not upload. This is because the flag is only set when the buffer is unmapped. We can fix this by setting the flags in map. This result in already mapped buffer that get mapped to be read in GL will be uploaded. The problem is that if the write mapper makes modification afterward, the modification will never get uploaded. https://bugzilla.gnome.org/show_bug.cgi?id=740900 --- gst-libs/gst/gl/gstglmemory.c | 26 +++++++++++--------------- gst-libs/gst/gl/gstglmemory.h | 1 - 2 files changed, 11 insertions(+), 16 deletions(-)
Created attachment 291804 [details] [review] [PATCH] gl: Add support for GstVideoAlignment This allow saving a copy with libav video decoders or decoders with similar padding requirement. https://bugzilla.gnome.org/show_bug.cgi?id=740900 --- gst-libs/gst/gl/gstglbufferpool.c | 15 +++++++++++++-- gst-libs/gst/gl/gstglmemory.c | 11 +---------- 2 files changed, 14 insertions(+), 12 deletions(-)
This is mostly a request for comment, if we think it's necessary, the first patch could be improve to remember the write mapped state and no clear the need upload flags. It could make sense, as it would mean that map read gl would always upload if already mapped read write for memory.
Created attachment 291941 [details] [review] [v2] gl: Add support for GstVideoAlignment This new version does not rely on adding API to GstVideoInfo. Instead it assumed that plane size = stride * padded_height, and obtain the padded height by padding the GstVideoAlignment to GstGLMemory. I've duplicated a lot of API to be "backward compatible" and reduce the patch size also. It might though be better just to add the parameter, and allow NULL.
Review of attachment 291803 [details] [review]: I think this makes sense. > This is mostly a request for comment, if we think it's necessary, the first > patch could be improve to remember the write mapped state and no clear the need > upload flags. It could make sense, as it would mean that map read gl would > always upload if already mapped read write for memory. Right so a readwrite mapped memory when mapped read/gl would upload every time. I think that makes sense as well.
As for the other patch. > gl: Add support for GstVideoAlignment If bug #740899 doesn't get fixed, I would prefer changing the GstGLMemory API to add the GstVideoAlignment parameter. Otherwise they both look good as well.
(In reply to comment #6) > As for the other patch. > > > gl: Add support for GstVideoAlignment > > If bug #740899 doesn't get fixed, I would prefer changing the GstGLMemory API > to add the GstVideoAlignment parameter. Otherwise they both look good as well. I think 740899 will not happen, we are concern that not having any extra padding will make the API at risk. Just to make sure, you'd prefer if I simply add the new parameter instead of duplicating ? I would prefer this too, I was just a bit concern with the patch size. Let me know, I'll update the patches accordingly.
(In reply to comment #7) > Just to make sure, you'd prefer if I simply add the new parameter instead of > duplicating ? I would prefer this too, I was just a bit concern with the patch > size. Let me know, I'll update the patches accordingly. Yes, just add the new parameter to the existing functions allowing NULL to mean the defaults.
Created attachment 292683 [details] [review] [PATCH] glmemory: Handle custom stride with OPENGL3 https://bugzilla.gnome.org/show_bug.cgi?id=740900 --- gst-libs/gst/gl/gstglmemory.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Created attachment 292684 [details] [review] [PATCH] glmemory: Handle upload/download flags from map Problem was that if buffer was mapped READWRITE (state of buffers from libav right now), mapping it READ/GL will not upload. This is because the flag is only set when the buffer is unmapped. We can fix this by setting the flags in map. This result in already mapped buffer that get mapped to be read in GL will be uploaded. The problem is that if the write mapper makes modification afterward, the modification will never get uploaded. https://bugzilla.gnome.org/show_bug.cgi?id=740900 --- gst-libs/gst/gl/gstglmemory.c | 26 +++++++++++--------------- gst-libs/gst/gl/gstglmemory.h | 1 - 2 files changed, 11 insertions(+), 16 deletions(-)
Created attachment 292685 [details] [review] [PATCH] gl: Add support for GstVideoAlignment This allow saving a copy with libav video decoders or decoders with similar padding requirement. https://bugzilla.gnome.org/show_bug.cgi?id=740900 --- ext/gl/gstgloverlay.c | 4 +- gst-libs/gst/gl/gstglbufferpool.c | 18 ++++++- gst-libs/gst/gl/gstglcolorconvert.c | 4 +- gst-libs/gst/gl/gstgldownload.c | 2 +- gst-libs/gst/gl/gstglmemory.c | 105 +++++++++++++++++++++++++----------- gst-libs/gst/gl/gstglmemory.h | 15 +++--- gst-libs/gst/gl/gstglupload.c | 6 +-- gst-libs/gst/gl/gstgluploadmeta.c | 4 +- 8 files changed, 107 insertions(+), 51 deletions(-)
Should be ready now, I've tested all combination I could and works fine now. Stride support was broken with OPENGL3 which I fixed here. Was OPENGL3 introdcuced in master or is it in 1.4 ? Let me know since this patch should probably be backported if it was already there. Combination I could test (all on Linux): X11: GLX/opengl GLX/opengl3 EGL/opengl EGL/gles2 Wayland: EGL/opengl EGL/gles2
opengl3 was added in master
As explain in bug #741501 , all buffers get discarded and reallocated every time with patch. This is because the size we allocate the buffer and the size we give to the base class differs. What has been suggested in this patch is to imply update the configuration with the new size after applying the alignment.
And by that, I mean this bug: https://bugzilla.gnome.org/show_bug.cgi?id=735386
Hmm, :-(, this one: https://bugzilla.gnome.org/show_bug.cgi?id=741420 But 735386 is somehow a duplicate.
Ok, the conclusion is that I should simply update the config size from VideoInfo. Will update the patch.
Comment on attachment 292685 [details] [review] [PATCH] gl: Add support for GstVideoAlignment With some modification to update the config, and recalculate the size to match our expectation.
commit 9954de1ccd05be868391b8268f0561ca4e83a9c3 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> glbufferpool: Always recalculate buffer size Actually we should always recalculate buffer size since our buffer size even when not-padded is smaller for many sub-sampled formats. This is because we don't add padding between the planes. https://bugzilla.gnome.org/show_bug.cgi?id=740900 commit 170a49f9011535f1efe89dd754afc2ed81d35e92 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Dec 19 12:12:08 2014 -0500 glmemory: No need for padding A memory object cannot be put on stack, so no need for padding. commit 9954de1ccd05be868391b8268f0561ca4e83a9c3 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Dec 19 12:11:08 2014 -0500 gl: Add support for GstVideoAlignment This allow saving a copy with libav video decoders or decoders with similar padding requirement. https://bugzilla.gnome.org/show_bug.cgi?id=740900 commit da3ae06cd17494ec2d78b9c3aee6296d4108d829 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Nov 22 11:25:23 2014 -0500 glmemory: Handle upload/download flags from map Problem was that if buffer was mapped READWRITE (state of buffers from libav right now), mapping it READ/GL will not upload. This is because the flag is only set when the buffer is unmapped. We can fix this by setting the flags in map. This result in already mapped buffer that get mapped to be read in GL will be uploaded. The problem is that if the write mapper makes modification afterward, the modification will never get uploaded. https://bugzilla.gnome.org/show_bug.cgi?id=740900 commit 4f3c33af3adf2dedfe90eaa9a7cbe6209762b20c Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Sat Dec 13 21:47:38 2014 -0500 glmemory: Handle custom stride with OPENGL3 https://bugzilla.gnome.org/show_bug.cgi?id=740900
commit 046639ddaaf41b4b966bd5e132da0a3aa8f08107 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Dec 19 13:18:50 2014 -0500 gltest: Port to new API https://bugzilla.gnome.org/show_bug.cgi?id=740900