After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 423700 - [multiqueue] leaks memory when flushing
[multiqueue] leaks memory when flushing
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
Other All
: Normal normal
: 0.10.14
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2007-03-28 12:22 UTC by Tommi Myöhänen
Modified: 2007-06-06 18:14 UTC
See Also:
GNOME target: ---
GNOME version: ---

patch to gstmultiqueue.c (1.98 KB, patch)
2007-03-28 12:23 UTC, Tommi Myöhänen
reviewed Details | Review
alternative patch to fix multiqueue from leaking when flushing (5.32 KB, patch)
2007-05-24 12:34 UTC, Tim-Philipp Müller
committed Details | Review

Description Tommi Myöhänen 2007-03-28 12:22:01 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:
Comment 1 Tommi Myöhänen 2007-03-28 12:23:28 UTC
Created attachment 85440 [details] [review]
patch to gstmultiqueue.c
Comment 2 Tim-Philipp Müller 2007-03-28 22:35:49 UTC
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 ...)
Comment 3 Tommi Myöhänen 2007-03-29 08:03:18 UTC
(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.

Comment 4 Tim-Philipp Müller 2007-05-24 12:34:38 UTC
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.
Comment 5 Tim-Philipp Müller 2007-06-06 18:14:47 UTC

 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),
        * 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.