GNOME Bugzilla – Bug 423700
[multiqueue] leaks memory when flushing
Last modified: 2007-06-06 18:14:47 UTC
Please describe the problem: When flushing e.g. when seeking, GstMultiQueueItems are freed but actual buffers/events aren't. Refcount of buffer/event is increased by 1 when creating a new GstMultiQueueItem and decreased by 1 when destroying GstMultiQueueItem. However if a buffer pushed to multiqueue is to be dropped due to a flush I think currently no one unrefs the actual buffer. Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 85440 [details] [review] patch to gstmultiqueue.c
I can reproduce the bug (see disabled testcase in the unit test I added today), but something doesn't look quite right here to me (might just be screwy semantics in GstMultiQueue and GstDataQueue or me being confused though). For example: - gst_multi_queue_item_destroy() already unrefs item->object, so why the second unref in the patch? - in gst_multi_queue_chain() it creates a new GstMultiQueueItem, which takes its own ref to the buffer, but it doesn't give away the taken-over reference to the buffer unconditionally at the end of the chain function, but only if gst_data_queue_push() fails (and I'm not even going to ask why gst_data_queue_push() doesn't take ownership of the passed item unconditionally, just like gst_pad_push*() do, or why it doesn't return a flow return directly ...)
(In reply to comment #2) > - gst_multi_queue_item_destroy() already unrefs item->object, so why > the second unref in the patch? The second unref is meant to free the gst_mini_object i.e. drop the buffer, assuming it arrived to multiqueue chain with refcount of 1. One might think that currently gst_multi_queue_item_destroy() only destroys the GstMultiQueueItem "container" but not the content itself. gst_multi_queue_item_destroy() is also called in the end of the loop function when buffers are pushed downstream.
Created attachment 88729 [details] [review] alternative patch to fix multiqueue from leaking when flushing Just looked at this again, I think that the implicit refcounting in multiqueue/dataqueue is a bit screwy due to the assumed additional mini object reference passed for the gst_pad_push(). So either data queue needs to drop this assumed additional reference in addition to destroying the item when it deletes/drops an item, or multiqueue needs to handle things a bit differently. I think it makes most sense to go about it like this: - whoever owns the GstDataQueueItem, owns a reference to item->object - item->destroy() should drop that reference as well - when multiqueue does gst_pad_push() or gst_event_push(), it should take its own reference for that This is least confusing IMHO and only requires changes in multiqueue (and some documentation updates in dataqueue), and also has no adverse effects other than leaks in case anyone else was already using the dataqueue API. Patch attached, also enables unit test for this case.
Committed: 2007-06-06 Tim-Philipp Müller <tim at centricular dot net> * libs/gst/base/gstdataqueue.c: * libs/gst/base/gstdataqueue.h: * plugins/elements/gstmultiqueue.c: (gst_single_queue_push_one), (gst_multi_queue_item_new), (gst_multi_queue_chain), (gst_multi_queue_sink_event): * tests/check/elements/multiqueue.c: (multiqueue_suite): Fix multiqueue leaking buffers and events when downstream or the queue are flushing. Make refcounting assumptions explicit and document them (shouldn't break existing code that uses it other than maybe leak miniobjects, but that already happens anyway). Add unit test for the most common flushing case. Fixes #423700.