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 787093 - omx: add dynamic buffer mode from OMX 1.2.0
omx: add dynamic buffer mode from OMX 1.2.0
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-omx
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-08-31 16:51 UTC by Guillaume Desmottes
Modified: 2017-12-14 09:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
omx: add API to implement dynamic buffers support (9.11 KB, patch)
2017-08-31 16:52 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc/dec: factor out input buffer allocation (4.74 KB, patch)
2017-08-31 16:52 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc: use dynamic buffer mode on input if possible (4.13 KB, patch)
2017-08-31 16:52 UTC, Guillaume Desmottes
needs-work Details | Review
omxvideodec: use dynamic buffer mode on input if possible (8.10 KB, patch)
2017-08-31 16:52 UTC, Guillaume Desmottes
none Details | Review
omx: add API to implement dynamic buffers support (9.11 KB, patch)
2017-10-19 14:35 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc/dec: factor out input buffer allocation (4.74 KB, patch)
2017-10-19 14:35 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc: use dynamic buffer mode on input if possible (5.85 KB, patch)
2017-10-19 14:35 UTC, Guillaume Desmottes
none Details | Review
omxvideodec: use dynamic buffer mode on input if possible (10.47 KB, patch)
2017-10-19 14:35 UTC, Guillaume Desmottes
none Details | Review
omx: add API to implement dynamic buffers support (9.11 KB, patch)
2017-11-30 14:26 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc/dec: factor out input buffer allocation (4.74 KB, patch)
2017-11-30 14:26 UTC, Guillaume Desmottes
none Details | Review
omxvideoenc: use dynamic buffer mode on input if possible (5.85 KB, patch)
2017-11-30 14:26 UTC, Guillaume Desmottes
none Details | Review
omxvideodec: use dynamic buffer mode on input if possible (10.47 KB, patch)
2017-11-30 14:26 UTC, Guillaume Desmottes
none Details | Review
omx: add API to implement dynamic buffers support (9.11 KB, patch)
2017-12-14 08:18 UTC, Guillaume Desmottes
committed Details | Review
omxvideoenc/dec: factor out input buffer allocation (4.74 KB, patch)
2017-12-14 08:18 UTC, Guillaume Desmottes
committed Details | Review
omxvideoenc: use dynamic buffer mode on input if possible (5.85 KB, patch)
2017-12-14 08:18 UTC, Guillaume Desmottes
committed Details | Review
omxvideodec: use dynamic buffer mode on input if possible (10.47 KB, patch)
2017-12-14 08:18 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2017-08-31 16:51:51 UTC
OMX 1.2.0 introduced a new dynamic buffer mode saving components from pre-allocating buffers's payload. By using it we can prevent mem copies between GStreamer and OMX.
Comment 1 Guillaume Desmottes 2017-08-31 16:52:19 UTC
Created attachment 358872 [details] [review]
omx: add API to implement dynamic buffers support

OMX 1.2.0 introduced a third way to manage buffers by allowing
components to only allocate buffers header during their initialization
and change their pBuffer pointer at runtime.
This new feature can save us a copy between GStreamer and OMX for each
input buffer.

This patch adds API to allocate and use such buffers.
Comment 2 Guillaume Desmottes 2017-08-31 16:52:24 UTC
Created attachment 358873 [details] [review]
omxvideoenc/dec: factor out input buffer allocation

No semantic change so far. I'm going to add an alternate way to allocate
input buffers.
Comment 3 Guillaume Desmottes 2017-08-31 16:52:29 UTC
Created attachment 358874 [details] [review]
omxvideoenc: use dynamic buffer mode on input if possible

If the OMX component supports dynamic buffer mode and the input buffers
are properly aligned avoid copying each input frame between OMX and
GStreamer.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 4 Guillaume Desmottes 2017-08-31 16:52:34 UTC
Created attachment 358875 [details] [review]
omxvideodec: use dynamic buffer mode on input if possible

Prevent from copying the input buffers between GStreamer and OMX.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 5 Nicolas Dufresne (ndufresne) 2017-09-06 19:31:02 UTC
Review of attachment 358874 [details] [review]:

::: omx/gstomxvideoenc.c
@@ +1155,3 @@
+  if (input_size >= self->enc_in_port->port_def.nBufferSize &&
+      (self->enc_in_port->port_def.nBufferAlignment == 0 ||
+          input_size % self->enc_in_port->port_def.nBufferAlignment == 0)) {

That is not what alignment means. To check the alignment you need to map the memory and check that the pointer value is a multiple of nBufferAlignment. For video buffers, you should check that the stride matches the port definition and that you have only 1 memory object.

To be robust, this check need to be done for every incoming buffers, in case it changed. The dynamic buffer mode can't handle it, but this way you'll be able to at least warn that buffers are dropped. Of course, bonus point if you want a proper way to fallback.

@@ +1487,3 @@
+     * by the OMX component. */
+    if (!gst_omx_buffer_map_frame (outbuf, inbuf, info)) {
+      GST_ERROR_OBJECT (self, "Failed to map input buffer");

If this is fatal, then GST_ELEMENT_ERROR please.
Comment 6 Guillaume Desmottes 2017-10-02 13:47:49 UTC
(In reply to Nicolas Dufresne (stormer) from comment #5)
> Review of attachment 358874 [details] [review] [review]:
> 
> ::: omx/gstomxvideoenc.c
> @@ +1155,3 @@
> +  if (input_size >= self->enc_in_port->port_def.nBufferSize &&
> +      (self->enc_in_port->port_def.nBufferAlignment == 0 ||
> +          input_size % self->enc_in_port->port_def.nBufferAlignment == 0)) {
> 
> That is not what alignment means. To check the alignment you need to map the
> memory and check that the pointer value is a multiple of nBufferAlignment.
> For video buffers, you should check that the stride matches the port
> definition and that you have only 1 memory object.

Ok, I suppose we have to do check this for the decoder as well then.

> To be robust, this check need to be done for every incoming buffers, in case
> it changed. The dynamic buffer mode can't handle it, but this way you'll be
> able to at least warn that buffers are dropped. Of course, bonus point if
> you want a proper way to fallback.

We can't really fallback so maybe dynamic mode should only be implemented if nBufferAlignment == 0?
It's a bit risky to enable it automatically if, by chance, the first buffer is mapped on an address fitting the alignment and then fail later when another buffer doesn't.
Do we have a way to know for sure if all incoming buffers will satisfy this requirement ? GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT doesn't seem to give us that.
Comment 7 Nicolas Dufresne (ndufresne) 2017-10-02 14:02:51 UTC
(In reply to Guillaume Desmottes from comment #6)
> > To be robust, this check need to be done for every incoming buffers, in case
> > it changed. The dynamic buffer mode can't handle it, but this way you'll be
> > able to at least warn that buffers are dropped. Of course, bonus point if
> > you want a proper way to fallback.
> 
> We can't really fallback so maybe dynamic mode should only be implemented if
> nBufferAlignment == 0?

Well, if this is the only thing you are checking, this means you accept VMALLOC memory, for fallback is trivial. If you have checked some other things, that result in other type of memory, then alignment will likely be a page anyway, and won't be by chance.

GStreamer allocation negotiation is not complete, so we have to make guesses on what is likely enough and implement as many fallback as possible.

> It's a bit risky to enable it automatically if, by chance, the first buffer
> is mapped on an address fitting the alignment and then fail later when
> another buffer doesn't.
> Do we have a way to know for sure if all incoming buffers will satisfy this
> requirement ? GST_BUFFER_POOL_OPTION_VIDEO_ALIGNMENT doesn't seem to give us
> that.

No, but we have a lot of code that depends these to be consistent (all our ORC code crashes deeply without checking if the pool have accepted teh VIDEO_ALIGNMENT or not). It's a mess when it crash deeply in the stack (specially with OMX). What I meant is that you have to fail the hard way.
Comment 8 Sebastian Dröge (slomo) 2017-10-19 14:26:02 UTC
What is the status of this?
Comment 9 Guillaume Desmottes 2017-10-19 14:35:27 UTC
Created attachment 361877 [details] [review]
omx: add API to implement dynamic buffers support

OMX 1.2.0 introduced a third way to manage buffers by allowing
components to only allocate buffers header during their initialization
and change their pBuffer pointer at runtime.
This new feature can save us a copy between GStreamer and OMX for each
input buffer.

This patch adds API to allocate and use such buffers.
Comment 10 Guillaume Desmottes 2017-10-19 14:35:32 UTC
Created attachment 361878 [details] [review]
omxvideoenc/dec: factor out input buffer allocation

No semantic change so far. I'm going to add an alternate way to allocate
input buffers.
Comment 11 Guillaume Desmottes 2017-10-19 14:35:37 UTC
Created attachment 361879 [details] [review]
omxvideoenc: use dynamic buffer mode on input if possible

If the OMX component supports dynamic buffer mode and the input buffers
are properly aligned avoid copying each input frame between OMX and
GStreamer.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 12 Guillaume Desmottes 2017-10-19 14:35:43 UTC
Created attachment 361880 [details] [review]
omxvideodec: use dynamic buffer mode on input if possible

Prevent from copying the input buffers between GStreamer and OMX.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 13 Julien Isorce 2017-11-08 00:12:29 UTC
Looks good. Just to confirm this is for the sink pad only, isn't it ? I mean for both decoder and encoder. (So the MAP_READ only)
Thx for having checked for any regression on rpi. Any chance that you tried on rpi to force it like on zynq ? (maybe it is supported too)
Comment 14 Guillaume Desmottes 2017-11-08 10:03:01 UTC
(In reply to Julien Isorce from comment #13)
> Just to confirm this is for the sink pad only, isn't it ? I mean
> for both decoder and encoder. (So the MAP_READ only)

Correct, it's only implemented for sink pads at the moment. Our decoder produces dmabuf so it's not that useful for us on src pads.

> Any chance that you tried
> on rpi to force it like on zynq ? (maybe it is supported too)

I did not. That seems very unlikely as rpi implements OMX 1.1.2.
Comment 15 Guillaume Desmottes 2017-11-30 14:26:24 UTC
Created attachment 364681 [details] [review]
omx: add API to implement dynamic buffers support

OMX 1.2.0 introduced a third way to manage buffers by allowing
components to only allocate buffers header during their initialization
and change their pBuffer pointer at runtime.
This new feature can save us a copy between GStreamer and OMX for each
input buffer.

This patch adds API to allocate and use such buffers.
Comment 16 Guillaume Desmottes 2017-11-30 14:26:40 UTC
Created attachment 364682 [details] [review]
omxvideoenc/dec: factor out input buffer allocation

No semantic change so far. I'm going to add an alternate way to allocate
input buffers.
Comment 17 Guillaume Desmottes 2017-11-30 14:26:47 UTC
Created attachment 364683 [details] [review]
omxvideoenc: use dynamic buffer mode on input if possible

If the OMX component supports dynamic buffer mode and the input buffers
are properly aligned avoid copying each input frame between OMX and
GStreamer.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 18 Guillaume Desmottes 2017-11-30 14:26:54 UTC
Created attachment 364684 [details] [review]
omxvideodec: use dynamic buffer mode on input if possible

Prevent from copying the input buffers between GStreamer and OMX.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 19 Guillaume Desmottes 2017-11-30 14:27:21 UTC
No change in patches. I just rebased on top of master.
Comment 20 Julien Isorce 2017-12-13 12:04:17 UTC
I just tested on Tizonia with its omxvp8dec and it works, great!

Guillaume, is there anything remaining ? Does it depend on https://bugzilla.gnome.org/show_bug.cgi?id=791211 ?
Comment 21 Guillaume Desmottes 2017-12-14 08:17:13 UTC
Awesome, thanks for testing!

No it's not related this bug, it's just a matter of merging the patches. Let me just rebase them first. :)
Comment 22 Guillaume Desmottes 2017-12-14 08:18:38 UTC
Created attachment 365521 [details] [review]
omx: add API to implement dynamic buffers support

OMX 1.2.0 introduced a third way to manage buffers by allowing
components to only allocate buffers header during their initialization
and change their pBuffer pointer at runtime.
This new feature can save us a copy between GStreamer and OMX for each
input buffer.

This patch adds API to allocate and use such buffers.
Comment 23 Guillaume Desmottes 2017-12-14 08:18:44 UTC
Created attachment 365522 [details] [review]
omxvideoenc/dec: factor out input buffer allocation

No semantic change so far. I'm going to add an alternate way to allocate
input buffers.
Comment 24 Guillaume Desmottes 2017-12-14 08:18:50 UTC
Created attachment 365523 [details] [review]
omxvideoenc: use dynamic buffer mode on input if possible

If the OMX component supports dynamic buffer mode and the input buffers
are properly aligned avoid copying each input frame between OMX and
GStreamer.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 25 Guillaume Desmottes 2017-12-14 08:18:56 UTC
Created attachment 365524 [details] [review]
omxvideodec: use dynamic buffer mode on input if possible

Prevent from copying the input buffers between GStreamer and OMX.

Tested on zynqultrascaleplus and rpi (without dynamic buffers).
Comment 26 Julien Isorce 2017-12-14 09:03:14 UTC
Comment on attachment 365521 [details] [review]
omx: add API to implement dynamic buffers support

commit da07a647b8ee0dbbf59f667e778c7b6b51a5cd7e
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Jul 20 16:31:54 2017 +0200

    omx: add API to implement dynamic buffers support
    
    OMX 1.2.0 introduced a third way to manage buffers by allowing
    components to only allocate buffers header during their initialization
    and change their pBuffer pointer at runtime.
    This new feature can save us a copy between GStreamer and OMX for each
    input buffer.
    
    This patch adds API to allocate and use such buffers.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=787093
Comment 27 Julien Isorce 2017-12-14 09:03:32 UTC
Comment on attachment 365522 [details] [review]
omxvideoenc/dec: factor out input buffer allocation

commit e55675b7ceb92286d539752c2c83939226304c7e
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Jul 20 12:56:37 2017 +0200

    omxvideoenc/dec: factor out input buffer allocation
    
    No semantic change so far. I'm going to add an alternate way to allocate
    input buffers.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=787093
Comment 28 Julien Isorce 2017-12-14 09:03:50 UTC
Comment on attachment 365523 [details] [review]
omxvideoenc: use dynamic buffer mode on input if possible

commit 533ee7fadcd167b92f263c1676ef7468c0ac2959
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Thu Jul 20 16:35:31 2017 +0200

    omxvideoenc: use dynamic buffer mode on input if possible
    
    If the OMX component supports dynamic buffer mode and the input buffers
    are properly aligned avoid copying each input frame between OMX and
    GStreamer.
    
    Tested on zynqultrascaleplus and rpi (without dynamic buffers).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=787093
Comment 29 Julien Isorce 2017-12-14 09:04:11 UTC
Comment on attachment 365524 [details] [review]
omxvideodec: use dynamic buffer mode on input if possible

commit 7048134fa9818fc7814942698e87570f31932f26
Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Date:   Wed Aug 9 12:07:33 2017 -0400

    omxvideodec: use dynamic buffer mode on input if possible
    
    Prevent from copying the input buffers between GStreamer and OMX.
    
    Tested on zynqultrascaleplus and rpi (without dynamic buffers).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=787093