GNOME Bugzilla – Bug 733614
Zero copy path between omxvideodec and v4l2sink
Last modified: 2016-04-16 14:56:01 UTC
Created attachment 281490 [details] [review] omxvideodec: Try adjusting downstream buffer pool if requirements are not met I try to have a zero-copy path between omxvideodec and v4l2sink using latest v4l2 user pointer implementation. e.g: filesrc ! demux ! parse ! omxvideodec ! v4l2sink io-mode=3 By default, it doesn't work because omxvideodec doesn't export his buffers and try to copy frame from his internal buffers to v4l2sink buffers. Since there is no memory allocated by v4l2 element in user pointer mode, it just fails. omxvideodec doesn't export its buffers because during negotiation/alloc_output_buffer, in my case, v4l2 buffer pool min-buffers and max-buffers are equal to 3 and my OMX component output port requires at least 6 buffers. See http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n581 For information, my v4l2sink buffer pool min and max are set to 3 because my component can't allocate buffers since my device doesn't support CREATE_BUFS yet. I currently found a solution to my problem, but I'm not sure it is the best way to handle this. I attached it, it consists in adjusting proposed pool parameters in decide_allocation().
Created attachment 281491 [details] Trace omxvideodec -> v4l2sink io-mode=3 Debug message log without any patches
omxdec shouldn't reject the pool because it's proposed setting is 3,3. Instead, it should negotation with the pool by doing changes to the config that would satisfied it's needs. Then set this config on the pool. If the pool isn't happy, it will return FALSE. You can then check what it would accept instead, by getting the config again. There is a helper to quickly check if the pool changes fits your needs, see gst_buffer_pool_config_validate_params(). Obviously, a v4l2 pool where CREATE_BUFS isn't supported, will always inforce that min==max.
Created attachment 281551 [details] [review] omxvideodec: try adjusting downstream buffer pool if requirements are not met It will try to increase pool min-buffers if it doesn't match port mBufferCountMin.
Review of attachment 281551 [details] [review]: ::: omx/gstomxvideodec.c @@ +2597,3 @@ + min = port->port_def.nBufferCountMin; + if (max < min) + max = min; Max could be 0. If you don't want to care, just set max to 0 here. Anyway, this patch is incomplete. Element that need to change the config need to do it before chaining to it's base class. Then it should update the allocation query with the new negotiated min/max, so the baseclass don't undo what have been done. It's also really odd that this implementation just blindly enable video meta, but does not remember if it's actually missing downstream. In presence of customer stride and no downstream video meta, it is likely that you want to copy the frames to a standard stride (gst_video_frame_copy() does that). Have a look at base classes, and other element propose_allocation.
You mean decide_allocation(), right ? I take a look at other decoders (libav and theora) and they chain to base class before changing the config and updating the allocation query. But I think, the main issue is that I am not sure the omxvideodec decide something in decide_allocation. Currently it only supports OMX_AllocateBuffer() so it will always use its's internal buffers with the underlying OMX component. I think it really decide to create a bufferpool and exports its buffer in gst_omx_video_dec_allocate_output_buffers() but only if it matches with proposed bufferpool. Currently, if downstream pool doesn't match or doesn't support video meta, omxvideodec copy the frame.
Created attachment 281962 [details] [review] omxvideodec: try adjusting buffer pool config if requirements are not met Updated with max=0 and allocation_pool update
Review of attachment 281962 [details] [review]: ::: omx/gstomxvideodec.c @@ +2607,3 @@ GST_BUFFER_POOL_OPTION_VIDEO_META); } gst_buffer_pool_set_config (pool, config); This is still not right, You need to check the return value, otherwise the pool is not yet configured, and the saved config might have been changed in a way that it does not work for our use case.
For the reference, from the doc: "If the parameters in config can not be set exactly, this function returns FALSE and will try to update as much state as possible. The new state can then be retrieved and refined with gst_buffer_pool_get_config()."
Created attachment 282072 [details] [review] omxvideodec: try adjusting buffer pool config if requirements are not met I update it to check set_config, If set_config adjust our config but remains suitable for our need, we will later be able to export OMX buffers. Else we will get the pool but use copy as previous.
Also, I don't test it on RPI (my sdcard died :( ), so for now, I think it should not be pushed anyway,
Review of attachment 282072 [details] [review]: Looks already better, there is still some oddness in the way it fallback to copy. Would it be possible to clarify ? ::: omx/gstomxvideodec.c @@ +2614,3 @@ + if (!gst_buffer_pool_config_validate_params (config, caps, size, min, max)) { + GST_DEBUG_OBJECT (dec, + "can't configure pool to suit our needs, use copy"); Is it intentional to fallthrough config_failed here ?
Yes, goto config_failed here will lead decide_allocation to return FALSE and videodec baseclass returns in error leading to a negotiation failure. But omxvideodec, for now, only use its internal buffers which are allocated by the OMX component. So if buffer pool don't suit it, it will not push its buffer. Instead it will acquire buffer from pool and copy frame to it, Since I don't know if I'm really clear and because I have some difficulties to understand the whole picture, you can take a look at these pieces of code: gst_omx_video_dec_allocate_output_buffers() http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n545 we will check current videodecoder pool and if it's good create an internal bufferpool http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n624 gst_omx_video_dec_loop() http://cgit.freedesktop.org/gstreamer/gst-omx/tree/omx/gstomxvideodec.c#n1436 If we have the internal pool (out_port_pool), we will push GstOMXBuffer, else we will try to allocate an output frame. However, in case of gst_buffer_pool_config_validate_params failure, it could be better to create a GstVideoBufferPool. A better approach may be to refactor/rework the omxvideodec pool management. For instance, omxvideodec can always have a internal buffer pool and decide to push buffer from it at decide_allocation time based on what downstream support, like the v4l2 plugins. However, they can be some issue with OMX layer, so it can be intersting to know why it has been done in such way.
I realise my instructions my not be that clear either. I'll try and find time to poke at this, though for my part I only have an RPi, which means without a v4l2sink.
Created attachment 282425 [details] [review] omxvideodec: try adjusting buffer pool config if requirements are not met Update log message to fix a format warning. I also test it on RPi and I have seen no regression.
(In reply to comment #13) > I realise my instructions my not be that clear either. I'll try and find time > to poke at this, though for my part I only have an RPi, which means without a > v4l2sink. No problem, and thanks for your patience. I also know I was not clear either.
Friendly ping :-)
Review of attachment 282425 [details] [review]: Looks good overall, except the fallthrough, but maybe you have a good reason, let me know. ::: omx/gstomxvideodec.c @@ +2614,3 @@ + if (!gst_buffer_pool_config_validate_params (config, caps, size, min, max)) { + GST_DEBUG_OBJECT (dec, + "can't configure pool to suit our needs, copy to downstream pool"); I think we should fail at this point. Otherwise the corrected (but not acceptable) config will be set on line 2619, which will pretty much always succeed, and lead to pipeline that most of the time will stall rather then fail.
(In reply to comment #17) > Review of attachment 282425 [details] [review]: > > Looks good overall, except the fallthrough, but maybe you have a good reason, > let me know. > > ::: omx/gstomxvideodec.c > @@ +2614,3 @@ > + if (!gst_buffer_pool_config_validate_params (config, caps, size, min, > max)) { > + GST_DEBUG_OBJECT (dec, > + "can't configure pool to suit our needs, copy to downstream pool"); > > I think we should fail at this point. Otherwise the corrected (but not > acceptable) config will be set on line 2619, which will pretty much always > succeed, and lead to pipeline that most of the time will stall rather then > fail. In fact, I don't think so, because omxvideodec always use OMX buffers. If provided pool doesn't suit, instead of giving its internal buffer, it will acquire one from pool and copy frame to it. See comment #12 for more details.
(In reply to comment #18) > (In reply to comment #17) > > I think we should fail at this point. Otherwise the corrected (but not > > acceptable) config will be set on line 2619, which will pretty much always > > succeed, and lead to pipeline that most of the time will stall rather then > > fail. > > In fact, I don't think so, because omxvideodec always use OMX buffers. If > provided pool doesn't suit, instead of giving its internal buffer, it will > acquire one from pool and copy frame to it. See comment #12 for more details. Oh I see. Do OMX uses a buffer pool ? can it allocate buffers on the fly or do it need to pre-allocate everything ? In such case, we should see something like: Check if omx internal buffers require video meta and if downstream support video meta. If we do need video meta to be added, and downstream don't support it, enable copy mode. Check if oxm internal pool min/max can be adjusted to fit downstream min + omx min. If it cannot be adjusted, copy mode. If not in copy mode, replace the pool in the query with your own, otherwise downstream pool maybe initialized for no reason. If in copy mode, set the config with value from downstream (you don't need extras) and leave it in the query. You'll have to initialize your own pool yourself. Whenever you get buffers from you own pool ready, acquire from downstream pool, copy, and push the copy.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > I think we should fail at this point. Otherwise the corrected (but not > > > acceptable) config will be set on line 2619, which will pretty much always > > > succeed, and lead to pipeline that most of the time will stall rather then > > > fail. > > > > In fact, I don't think so, because omxvideodec always use OMX buffers. If > > provided pool doesn't suit, instead of giving its internal buffer, it will > > acquire one from pool and copy frame to it. See comment #12 for more details. > > Oh I see. Do OMX uses a buffer pool ? can it allocate buffers on the fly or do > it need to pre-allocate everything ? Yes, buffer pool and you need to pre-allocate. And if you want to change anything you need to deallocate all buffers and allocate new ones. > In such case, we should see something like: > > Check if omx internal buffers require video meta and if downstream support > video meta. If we do need video meta to be added, and downstream don't support > it, enable copy mode. > > Check if oxm internal pool min/max can be adjusted to fit downstream min + omx > min. If it cannot be adjusted, copy mode. You can only check this by trying to set the maximum btw
Why not set io-mode to 2 (i.e mmap) and tell omx to use those buffers (i.e OMX_UseBuffer) ? For a start you can have a look at http://cgit.collabora.com/git/raspberry-pi/gst-omx.git/log/?h=rpi-1.0.0.1 The commit named "omxvideodec: integrate resize component on RPI" adds support for OMX_UseBuffer. Well this commit also adds support for the broadcom resize component, which you does not need here. I should have split it into at least 2 patches. This way it was possible to tell OMX to use the XShmImage(s) allocated by the downstream ximagesink's bufferpool. This is similar to OMX_UseEGLImage btw.
OMX_UseBuffer() is completely useless API, like the USERPTR API in v4l2. You can pass any type of memory in there but the port can reject it, and you have no idea what kind of memory the port actually wants. I also some OpenMAX implementations where no system memory pointer was passed to OMX_UseBuffer() but handles (think of file descriptors).
(In reply to comment #0) > e.g: filesrc ! demux ! parse ! omxvideodec ! v4l2sink io-mode=3 (In reply to comment #22) > OMX_UseBuffer() is completely useless API, like the USERPTR API in v4l2. Hi, I thought the subject here was about USERPTR, i.e io-mode=3 (http://cgit.freedesktop.org/gstreamer/gst-plugins-good/tree/sys/v4l2/gstv4l2object.h#n57)
(In reply to comment #22) > OMX_UseBuffer() is completely useless API, like the USERPTR API in v4l2. You > can pass any type of memory in there but the port can reject it, and you have > no idea what kind of memory the port actually wants. > > I also some OpenMAX implementations where no system memory pointer was passed > to OMX_UseBuffer() but handles (think of file descriptors). I must say this is a slight contradiction. Julien and I have used USERPTR successfully on the Cotton Candy with the exact same approach (except we had v4l2videodec instead of OMX dec). It obviously depends on the driver being able to handle the type of memory produced by previous element, hence is dedicated to controlled environment. What I understand of this bug is that omx decide_allocation is broken, and similarly to v4l2, this is really complex as there is many details that need to be considered. In this mode, v4l2 should probably not offer a pool, but omx dec should read the pool configuration as a something to be added to it's own requirement. I think my last review described this well.
As pointed out by Sebastian, UseBuffer will mainly depend of the component and client behavior. For instance Android passes a NativeBuffer handler rather than a memory pointer. Also we can't be sure with UseBuffer or v4l2 userptr that it will works in all cases, that's why I think they shall not be used as default but just in a specific environment. By the way, I think the main issue is about the omx bufferpool, push/copy mode and the fact that omxvideodec doesn't set its own bufferpool in decide_allocation. When I have some time, I will try to poke at this :)
Any update here ?
Unfortunately, I didn't have time to work at this and I don't think I will have some in the near future.
Closing this bug report as no further information has been provided. Please feel free to reopen this bug report if you can provide the information that was asked for in a previous comment. Thanks!