GNOME Bugzilla – Bug 787093
omx: add dynamic buffer mode from OMX 1.2.0
Last modified: 2017-12-14 09:05:05 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.
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.
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.
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).
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).
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.
(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.
(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.
What is the status of this?
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.
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.
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).
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).
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)
(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.
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.
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.
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).
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).
No change in patches. I just rebased on top of master.
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 ?
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. :)
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.
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.
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).
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 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 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 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 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