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 784069 - Add support for OMX_UseBuffer in gstomxvideodec
Add support for OMX_UseBuffer in gstomxvideodec
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other All
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-06-21 22:43 UTC by Julien Isorce
Modified: 2018-06-08 15:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
omxvideodec: can use OMX_UseBuffer (5.88 KB, patch)
2017-06-21 22:44 UTC, Julien Isorce
none Details | Review
omxvideodec: can use OMX_UseBuffer (6.08 KB, patch)
2017-06-29 22:28 UTC, Julien Isorce
none Details | Review
omx: do not always print an error if OMX_{UseBuffer,EGLImage} fails (1.29 KB, patch)
2017-07-12 13:12 UTC, Julien Isorce
committed Details | Review
omxvideodec: can use OMX_UseBuffer (7.15 KB, patch)
2017-07-12 13:18 UTC, Julien Isorce
committed Details | Review

Description Julien Isorce 2017-06-21 22:43:40 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.
Comment 1 Julien Isorce 2017-06-21 22:44:42 UTC
Created attachment 354206 [details] [review]
omxvideodec: can use OMX_UseBuffer
Comment 2 Julien Isorce 2017-06-29 22:28:37 UTC
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
Comment 3 Guillaume Desmottes 2017-07-11 12:43:47 UTC
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()?
Comment 4 Julien Isorce 2017-07-11 14:52:38 UTC
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.
Comment 5 Nicolas Dufresne (ndufresne) 2017-07-11 15:03:37 UTC
There is GstVideoFrame which is designed to keep a mapped buffer around.
Comment 6 Julien Isorce 2017-07-12 13:12:23 UTC
Created attachment 355423 [details] [review]
omx: do not always print an error if OMX_{UseBuffer,EGLImage} fails
Comment 7 Julien Isorce 2017-07-12 13:18:19 UTC
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).
Comment 8 Guillaume Desmottes 2017-07-12 15:12:42 UTC
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 9 Julien Isorce 2017-07-12 17:14:52 UTC
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 10 Julien Isorce 2017-07-12 17:15:47 UTC
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
Comment 11 Julien Isorce 2017-07-12 17:17:13 UTC
(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!
Comment 12 Guillaume Desmottes 2018-05-23 10:53:47 UTC
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.
Comment 13 Julien Isorce 2018-05-24 00:02:49 UTC
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.
Comment 14 Julien Isorce 2018-06-07 19:46:16 UTC
Guillaume did you try the steps above ?
Comment 15 Guillaume Desmottes 2018-06-08 07:43:30 UTC
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. :)
Comment 16 Julien Isorce 2018-06-08 15:21:08 UTC
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