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 736782 - compositor: performance degrades over time
compositor: performance degrades over time
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.4.1
Other Linux
: Normal normal
: 1.4.6
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-17 08:33 UTC by Jan Alexander Steffens (heftig)
Modified: 2015-05-12 08:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (9.13 KB, patch)
2014-09-17 14:59 UTC, Jan Alexander Steffens (heftig)
reviewed Details | Review
rebased patch (8.66 KB, patch)
2014-10-06 06:53 UTC, Jan Alexander Steffens (heftig)
committed Details | Review

Description Jan Alexander Steffens (heftig) 2014-09-17 08:33:35 UTC
Comparing the following two pipelines:

gst-launch-1.0 videotestsrc ! videomixer ! progressreport ! fakesink

gst-launch-1.0 videotestsrc ! compositor ! progressreport ! fakesink

The one using compositor both grows in memory use and slows down over time. It ends up spending most of its time in g_source_iter_next.
Comment 1 Nicolas Dufresne (ndufresne) 2014-09-17 13:27:25 UTC
Looks like some g_source/idle callback leak.
Comment 2 Jan Alexander Steffens (heftig) 2014-09-17 14:59:28 UTC
Created attachment 286388 [details] [review]
patch

Here's a patch that fixes the bug. "make check" still passes.
Comment 3 Jan Alexander Steffens (heftig) 2014-09-17 15:01:37 UTC
Oh, and just to repeat the patch description here because I forgot:


aggregator: Replace GMainContext with GAsyncQueue

The previous implementation kept accumulating GSources,
slowing down the iteration and leaking memory.

Instead of trying to fix the mcontext flushing, replace
it with a GAsyncQueue which is simple to flush and has
less overhead.
Comment 4 Thibault Saunier 2014-09-17 15:03:40 UTC
Review of attachment 286388 [details] [review]:

Apart from the question it looks good. I will run my testsuite on it.

::: gst-libs/gst/base/gstaggregator.c
@@ -486,1 +469,0 @@
-  /*  Clean the stack of GSource set on the MainContext */

Why don't you flush here directly?
Comment 5 Jan Alexander Steffens (heftig) 2014-09-17 15:11:06 UTC
(In reply to comment #4)
> ::: gst-libs/gst/base/gstaggregator.c
> @@ -486,1 +469,0 @@
> -  /*  Clean the stack of GSource set on the MainContext */
> 
> Why don't you flush here directly?

There needs to be one item on the queue so that gst_async_queue_pop is unblocked after priv->running is set to FALSE. I suppose I could do "QUEUE_FLUSH (self); QUEUE_PUSH (self);" but that looks weirder to me than pushing one item after running=FALSE, and then flushing the queue once the src task is stopped.
Comment 6 Thibault Saunier 2014-09-17 15:22:25 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > ::: gst-libs/gst/base/gstaggregator.c
> > @@ -486,1 +469,0 @@
> > -  /*  Clean the stack of GSource set on the MainContext */
> > 
> > Why don't you flush here directly?
> 
> There needs to be one item on the queue so that gst_async_queue_pop is
> unblocked after priv->running is set to FALSE. I suppose I could do
> "QUEUE_FLUSH (self); QUEUE_PUSH (self);" but that looks weirder to me than
> pushing one item after running=FALSE, and then flushing the queue once the src
> task is stopped.

Got it, it makes sense then yes. I will check a few things and push that I think. Thanks for your patch.
Comment 7 Jan Alexander Steffens (heftig) 2014-10-06 06:53:43 UTC
Created attachment 287805 [details] [review]
rebased patch

Rebased the patch to current -bad master, as it conflicted with 1b4547f.
Comment 8 Thibault Saunier 2014-10-06 17:33:06 UTC
commit dce92c75b1062427605d25d1bc3ff6bb8818d7c4
Author: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>
Date:   Wed Sep 17 16:48:02 2014 +0200

    aggregator: Replace GMainContext with GAsyncQueue (v2)
    
    The previous implementation kept accumulating GSources,
    slowing down the iteration and leaking memory.
    
    Instead of trying to fix the main context flushing, replace
    it with a GAsyncQueue which is simple to flush and has
    less overhead.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=736782
Comment 9 Thibault Saunier 2014-10-06 17:33:45 UTC
Thanks a lot for your patch! :)