GNOME Bugzilla – Bug 784069
Add support for OMX_UseBuffer in gstomxvideodec
Last modified: 2018-06-08 15:21:08 UTC
For example this allows the gstomxdecoder to directly fill the pixmaps coming from the video sink. It avoids a buffer copy when the decoder uses a pool provided by a downstream element. If it fails to setup buffers with OMX_UseBuffer the decoders should fallback to usual OMX_AllocateBuffer code path. It will allow to test on desktop the GstOMXBufferPool->other_pool management which is currently only used in the OMX_UseEGLImage case, i.e. on Rpi. So easier for maintenance and enhancements.
Created attachment 354206 [details] [review] omxvideodec: can use OMX_UseBuffer
Created attachment 354712 [details] [review] omxvideodec: can use OMX_UseBuffer Compared to previous version I just added one line which is very similar than https://bug784365.bugzilla-attachments.gnome.org/attachment.cgi?id=354709
Review of attachment 354712 [details] [review]: Shouldn't we also check if the buffers provided by the pool are big enough? ::: omx/gstomxvideodec.c @@ +841,3 @@ + gst_buffer_map (buffer, &info, GST_MAP_WRITE); + images = g_list_append (images, info.data); + "Failed to allocated %d-th buffer of type %s, n_mem: %d", i, Is it safe to unmap already while the data pointer is still stored in images and will be used later by gst_omx_port_use_buffers()?
Review of attachment 354712 [details] [review]: ::: omx/gstomxvideodec.c @@ +841,3 @@ + gst_buffer_map (buffer, &info, GST_MAP_WRITE); + images = g_list_append (images, info.data); + "Failed to allocated %d-th buffer of type %s, n_mem: %d", i, The only guard is that gst-omx has a ref on the associated GstBuffer when calling gst_omx_port_use_buffers. Same when playing, gst-omx has a ref on the GstBuffer before to let the omx platform write into the data. I have mimic what has been done for eglimage which is not mappable. Maybe here I can make sure to keep the buffer mapped while gst_omx_port_use_buffers is called and also when gst-omx calls gst_omx_port_release_buffer from gst_omx_buffer_pool_release_buffer. And unmap when calling acquire. For the other remark, indeed I can check that info.size >= port->port_def.nBufferSize. And also check gst_buffer_map ret.
There is GstVideoFrame which is designed to keep a mapped buffer around.
Created attachment 355423 [details] [review] omx: do not always print an error if OMX_{UseBuffer,EGLImage} fails
Created attachment 355424 [details] [review] omxvideodec: can use OMX_UseBuffer Addressed Guillaume's remarks (keep mapped while calling gst_omx_port_use_buffers / check buffer are big enough), thx for the review btw. I also used GstVideoFrame to avoid managing both a list of GstInfo and a list of GstBuffer (Thx Nicolas for the suggestion).
Review of attachment 355424 [details] [review]: looks good to me; some trivial comments. ::: omx/gstomxvideodec.c @@ +2699,3 @@ + self->use_buffers = FALSE; + if (gst_query_get_n_allocation_pools (query) > 0) { + GST_DEBUG_OBJECT (self, "Try using downstream buffer with OMX_UseBuffer"); "bufferS" ? ::: omx/gstomxvideodec.h @@ +77,2 @@ GstFlowReturn downstream_flow_ret; + gboolean use_buffers; maybe add a comment explaining the semantic of this variable?
Comment on attachment 355423 [details] [review] omx: do not always print an error if OMX_{UseBuffer,EGLImage} fails commit e60ac7e1c0cd0fc4bb7673c16d7c7e13e46f9dd9 Author: Julien Isorce <jisorce@oblong.com> Date: Wed Jul 12 10:29:16 2017 +0100 omx: do not always print an error if OMX_{UseBuffer,EGLImage} fails Let the caller decide to print an error. Because it can be part of a normal trial path. https://bugzilla.gnome.org/show_bug.cgi?id=784069
Comment on attachment 355424 [details] [review] omxvideodec: can use OMX_UseBuffer commit 80c6dd46a1e290716cc22e23ffdf076ea69a775a Author: Julien Isorce <jisorce@oblong.com> Date: Thu Jun 29 22:48:47 2017 +0100 omxvideodec: use OMX_UseBuffer For example this allows the omx decoder to directly fill the pixmaps coming from the video sink. It only avoids a buffer copy when the decoder uses a pool provided by a downstream element. So let's restrict this usage to situations where the decoder decides to use a downstream buffer pool. Tested with Tizonia/OMX.Aratelia.video_decoder.vp8 and with Bellagio/OMX.mesa.video_decoder.avc. If it fails to setup buffers with OMX_UseBuffer the decoders fallbacks to usual OMX_AllocateBuffer. Also it allows to test on desktop the GstOMXBufferPool->other_pool management which was previously only used in the OMX_UseEGLImage case, i.e. on Rpi. https://bugzilla.gnome.org/show_bug.cgi?id=784069
(In reply to Guillaume Desmottes from comment #8) > Review of attachment 355424 [details] [review] [review]: > > looks good to me; some trivial comments. > > ::: omx/gstomxvideodec.c > @@ +2699,3 @@ > + self->use_buffers = FALSE; > + if (gst_query_get_n_allocation_pools (query) > 0) { > + GST_DEBUG_OBJECT (self, "Try using downstream buffer with > OMX_UseBuffer"); > > "bufferS" ? > > ::: omx/gstomxvideodec.h > @@ +77,2 @@ > GstFlowReturn downstream_flow_ret; > + gboolean use_buffers; > > maybe add a comment explaining the semantic of this variable? Done, thx!
Hi Julien. You mentioned that this patch allowed you to test GstOMXBufferPool->other_pool code path on desktop. Could you please describe the exact setup (OMX stack, pipeline, etc) you used to do so? I'd like to do some testing as well.
Sure: git clone https://github.com/tizonia/tizonia-openmax-il export TIZONIA_REPO_DIR=/path/to/tizonia/repo export TIZONIA_INSTALL_DIR=/path/to/install/dir export PATH=$TIZONIA_REPO_DIR/tools:$PATH tools/tizonia-dev-build --deps tools/tizonia-dev-build --no-player --debug --install GST_OMX_CONFIG_DIR=path/to/gst-omx/config/tizonia/ gst-launch-1.0 filesrc location=test.mkv ! matroskademux ! vp8dec ! xvimagesink This will use the GstOMXBufferPool->other_pool code path. (also see the patches on https://bugzilla.gnome.org/show_bug.cgi?id=786738 but should not be needed for simple cases) If you also need to use the eglimage path on desktop, it is a few extra steps (providing your graphic card supports video decoding), let me know if you need that too.
Guillaume did you try the steps above ?
I did but hit a build issue or something. I found another way to test so didn't really bother to investigate. I'll do next time I want to test this. :)
If it is a build error in Tizonia, please report it here https://github.com/tizonia/tizonia-openmax-il/issues Also do not forget to pass --no-player