GNOME Bugzilla – Bug 722686
omxvideodec: integrate resize component
Last modified: 2018-05-07 15:42:42 UTC
Created attachment 266858 [details] [review] omxvideodec: integrate resize component on RPI It enables the use of ximagesink on RPI. Or any video sink that does not use GL on RPI. Total CPU load at 20% for a 640x360 video. Total CPU load at 60% for 1280x720 video. (if you do not move the mouse :) ) The OMX.broadcom.resize component does colorspace conversion. So integrating it allows to support more downstream elements (or make them usable because it drastically reduces CPU load) Ex: Allow to output RGB16 if downstream does not support I420 and if "video_decode" can only output I420. Only available on RPI (passing --with-omx-target=rpi to the configure script) because I only tested on RPI and also because I'm sure if the resize element is available on other platform. And for now it's instantiated using its name directly: "OMX.broadcom.resize" But you does not necessarly need to have GST_EGL as it's the case to use "OMX.broadcom.egl_render" First, omxvideodec tries "egl_render" (if egl available) then if it fails it directly tries "video_decode" as useall but then if it fails again it tries "resize" with a compatible downstream colorformat. When trying the resize component it try to do OMX_UseBuffer (useful to use downstream XImage create using XShmCreateImage) and if it fails to actually use those downstream buffers then it fallbacks to OMX_AllocateBuffer. Ex: Allow to have zero-copy with omxh264dec ! ximagesink (OMX_UseBuffer) but omxh264dec ! tee ! ximagesink will do OMX_AllocateBuffer because tee does not forward the allocation query. example: https://drive.google.com/file/d/0Bw6t368wtIMYS1ZOOTF2REFKdkk
Another result: https://docs.google.com/file/d/0Bw6t368wtIMYNlhNRkd5MU04TlE
Created attachment 266989 [details] [review] omxvideodec: integrate resize component on RPI Improved negotiation logic when the resize component is available. So that when using playbin the decoder has a chance to exactly select the format that the video sink actually supports.
Could be good to do a gst-omx release before adding that. Also last release was almost 1 year ago.
We also have a 1.0 branch to make another release from if needed.
The master branch is compatible with -core and -base 1.2 (not tried with 1.0) So maybe we could create a 1.2 branch from master and make a 1.2 release ?
I keep the resize stuff updated here http://cgit.collabora.com/git/user/julien/gst-omx.git/log/?h=resize
(In reply to comment #5) > The master branch is compatible with -core and -base 1.2 (not tried with 1.0) > > So maybe we could create a 1.2 branch from master and make a 1.2 release ? No, because the differences between 1.0 and master depend on API that is not available yet. We could do a new 1.0 release with more backported fixes though.
Created attachment 268926 [details] [review] omxvideodec: integrate resize component on RPI Now also supports video scaling (before it supported colorspace conversion only)
Created attachment 268928 [details] [review] omxvideodec: populate the most downstream output port on reset make seeking work
Created attachment 268930 [details] [review] omxvideodec: catch error when calling gst_omx_port_populate in reset
Created attachment 268931 [details] [review] omx: only block upstream on flushing
Created attachment 268932 [details] [review] omxvideodec: use flush because reset is deprecated
Created attachment 268933 [details] [review] omxvideodec: if no frames found do not pause the task if src pad is flushing
Review of attachment 268926 [details] [review]: What I've seen are mostly structure or cosmetic issues that could be improved in order to keep the code more readable. It seems to me that tunneling related code is correct. ::: omx/gstomxvideodec.c @@ +315,3 @@ + use_resizer = GST_OMX_VIDEO_DEC (pool->element)->use_resizer; +#else + use_resizer = FALSE; This #else case seems not necessary if you already initialize use_resizer to FALSE. @@ +334,2 @@ } GST_OBJECT_UNLOCK (pool); Maybe would be better add a gboolean is_raw and just have a single GST_OBJECT_UNLOCK; return (is_raw ? raw_video_options : options); @@ +350,3 @@ + use_resizer = GST_OMX_VIDEO_DEC (pool->element)->use_resizer; +#else + use_resizer = FALSE; This #else case seems not necessary if you already initialize use_resizer to FALSE. @@ +915,3 @@ + out_port_index = param.nStartPortNumber + 1; + } + GST_OMX_INIT_STRUCT (¶m); Regarding the previous block, maybe would be better introduce a helper function to be used in both use cases egl_render/resize. @@ +1468,3 @@ #else port = self->dec_out_port; #endif Maybe would be better to initialize port = self->dec_out_port; first and remove all else cases. @@ +1852,3 @@ + gst_omx_error_to_string (err), err); + goto done; + gst_omx_error_to_string (err), err); Previous error message refer to "resize output port" but I've the feeling there's a missing condition or that this code could be triggered for non resie output port. Maybe it should be refactored into a helper function. @@ +1956,3 @@ +#else + bufsize = self->dec_out_port->port_def.nBufferSize; +#endif maybe better to initialize bufsize = self->dec_out_port->port_def.nBufferSize; and simplify this conditional block to: #if defined (USE_OMX_TARGET_RPI) if (self->use_resizer) bufsize = self->res_out_port->port_def.nBufferSize; #endif @@ +2024,1 @@ #endif Maybe better first initialize to port = self->dec_out_port; and remove the else cases. @@ +2470,3 @@ +static gboolean gst_omx_video_dec_negotiate (GstOMXVideoDec * self); + Maybe better move the forward declaration to the top. @@ +2494,3 @@ #else port = self->dec_out_port; #endif Simplify this like previous ones mentioned. @@ +2903,1 @@ #endif Maybe add an intermediate sec_in_port/sec_out_port/sec_component (sec as secondary component) and keep a single #if block and avoid duplicate code and add conditionals. @@ +2920,1 @@ #endif Avoid conditionals if intermediate variable is added for component above. @@ +2940,1 @@ #endif Avoid conditionals if intermediate variable is added for component above. @@ +3410,3 @@ #else GstOMXPort *out_port = self->dec_out_port; #endif Initialize first to GstOMXPort *out_port = self->dec_out_port; and simplify the conditional here. @@ +3530,3 @@ + + gst_omx_component_set_state (self->dec, OMX_StateExecuting); + gst_omx_component_get_state (self->dec, 1 * GST_SECOND); Maybe better add a helper function for this block instead of duplicate it.
Comment on attachment 268928 [details] [review] omxvideodec: populate the most downstream output port on reset Handled by https://bugzilla.gnome.org/show_bug.cgi?id=726038
Comment on attachment 268930 [details] [review] omxvideodec: catch error when calling gst_omx_port_populate in reset Handled by https://bugzilla.gnome.org/show_bug.cgi?id=726038
Comment on attachment 268932 [details] [review] omxvideodec: use flush because reset is deprecated Handled by https://bugzilla.gnome.org/show_bug.cgi?id=726038
Comment on attachment 268931 [details] [review] omx: only block upstream on flushing Josep told me that this one is actually not correct
Comment on attachment 268933 [details] [review] omxvideodec: if no frames found do not pause the task if src pad is flushing This one was just a work-around and the real pb has been fixed in commit 73d83f311c23429f86fc2377f2b2828db5af9898, see https://bugzilla.gnome.org/show_bug.cgi?id=726038
As the plan is to add tunnelling support https://bugzilla.gnome.org/show_bug.cgi?id=726325, I think OMX.broadcom.resize should be put in its one gstelement (omxcolorscale ? as it does scaling and colorspace conversion). But then what about using it automatically through playbin ? I am thinking about 2 solutions: 1 * selectable videoscale / videoconvert based on ranks: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/playback/gstplaysinkvideoconvert.c#n63 Does it require to have a GstBaseVideoScale base class ? So that playbin can use the most ranked scaler element. 2 * having a omxvideodecbin that is a GstBin with omxvideodec and omxcolorscale in-there and linked together. (This way videoconvert ! videoscale will work in passthrough) 3 * using current "video-sink" property of playbin and just doing manually: video-sink="omxcolorscale ! videosink" What do you think ? Other ideas ? Same questions about OMX.broadcom.egl_render. In the mean time to have tunnelling support what do you think about pushing the attached patch ? (I'll soon rebase it on current master) Because it's confined in "#if defined (USE_OMX_TARGET_RPI)" blocks so that when tunnelling will be there it will not be difficult to remove it (considering the on going dev during this time frame).
Woul it be possible to open a seperate bug about that, it's also relevant for me, in v4l2.
One thing you should notice: This will SW convert: omxdec ! omxresize ! videoconvert ! rgbsink While this will HW convert: omxdec ! vieoconvert ! omxresize ! rgbsink
(In reply to comment #20) > As the plan is to add tunnelling support > https://bugzilla.gnome.org/show_bug.cgi?id=726325, I think OMX.broadcom.resize > should be put in its one gstelement (omxcolorscale ? as it does scaling and > colorspace conversion). Yes, that would be better. So let's WONTFIX this bug here? > But then what about using it automatically through playbin ? I am thinking > about 2 solutions: > > 1 * selectable videoscale / videoconvert based on ranks: > http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/playback/gstplaysinkvideoconvert.c#n63 > Does it require to have a GstBaseVideoScale base class ? > So that playbin can use the most ranked scaler element. Going from the element factory classification should be enough. Just needs to be implemented, and especially need to autoplug a converter that handles the relevant caps. > 2 * having a omxvideodecbin that is a GstBin with omxvideodec and > omxcolorscale in-there and linked together. (This way videoconvert ! videoscale > will work in passthrough) Also acceptable, but 1) should definitely be implemented. > 3 * using current "video-sink" property of playbin and just doing manually: > video-sink="omxcolorscale ! videosink" > > What do you think ? Other ideas ? Same questions about OMX.broadcom.egl_render. I think egl_render would be good to keep inside the decoder. It's uncommon that this is a separate component (other OMX implementations just allow OMX_UseEGLImage() on the decoder ports) and adding generic autoplugging support for that seems unlikely to happen.
Ok thx for you answers. I'll keep the bug open and just rename it when I start the omxcolorscale that will wrap any component like OMX.broadcom.resize
As it doesn't look like anybody wants to work on implementing proper tunneling and it will probably need quite some time to implement properly, maybe we should integrate these patches nonetheless. But maybe by having some internal helper structures for setting up tunnels so that it doesn't become an unmaintainable mess. Julien, would you be interested in updating your patch to latest gst-omx and refactor the tunnel setup (or is that done already)? I'm thinking of something that doesn't require us to keep track of all the components of the tunnel all the time and remember which is the first and last component of the tunnel.
Hi Sebastian, what you have suggested is great! I would be very interested but I don't have enough time to properly do it. (and no rpi right now but that's a minor pb) I can point where is the latest code: All the commits from "omxvideodec: integrate resize component on RPI" to the head commit : http://cgit.collabora.com/git/raspberry-pi/gst-omx.git/log/?h=rpi-1.0.0.1 Josep already started to squash some commits, addressed some remarks from comment #14, and more cleanup. His latest work is here: http://cgit.collabora.com/git/user/julien/gst-omx.git/commit/?h=resize_josep_last&id=50a954824a8a7770b53961efff06886d3d6815a5 1: The first step is to start from the commit "omxvideodec: integrate resize component on RPI" in "resize_josep_last" branch. And merge commits from branch rpi-1.0.0.1 into branch "resize_josep_last" and squash them. 2: Second step is to apply what you suggested, i.e. some helpers to setup a tunnel to avoid duplicating code. If it's easier, note that the tunnel has minimum 1 element and maximum 2 elements in our case. - Josep do you have time to do step1 ? - Sebastian do you think we should consider that we only have 1-2 elements, or the general case N ? Also in our case, the decoder component is always the first one, we just try a list of element (egl, resize) to tunnel with the decoder. For the resize component we add it only if the decoder cannot do it itself. For the eglimage component it's different, we always try at the beginning because it's more efficient. But once the second element is decided, the code is very similar.
You already have 3 components with video_dec, resize and egl_render. I think it should be generic for N components.
Just a quick ping, is there any progress on this? The http://cgit.collabora.com/git/raspberry-pi/gst-omx.git/log/?h=rpi-1.0.0.1 branch shows recent work, but it's not fully clear whether all the fixes that appear on the official gst-omx also appear on this branch.
(In reply to minfrin from comment #28) > Just a quick ping, is there any progress on this? > The only small update is that I extracted the part that add support for OMX_UseBuffer and I submitted here https://bugzilla.gnome.org/show_bug.cgi?id=784069 . In the branch you pointed it was hidden in the commit that integrates the rpi's resize component. But I do not plan to more on that particular subject so anyone feel free to go ahead. > The > http://cgit.collabora.com/git/raspberry-pi/gst-omx.git/log/?h=rpi-1.0.0.1 > branch shows recent work, but it's not fully clear whether all the fixes > that appear on the official gst-omx also appear on this branch. I checked and some commits are already merged but all the last 10 or 15 commits are not if I am not mistaken. I added Thibault in CC.
There does not seem to be any progress on this and nobody working on taking this up, so let's close this for now. Feel free to reopen if someone wants to work on this again.