GNOME Bugzilla – Bug 726107
omxvideodec: Drain pipeline to support adaptive streaming scenarios and partially fix gpu resource leaks
Last modified: 2018-05-09 16:32:36 UTC
At http://cgit.freedesktop.org/~adn770/gst-omx/log/?h=drain can be found a patch that allow for reconfiguration in _set_format changes when used in testegl. Also it's provided another patch that take care of release the egl images.
I don't know that you did bugreport here This pipeline don't work gst-launch-1.0 uridecodebin uri="file:///home/pi/test2/ar_problem/test_ar_problem2.ts" use-buffering=false name=demux caps="video/mpeg, parsed=true; audio/x-raw; text/x-raw" \ ! video/mpeg, parsed=true \ ! omxmpeg2videodec \ ! omxh264enc target-bitrate=2000000 control-rate=1 ! video/x-h264, profile="high" ! fakesink sync=true \ demux. ! audio/x-raw ! fakesink http://www.mdragon.org/test_ar_problem2.ts
Try with fakesink enable-last-sample=FALSE. Other changes are required at GstBaseSink to handle the drain query. My current test case is by using the testegl example were I've also added the drain query handling.
I've discussed with Jan during the hackfest, and DRAIN query was not designed for this purpose. Wim has proposed to introduce a RECLAIM event, or some more generic mechanism, where the buffers would be drained and copied, in order to release them as fast as possible. Refer to: https://bugzilla.gnome.org/show_bug.cgi?id=682770
Sure, but for the time being this is the only way to ensure that buffers are released before deallocate them. And it's in the line of http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=093574053fae243ec5fcfd6adae8185053b1e896 I think that we can adopt this change as an interim solution until there's a new API to handle the use case in a better way. (In reply to comment #3) > I've discussed with Jan during the hackfest, and DRAIN query was not designed > for this purpose. Wim has proposed to introduce a RECLAIM event, or some more > generic mechanism, where the buffers would be drained and copied, in order to > release them as fast as possible. > > Refer to: https://bugzilla.gnome.org/show_bug.cgi?id=682770
Created attachment 272863 [details] [review] Use drain query to reclaim memory back and wait
(In reply to comment #4) > I think that we can adopt this change as an interim solution until there's a > new API to handle the use case in a better way. I'm more inclined to working out the already started solution. How good is the transition with the drain query right now ? Do you see a gap, or a small image delay ?
Review of attachment 272863 [details] [review]: ::: omx/gstomxvideodec.c @@ +1447,3 @@ if (self->draining) { +#if defined (USE_OMX_TARGET_RPI) && defined (HAVE_GST_EGL) + if (self->eglimage) { Why is this all RPI specific ? Don't we have buffer pools in general with OMX ? @@ +1449,3 @@ + if (self->eglimage) { + gst_pad_peer_query (GST_VIDEO_DECODER_SRC_PAD (self), + gst_query_new_drain ()); That is a bit weird, why would you want to reclaim you buffers before you have drained your decoder ?
There a more complications with a new oob event to reclaim buffers. If the new event is non-serialized, it would be very racy if we don't do something clever. You would need to make a start/stop event, like how the flushing works or you would need to try to serialize in the handler (a bit like how the logic of flush-stop works). It needs more thinking..
(In reply to comment #6) > I'm more inclined to working out the already started solution. How good is the > transition with the drain query right now ? Do you see a gap, or a small image > delay ? Few (2-3) frames are dropped because they don't arrive in time, but I believe that a similar behaviour will happen when the queue will copy the buffers, as it might consume some cpu time, in reaction to the RECLAIM event. Although that the behaviour of loosing some frames is expected for adaptive scenarios like MS smooth streaming (I believe that I've read some references to this fact in the spec). (In reply to comment #7) > Review of attachment 272863 [details] [review]: > > ::: omx/gstomxvideodec.c > @@ +1447,3 @@ > if (self->draining) { > +#if defined (USE_OMX_TARGET_RPI) && defined (HAVE_GST_EGL) > + if (self->eglimage) { > > Why is this all RPI specific ? Don't we have buffer pools in general with OMX ? Well this is an initial patch, some more changes must come for ximagesink/eglglessink to handle the drain query and copy the buffer for reexpose. > > @@ +1449,3 @@ > + if (self->eglimage) { > + gst_pad_peer_query (GST_VIDEO_DECODER_SRC_PAD (self), > + gst_query_new_drain ()); > > That is a bit weird, why would you want to reclaim you buffers before you have > drained your decoder ? Here we already drained the decoder. In omx video dec the decoder is drained by inserting an EOS tag on the input and waiting for it the the output port and keeping a boolean at element level to distinguish between drains and real EOS.
(In reply to comment #8) > There a more complications with a new oob event to reclaim buffers. > > If the new event is non-serialized, it would be very racy if we don't do > something clever. You would need to make a start/stop event, like how the > flushing works or you would need to try to serialize in the handler (a bit like > how the logic of flush-stop works). It needs more thinking.. I did foresee that. Though I was to propose to retract on doing it out of band. My idea is that the event should hold the buffer pool it's aiming at. If an element does not know what buffer pool it's using, it would just drain, but as soon as an element knows it would end the chain. This would return sooner and increase the chance we don't loose frames. Also, for threaded decoders, it would make more sense to run the reclaim (or drain) from the thread the push buffers out. This way you can start queuing frame for decoding sooner.
First I've wanted to use testegl as a test bed for this approach as a first step before introduce changes in the other sink elements. My primary goal is be able to handle adaptive streaming scenarios in the RPI with 1.4 GStreamer series and the RECLAIM event might not be available until 1.6, that's the reason why I wanted to explore with the current available tools although it might not be the most perfect solution. Right now there's multiple issues linked to this missing reclaim step. And a GPU resource leak that I'm also investigating which is related to this set of scenarios.
Created attachment 272886 [details] [review] testegl: keep a ref on the buffer instead of the memory So that it does like a real videosink, like eglglessink
Created attachment 272887 [details] [review] eglglessink: release buffer ref on DRAIN
The reason I'd prefer we make a blocker of it, is that I have the same issue in v4l2 decoder (and many buffer pools that cannot detach from it's buffers). Obviously we can probably improve overall support of drain query and just make it work. Let's try and talk about that on IRC soon and not do our own little thing in the corner. The basic idea is that, the query should stop as soon as there is a stream pool change. E.g. a encoder will have two pools, when it receive the drain query, it should drain anything related to it's input pool but there is no point in forwarding. Though as said, drain was not designed to do that, and being a query is just a non-sense to me.
I agree with all of you :) Also I confirm that with all the attached patches it works. (also thanks to http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=093574053fae243ec5fcfd6adae8185053b1e896 or either set enable-last-smaple to false) Also I guess this way mostly works for omxvideodec because it calls gst_omx_port_wait_buffers_released, so after sending the query it actually waits some times for the buffers to be actually back into the pool.
Created attachment 273086 [details] [review] examples: fix several memory leaks in the testegl example Fixes slice, EGLImage and GLTexture leaks.
Review of attachment 272886 [details] [review]: This patch is correct.
Created attachment 273143 [details] [review] omxvideodec: Implement pipeline draining to support adaptive scenarios As was required use the drain query to reclaim memories back in all cases and not only for RPI/EGL scenario.
Review of attachment 272887 [details] [review]: I've tested it and it works. I don't have anything to object on the code itself.
Review of attachment 273086 [details] [review]: Looks good !
Review of attachment 273143 [details] [review]: Looks good !
I validated the 4 attached patches with the speedway_raw.h264 file available here https://drive.google.com/file/d/0Bw6t368wtIMYNFVsdXZheVd6Y00/edit?usp=sharing. ./testegl file:///home/pi/speedway_raw.h264 gst-launch-1.0 filesrc location=speedway_raw.h264 ! h264parse ! omxh264dec ! queue ! eglglessink
Also note that after discussions with other maintainers it has been decided that the DRAIN query is the current solution to reclaim buffers, as there is no enough time to introduce a new API before the 1.4 release.
commit 718fd1bb93a09137a139d050e537f07daaf654b3 Author: Josep Torra <n770galaxy@gmail.com> Date: Mon Mar 10 17:43:50 2014 +0100 omxvideodec: Implement pipeline draining to support adaptive scenarios When draining due a format change also drain the pipeline to reclaim back all buffers. https://bugzilla.gnome.org/show_bug.cgi?id=726107 commit 5724df75231e83c510b00c851ae25aa5024e5798 Author: Josep Torra <n770galaxy@gmail.com> Date: Thu Mar 27 13:57:32 2014 +0100 examples: fix several memory leaks in the testegl example Ensure to call to image_data_free in order to release GPU resources. Also ensure to destroy EGLImage and GLTexture from proper thread/context. https://bugzilla.gnome.org/show_bug.cgi?id=726107 commit 6995b4d5b344a1d17de9e06245786621eae26bc4 Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Tue Mar 25 17:16:31 2014 +0000 examples: keep a ref on the buffer instead of the memory Like in eglglessink https://bugzilla.gnome.org/show_bug.cgi?id=726107
Comment on attachment 272887 [details] [review] eglglessink: release buffer ref on DRAIN commit 7dd3b2fff78c9b35dcae553d062678326905a950 Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Fri Mar 28 11:54:45 2014 +0000 eglglessink: unref last buffer on DRAIN Similar to 093574053fae243ec5fcfd6adae8185053b1e896 in gstbasesink https://bugzilla.gnome.org/show_bug.cgi?id=726107
Review of attachment 273143 [details] [review]: Some suggestion. ::: omx/gstomxvideodec.c @@ +1448,3 @@ + /* Drain the pipeline to reclaim all memories back to the pool */ + gst_pad_peer_query (GST_VIDEO_DECODER_SRC_PAD (self), + gst_query_new_drain ()); Considering drain is expensive, maybe we should check if it's needed first ? We should also check the buffer do have come back afterward, and probably fail cleanly if it didn't. I suspect many element don't handle drain at all, and may not give back certain buffers kept as observation.
Do we know now why this drain query is required? In my platform this drain query takes almost 200+ms and it increases EOS handling time of pipeline. Without this drain query i dont see any issue. So can i remove this query? Any suggestion?
Without that, renegotiation will fail,or worst, buffers will be freed in omx stack back. If it works for you, it means zero copy does not work for you. Though, using drain query instead of just an allocation query is overkill. Some work on the memory wrapper is needed to properly fix this.