GNOME Bugzilla – Bug 693772
avdec: decoder frame list getting long
Last modified: 2013-12-13 13:39:22 UTC
There is a possible decoder leak in gst-libav. With first test file, the number of "leaked" frames seems to be capped, it could be just decoder not giving any output with empty VOPs at the beginning of file. With second file, the numbers keeps increasing. https://s3.amazonaws.com/MatejK/test1.avi https://s3.amazonaws.com/MatejK/test2.avi
I can confirm this. This should be a) fixed somehow in libav/gst-libav and b) this warning in the video decoder base class should be a bit more clever in general :)
I was going to run through all the decoders today and force "parsed=true" on the sink caps. That should fix nearly everything. I'll also write a better WARNING message.
Well, with these files you get the exact same result with mpeg 4 parser before decoder.
FIY, I tried h.264 decoder on windows and it needs 16 frames in before it starts pushing anything out. I really think this message should be configurable at least.
On RPi there's an internal queue in the codecs that allows up to 100 compressed frames... so the list would grow up to that size on RPi without any problems.
Degraded this warning to a debug output for now until someone has a better idea. commit bd62595a758d63544f09e45d9f8d59804ffc20e0 Author: Sebastian Dröge <slomo@circular-chaos.org> Date: Tue Jun 4 17:49:55 2013 +0200 videodecoder: Change GST_WARNING to a GST_DEBUG It's completely normal for some decoders to queue 50-60 frames without it causing any problems, e.g. RPi. Nonetheless with the files above there's a bug in gst-libav
Looks like we can be piling up pending frames in 2 cases: (1) A frame has had a _get_buffer call happen for it, but it has never popped up as a "decoded one", i.e. never ended up submitted to _finish_frame. This could be due to decode-only data/frames or maybe also in some 'hurry up decoding' cases (?). It would eventually have _release_buffer called for it, but that one would only drop a ref and not arrange removing it altogether from the list. (2) A frame never has a _get_buffer call happen for it (and so also no _release_buffer, and no _finish_frame). This could happen due to buggy decoder (?), but also due to misparsed input (interlaced frame stuff or otherwise rare/exotic cases) or maybe keyframe-only (skip) decoding cases. Patches follow that try to fix this.
Btw, at least one sample here is an example of (2), and the file in bug #712219 is an example of (1) (when used with avdec_vp8).
Created attachment 262897 [details] [review] videodecoder: make _release_frame external API Patches makes an internal function external API to allow subclass to perform some more cleanup. The effect is slightly similar to _finish_frame with DECODE_ONLY flag, but performs even less accounting.
Created attachment 262898 [details] [review] avviddec: really release frame at proper time Using API from previous patch, this should solve case (1) outlined above.
Created attachment 262899 [details] [review] avvidec: discard unused input frames This one performs a bit of trickery to determine frames that never had some _get_buffer happen, and so are not worth keeping around anymore at some point. Will commit in some time pending comments and a bit of further testing.
Review of attachment 262897 [details] [review]: ::: gst-libs/gst/video/gstvideodecoder.c @@ +2339,3 @@ + * after which it is considered finished and released. + * + * Since: 1.3 should be Since 1.4
Review of attachment 262898 [details] [review]: ::: ext/libav/gstavviddec.c @@ +545,3 @@ if (frame->mapped) gst_video_frame_unmap (&frame->vframe); + gst_video_decoder_release_frame (GST_VIDEO_DECODER (ffmpegdec), frame->frame); How do you prevent calling both finish_frame() and release_frame() ?
(In reply to comment #13) > Review of attachment 262898 [details] [review]: > > ::: ext/libav/gstavviddec.c > @@ +545,3 @@ > if (frame->mapped) > gst_video_frame_unmap (&frame->vframe); > + gst_video_decoder_release_frame (GST_VIDEO_DECODER (ffmpegdec), > frame->frame); > > How do you prevent calling both finish_frame() and release_frame() ? I don't have to. It is perfectly safe when that happens. Then release_frame will only unref the codec frame, which is what happened on that line before patch. If it is still in the list (as when _finish_frame not called), then it will also remove it there. That is the behaviour which is needed there, as we need to release the frame "all the way".
Btw, this should also work I think for H264 streams where the interlaced field (top or bottom) are encoded separatly, what do you think ?
Review of attachment 262899 [details] [review]: I think it's a rather good solution, it covers all the use cases and cannot really be handled from the baseclass.
Review of attachment 262898 [details] [review]: Agreed, so asside the Since line seems good.
Comment on attachment 262897 [details] [review] videodecoder: make _release_frame external API ACK, please push all this after fixing the Since :) gst_video_decoder_release_frame() should also improve things a bit in gst-omx and elsewhere
Thanks for the review and comments. I think/hope it should also work for the interlaced H264 case. The intention at least is to resolve any leaking (other than possibly due to libav), while also compensating for some other parsing glitches etc. I am also thinking of introducing a GST_VIDEO_CODEC_FRAME_FLAG_NO_FRAME flag (akin to the baseparse _NO_FRAME flag) that signals a "frame" to be even more discardable than _DECODE_ONLY. That is, it allows making this frame cleaning re-usable by having it done by the baseclass for frames marked as _NO_FRAME (which also avoids the frame list copying in addition to being reusable). Also, coming to think of it, it is probably safer/required to have _release_frame take _STREAM_LOCK when manipulating the frame list.
Did some more testing and seems to work out fine. Fwiw, looks like the avi clips mentioned here contain not-codec (null vop frames), which are being discarded. Afaik, those are typically inserted by mencoder/libav when muxing an avi if needed to preserve a/v sync. So libav is likely doing the right thing in discarding these, and we should now be handling this as well. Will stick to these patches for now, and move to the GstVideoDecoder _NO_FRAME approach following more field/master testing. [in -base] commit 40fc3060174bc402fdd9584b09b917d696a40724 Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Tue Nov 26 20:50:33 2013 +0100 videodecoder: make _release_frame external API ... so subclasses can release a frame all the way (also from frame list) without having to pass through _finish_frame or _drop_frame. The latter may not be applicable, or may or may not have already been called for the frame in question. See https://bugzilla.gnome.org/show_bug.cgi?id=693772 [in -libav] commit fa5865c0cdab48c6701fdf6f437ccaea3ab3286a Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Tue Nov 26 20:57:37 2013 +0100 avviddec: discard unused input frames ... to avoid these piling up in list of pending frames. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=693772 commit 2ec4008ea5f3296f50d56b11422ed5d56de6692b Author: Mark Nauwelaerts <mnauw@users.sourceforge.net> Date: Tue Nov 26 20:55:43 2013 +0100 avviddec: really release frame at proper time ... by also removing it from the pending list of frames, where it may still be in if it has never been submitted to _finish. This could happen if is a decode-only frame, or in skipped decoding situation, ... Fixes https://bugzilla.gnome.org/show_bug.cgi?id=693772
Comment on attachment 262897 [details] [review] videodecoder: make _release_frame external API (with minor adjustments as mentioned)
All backported to 1.2, will be in 1.2.2 release