GNOME Bugzilla – Bug 796918
omxvideoenc: implement dmabuf export on encoder input buffers
Last modified: 2018-08-31 07:11:09 UTC
The following patches adds support for exporting buffers dmabuf input buffers to upstream enabling use to do things like 'v4l2src io-mode=dmabuf-import ! omxh264enc'. This feature has only be tested on the zynq but should be easily re-usable on other platforms as well. While doing so I also improved the buffers life cycle management and fixed various related issues. I did some testing on the pi and didn't spot any regression.
Created attachment 373254 [details] [review] omx: factor out gst_omx_port_set_dmabuf() No semantic change. I also made the debug message a bit clearer.
Created attachment 373255 [details] [review] omx: add pBuffer to OMX_PERFORMANCE logs Can be useful to check the fd being passed when using dmabuf.
Created attachment 373256 [details] [review] omxvideoenc: factor out gst_omx_video_enc_deallocate_in_buffers() Will add extra code when adding input buffer pool.
Created attachment 373257 [details] [review] omxvideoenc: factor out gst_omx_video_enc_set_to_idle() No semantic change. We'll have to use this when the input pool is activated so we can allocate buffers.
Created attachment 373258 [details] [review] omx: factor out gst_omx_buffer_get/set_omx_buf() Move the qdata code to helper functions as I'm going to need them in omxvideoenc to implement dmabuf export.
Created attachment 373259 [details] [review] omxvideo{enc,dec}: stop calling shutdown() in change_state This is no longer needed since we implemented close() vfuncs as the encoder/decoder base class already take care of calling close() (which is calling shutdown()) in its own change_state implementation. We also move the shut down of the component from PAUSED_TO_READY to READY_TO_NULL. By doing so upstream will have already deactivated the pool from the encoder and so won't be preventing the OMX state change as the buffers will all be released.
Created attachment 373260 [details] [review] omx: call gst_omx_buffer_unmap() when handling BUFFER_DONE When using a input buffer pool, the buffer may be released to the pool when gst_omx_buffer_unmap() is called. We need to have buf->used unset at this point as the pool may use it to check the status of the pool. {Empty,Fill}BufferDone is called from OMX internal threads while messages are handled from gst elements' thread. Best to do all this when handling the message so we don't mess with OMX threads and keep the original thread/logic split.
Created attachment 373261 [details] [review] omxbufferpool: deallocate OMX buffers when stopping The pool is stopped when all the buffers have been released. Deallocate when stopping so we are sure that the buffers aren't still used by another element.
Created attachment 373262 [details] [review] turn GstOMXComponent to a GstMiniObject Will use it for refcounting.
Created attachment 373263 [details] [review] omxbufferpool: reference the OMX component Now that the pool is responsible of freeing the OMX buffers, we need to ensure that the OMX component stay alive while the pool is as we rely on the component to free the buffers. The GstOMXPort is owned by the component so no need to ref this one.
Created attachment 373264 [details] [review] omxvideodec: prevent timeout when shutting down because of pending out buffers The OMX transition state to Loaded won't be complete until all buffers have been freed. There is no point waiting, and timeout, if we know that output buffers haven't been freed yet. The typical scenario is output buffers being still used downstream and being freed later when released back to the pool.
Created attachment 373265 [details] [review] omxvideodec: fix pool caps reference stealing gst_buffer_pool_config_get_params() doesn't ref the returning caps; so gst_caps_replace() was unreffing the reference owned by the pool.
Created attachment 373266 [details] [review] omxvideodec: fix gst_video_info_from_caps() caps assertion The "use buffers" code path uses gst_video_info_from_caps() which is asserting if caps is NULL (because pool was rejected).
Created attachment 373267 [details] [review] omxvideodec: factor out gst_omx_try_importing_buffer() No semantic change, just make the code clearer and improve debug output.
Created attachment 373268 [details] [review] omxvideodec: don't import not dmabuf when dec is in dmabuf mode Fix 'omxh264 ! videocrop' pipeline.
Created attachment 373269 [details] [review] omxvideoenc: implement dmabuf export on input buffers Propose pool upstream so input buffers can be allocated by the port and exported as dmabuf. The actual OMX buffers are allocated when the pool is activated, so we don't end up doing useless allocations if the pool isn't used.
Created attachment 373270 [details] [review] omxbufferpool: check if pool->buffers still exist when stopping The pool can be deactivated as part of its finalizing process, in much case pool->buffers may no longer exist.
Created attachment 373271 [details] [review] omxvideodec: don't import OMX buffers from downstream We already have code configuring the encoder stride and slice height when receiving the first buffer from upstream. We don't have an equivalent when the encoder is exporting its buffers to the decoder. There is no point adding it and making the code even more complex as we wouldn't gain anything by exporting from the encoder to the decoder. The dynamic buffer mode already ensures 0-copy between OMX components.
Created attachment 373279 [details] [review] omxvideodec: factor out gst_omx_try_importing_buffer() No semantic change, just make the code clearer and improve debug output.
Created attachment 373280 [details] [review] omxvideodec: don't import OMX buffers from downstream We already have code configuring the encoder stride and slice height when receiving the first buffer from upstream. We don't have an equivalent when the encoder is exporting its buffers to the decoder. There is no point adding it and making the code even more complex as we wouldn't gain anything by exporting from the encoder to the decoder. The dynamic buffer mode already ensures 0-copy between OMX components.
Branch also available here: https://gitlab.collabora.com/cassidy/gst-omx/commits/dma-export
Created attachment 373350 [details] [review] omx: allow gst_omx_port_acquire_buffer() to not wait for buffers Will be needed to implement GST_BUFFER_POOL_ACQUIRE_FLAG_DONTWAIT. Upstream-Status: Pending Signed-off-by: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk>
Created attachment 373351 [details] [review] omxvideoenc: implement dmabuf export on input buffers Propose pool upstream so input buffers can be allocated by the port and exported as dmabuf. The actual OMX buffers are allocated when the pool is activated, so we don't end up doing useless allocations if the pool isn't used.
Created attachment 373352 [details] [review] omxvideodec: don't import OMX buffers from downstream We already have code configuring the encoder stride and slice height when receiving the first buffer from upstream. We don't have an equivalent when the encoder is exporting its buffers to the decoder. There is no point adding it and making the code even more complex as we wouldn't gain anything by exporting from the encoder to the decoder. The dynamic buffer mode already ensures 0-copy between OMX components.
Review of attachment 373254 [details] [review]: Lgtm
Review of attachment 373255 [details] [review]: Lgtm
Review of attachment 373256 [details] [review]: Lgtm
Review of attachment 373257 [details] [review]: Lgtm
Review of attachment 373258 [details] [review]: Lgtm.
Review of attachment 373259 [details] [review]: Lgtm
Review of attachment 373260 [details] [review]: Lgtm.
Review of attachment 373261 [details] [review]: An intermediate, as I remember there is some state check added later. Lgtm.
Review of attachment 373262 [details] [review]: Lgtm.
Review of attachment 373262 [details] [review]: .
Review of attachment 373263 [details] [review]: Lgtm.
Review of attachment 373264 [details] [review]: Lgtm.
Review of attachment 373265 [details] [review]: Lgtm.
Review of attachment 373266 [details] [review]: Make sense.
Review of attachment 373268 [details] [review]: Just need to fix the commit here, "don't export non-dmabuf" ?
Review of attachment 373279 [details] [review]: Wow, I can read this code now !
Review of attachment 373350 [details] [review]: Please drop the Upstream-Status: and Signed-off-by:. It's fascinating when moving from one patch to the other to see how identical some of the baseclass code can be.
Review of attachment 373351 [details] [review]: This one I'll go through again tomorrowm but didn't found or think of anything wrong atm.
Review of attachment 373352 [details] [review]: Lgtm.
Review of attachment 373268 [details] [review]: No, that's the code trying to use a pool from downstream in the decoder; so it's an importation here. I did edit the commit message to: " Fix 'omxh264dec ! videocrop' pipeline."
Review of attachment 373350 [details] [review]: done. Yeah, loads of copy/paste between classes. :(
Review of attachment 373268 [details] [review]: Sorry, export was a typo of mine, I wanted not to be changed in non-, or just add the missing verb that allow using not.
Review of attachment 373268 [details] [review]: Ah ok! I changed it to: commit b1d4418eee8d8e0637d5f6847df08fcaf516ee0e Author: Guillaume Desmottes <guillaume.desmottes@collabora.co.uk> Date: Tue Jul 31 15:04:33 2018 +0200 omxvideodec: don't import non-dmabuf when dec is in dmabuf mode Fix 'omxh264dec ! videocrop' pipeline. https://bugzilla.gnome.org/show_bug.cgi?id=796918
Attachment 373254 [details] pushed as f0964df - omx: factor out gst_omx_port_set_dmabuf() Attachment 373255 [details] pushed as 998bfbb - omx: add pBuffer to OMX_PERFORMANCE logs Attachment 373256 [details] pushed as 7d3b0cd - omxvideoenc: factor out gst_omx_video_enc_deallocate_in_buffers() Attachment 373257 [details] pushed as 092788a - omxvideoenc: factor out gst_omx_video_enc_set_to_idle() Attachment 373258 [details] pushed as 457e1b9 - omx: factor out gst_omx_buffer_get/set_omx_buf() Attachment 373259 [details] pushed as 3b92b22 - omxvideo{enc,dec}: stop calling shutdown() in change_state Attachment 373260 [details] pushed as d2f7e21 - omx: call gst_omx_buffer_unmap() when handling BUFFER_DONE Attachment 373261 [details] pushed as 7b6be34 - omxbufferpool: deallocate OMX buffers when stopping Attachment 373262 [details] pushed as 8efa095 - turn GstOMXComponent to a GstMiniObject Attachment 373263 [details] pushed as 86a6703 - omxbufferpool: reference the OMX component Attachment 373264 [details] pushed as 0996019 - omxvideodec: prevent timeout when shutting down because of pending out buffers Attachment 373265 [details] pushed as 9ed1c2e - omxvideodec: fix pool caps reference stealing Attachment 373266 [details] pushed as 7d8a314 - omxvideodec: fix gst_video_info_from_caps() caps assertion Attachment 373279 [details] pushed as be5ec66 - omxvideodec: factor out gst_omx_try_importing_buffer() Attachment 373350 [details] pushed as 34bc02e - omx: allow gst_omx_port_acquire_buffer() to not wait for buffers Attachment 373351 [details] pushed as 7be54ad - omxvideoenc: implement dmabuf export on input buffers Attachment 373352 [details] pushed as f108eea - omxvideodec: don't import OMX buffers from downstream
Hi Guillaume, nice work! Just to confirm, this only works for Zynq right ? Mostly because of the specific flags/struct described here https://bugzilla.gnome.org/show_bug.cgi?id=726325#c48 . Adding generic/similar flags in Tizonia would allow to use 99% of these patches right ? (not accounting the work to make it work on the component side, i.e in mesa/omx)
Yes, I only enabled it only on the Zynq. Indeed, I think the only extension API used here is the same one as when producing dmabuf. Implement it and you should be able to re-use export and import modes.