GNOME Bugzilla – Bug 763460
vaapidecoder: events from frames might leak when shutting down
Last modified: 2016-04-01 13:07:28 UTC
See associated patch commit message. It was reproducible using a pipeline with dvb:// that would shutdown and restart every few seconds. This is not directly related to dvb/mpegts but it seems that hose formats output lots of tag events that makes it easy to reproduce the event leak.
Created attachment 323631 [details] [review] videodecoder: fix events leak When a frame is released its associated events are transfered to the 'pending_events' list to be pushed with the next frame. The leak happens because, when shutting down, videodecoder frees its pending_events lis and then unrefs the pool. By unreffing the pool, its associated frames are also unreffed and those frames might contain events that would be added to the pending_events list that won't be freed again. We could re-free the pending_events list after unreffing the pool to make sure those events are freed but, as the pool can have references elsewhere, it won't guarantee that it is going to be freed at that moment. Instead, this patch fixes it by adding a boolean to indicate whether the videodecoder is started or not ( > READY state) and, depending on that, the events from a released frame are directly freed or added to the pending_events list.
Comment on attachment 323631 [details] [review] videodecoder: fix events leak I think I prefer to re-free the list :) Instead of the new boolean you can also check GST_STATE(dec) <= READY.
(In reply to Sebastian Dröge (slomo) from comment #2) > Comment on attachment 323631 [details] [review] [review] > videodecoder: fix events leak > > I think I prefer to re-free the list :) Instead of the new boolean you can > also check GST_STATE(dec) <= READY. Agreed on the new boolean but re-freeing the list might not always work if someone else has a reference on the bufferpool and unrefs it after videodecoder.
How is the events list related to a buffer pool?
My mistake, it is actually gst_vaapidecode_destroy that is unreffing GstVideoCodecFrames. It is called in READY_TO_NULL. I guess we need to free all events from those frames when we reach ready state. They shouldn't make sense when a new stream is restarted.
In PAUSED->READY we should get rid of all frames and events and whatever else they contain
Created attachment 324307 [details] [review] vaapidecode: add stop function In the end it seemed that it was only vaapidecode missing an implementation to videodecoder's stop function
Comment on attachment 323631 [details] [review] videodecoder: fix events leak So let's keep videodecoder as is then :)
Comment on attachment 324307 [details] [review] vaapidecode: add stop function Looks good to me
Any further comments from gstreamer-vaapi maintainers? Otherwise I'll just push it this week.
(In reply to Thiago Sousa Santos from comment #10) > Any further comments from gstreamer-vaapi maintainers? > > Otherwise I'll just push it this week. Sorry, I lost track of this issue. Looks good. Not a blocker, but I would try to merge, somehow, _stop()/close()/_destroy() in order to avoid code duplication.
Created attachment 325087 [details] [review] vaapidecode: add stop function Updated moving some cleanup from finalize() to stop(). The only common call now is the _destroy() function that is called both from _close() and from _reset_full() but I'm not sure we can change much as the reset_full has to destroy the current decoder to create a new one.
commit 506c9e2b5bbc50730b0083598b346b0fcc41ec78 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Mar 18 20:00:52 2016 -0300 vaapidecode: add stop function Clear any status on the current stream: stored frames, caps and decoder configuration https://bugzilla.gnome.org/show_bug.cgi?id=763460