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 712597 - multiqueue: regression: buffering of live radio stream never finishes
multiqueue: regression: buffering of live radio stream never finishes
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.2.1
Other All
: Normal blocker
: 1.2.3
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-11-18 14:31 UTC by Arnaud Vrac
Modified: 2014-01-27 19:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
multiqueue: prevent buffering forever with playbin (1.64 KB, patch)
2014-01-15 03:24 UTC, Thiago Sousa Santos
reviewed Details | Review
multiqueue: prevent buffering forever with playbin (1.66 KB, patch)
2014-01-15 12:38 UTC, Thiago Sousa Santos
committed Details | Review
do not reduce single queue below current level (1.34 KB, patch)
2014-01-27 10:14 UTC, Arnaud Vrac
committed Details | Review

Description Arnaud Vrac 2013-11-18 14:31:57 UTC
The following pipeline gets stuck at the buffering state:

gst-launch-1.0 playbin uri='http://adwzg.tdf-cdn.com/3678/nrj_3678.mp3?type=.flv&awparams=radio:Ch-rie-FM-80'

This is using the default buffering values of queue2 in uridecodebin, which are 2s for max-size-time and rate-estimate enabled. The problem is that the live radio stream has a small buffer, so the queue is always empty. The buffering will stop when the downstream multiqueue gets full, which can take a very long time since you have to fill 2MB at 64kbits/s.
Comment 1 Arnaud Vrac 2013-11-18 14:37:07 UTC
note that I have filled this bug in the gstreamer category, since the bug fix probably needs to be done in queue2
Comment 2 Olivier Crête 2013-12-05 23:56:16 UTC
This worked fine in 1.0, so it's a regression we should fix.
Comment 3 Thiago Sousa Santos 2013-12-31 15:14:14 UTC
FWIW it plays, just takes a very long time to start
Comment 4 Thiago Sousa Santos 2013-12-31 18:15:41 UTC
The regression was caused by this commit

commit 7f657358a843afcc2d301773dd122adfe06b4b9f
Author: Matej Knopp <matej.knopp@gmail.com>
Date:   Tue Sep 3 23:59:05 2013 +0200

    multiqueue: Don't reduce single queue visible size below its current level
    
    If the multiqueue has automatically grown chances are good that
    we will cause the pipeline to starve if the maximum level is reduced
    below that automatically grown size.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=707156
Comment 5 Thiago Sousa Santos 2014-01-15 03:24:11 UTC
Created attachment 266326 [details] [review]
multiqueue: prevent buffering forever with playbin

When prerolling/buffering, multiqueue has its buffers limit set
to 0, this means it can take an infinite amount of buffers.

When prerolling/buffering finishes, its limit is set back to 5, but
only if the current level is lower than 5. It should (almost) never be
and this will cause prerolling/buffering to need to wait to reach the
hard bytes and time limits, which are much higher.

This can lead to a very long startup time. This patch fixes this
by setting the single queues to the max(current, new_value) instead
of simply ignoring the new value and letting it as infinite(0)

https://bugzilla.gnome.org/show_bug.cgi?id=721597
Comment 6 Sebastian Dröge (slomo) 2014-01-15 07:58:55 UTC
Review of attachment 266326 [details] [review]:

::: plugins/elements/gstmultiqueue.c
@@ +510,2 @@
         /* do not reduce max size below current level if the single queue has grown because of empty queue */
         if (new_size >= size.visible && size.visible <= mq->max_size.visible)

I do not understand the second condition there. Why do we only do that if the current size is smaller than the maximum size? What should matter is the new_size only, no?

@@ +512,3 @@
           q->max_size.visible = new_size;
+        else if (new_size != 0 && q->max_size.visible == 0)
+          q->max_size.visible = MAX (new_size, size.visible);

Which means that removing the above second condition would fix this too?
Comment 7 Thiago Sousa Santos 2014-01-15 12:09:43 UTC
(In reply to comment #6)

Let me give some real data for this particular stream. At the beginning, the max-size is set to 0, the queue grows until 169 when it is set again. This time it is set to 5. In the current code, this second value is ignored because it is smaller than 169, and it will buffer until the bytes/time limit are reached.

> Review of attachment 266326 [details] [review]:
> 
> ::: plugins/elements/gstmultiqueue.c
> @@ +510,2 @@
>          /* do not reduce max size below current level if the single queue has
> grown because of empty queue */
>          if (new_size >= size.visible && size.visible <= mq->max_size.visible)
> 
> I do not understand the second condition there. Why do we only do that if the
> current size is smaller than the maximum size? What should matter is the
> new_size only, no?

Yeah, I also was wondering about that second condition, but didn't want to touch it.

> 
> @@ +512,3 @@
>            q->max_size.visible = new_size;
> +        else if (new_size != 0 && q->max_size.visible == 0)
> +          q->max_size.visible = MAX (new_size, size.visible);
> 
> Which means that removing the above second condition would fix this too?

I don't think so, new_size is 5 and size.visible is 169, so it would ignore
this new_size. AFAIU, we want to set always the MAX(new_size, size.visible),
taking care with the 0 value that means infinite.
Comment 8 Sebastian Dröge (slomo) 2014-01-15 12:14:32 UTC
(In reply to comment #7)
> (In reply to comment #6)
> 
> Let me give some real data for this particular stream. At the beginning, the
> max-size is set to 0, the queue grows until 169 when it is set again. This time
> it is set to 5. In the current code, this second value is ignored because it is
> smaller than 169, and it will buffer until the bytes/time limit are reached.

Ack

> > Review of attachment 266326 [details] [review] [details]:
> > 
> > ::: plugins/elements/gstmultiqueue.c
> > @@ +510,2 @@
> >          /* do not reduce max size below current level if the single queue has
> > grown because of empty queue */
> >          if (new_size >= size.visible && size.visible <= mq->max_size.visible)
> > 
> > I do not understand the second condition there. Why do we only do that if the
> > current size is smaller than the maximum size? What should matter is the
> > new_size only, no?
> 
> Yeah, I also was wondering about that second condition, but didn't want to
> touch it.

Let's fix it while we're at it ;)

> > 
> > @@ +512,3 @@
> >            q->max_size.visible = new_size;
> > +        else if (new_size != 0 && q->max_size.visible == 0)
> > +          q->max_size.visible = MAX (new_size, size.visible);
> > 
> > Which means that removing the above second condition would fix this too?
> 
> I don't think so, new_size is 5 and size.visible is 169, so it would ignore
> this new_size. AFAIU, we want to set always the MAX(new_size, size.visible),
> taking care with the 0 value that means infinite.

Yes, I agree. I think we can combine these two conditions like you say by always assigning MAX() and treating 0 as infinity.
Comment 9 Thiago Sousa Santos 2014-01-15 12:38:26 UTC
Created attachment 266352 [details] [review]
multiqueue: prevent buffering forever with playbin

When prerolling/buffering, multiqueue has its buffers limit set
to 0, this means it can take an infinite amount of buffers.

When prerolling/buffering finishes, its limit is set back to 5, but
only if the current level is lower than 5. It should (almost) never be
and this will cause prerolling/buffering to need to wait to reach the
hard bytes and time limits, which are much higher.

This can lead to a very long startup time. This patch fixes this
by setting the single queues to the max(current, new_value) instead
of simply ignoring the new value and letting it as infinite(0)
Comment 10 Sebastian Dröge (slomo) 2014-01-15 12:41:42 UTC
Review of attachment 266352 [details] [review]:

Looks good to me and makes much more sense than what was there before IMHO :)
Comment 11 Thiago Sousa Santos 2014-01-15 13:03:36 UTC
Thanks for the review, pushed to master

commit 139c96c129149266669c9799ad3b30d84f62065b
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Wed Jan 15 00:12:26 2014 -0300

    multiqueue: prevent buffering forever with playbin
    
    When prerolling/buffering, multiqueue has its buffers limit set
    to 0, this means it can take an infinite amount of buffers.
    
    When prerolling/buffering finishes, its limit is set back to 5, but
    only if the current level is lower than 5. It should (almost) never be
    and this will cause prerolling/buffering to need to wait to reach the
    hard bytes and time limits, which are much higher.
    
    This can lead to a very long startup time. This patch fixes this
    by setting the single queues to the max(current, new_value) instead
    of simply ignoring the new value and letting it as infinite(0)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=712597
Comment 12 Thiago Sousa Santos 2014-01-15 13:34:03 UTC
Pushed the fix to the 1.2 branch

commit ddc76753e0338b128d1363f1dd72516ee90feb83
Author: Thiago Santos <ts.santos@sisa.samsung.com>
Date:   Wed Jan 15 00:12:26 2014 -0300

    multiqueue: prevent buffering forever with playbin
    
    When prerolling/buffering, multiqueue has its buffers limit set
    to 0, this means it can take an infinite amount of buffers.
    
    When prerolling/buffering finishes, its limit is set back to 5, but
    only if the current level is lower than 5. It should (almost) never be
    and this will cause prerolling/buffering to need to wait to reach the
    hard bytes and time limits, which are much higher.
    
    This can lead to a very long startup time. This patch fixes this
    by setting the single queues to the max(current, new_value) instead
    of simply ignoring the new value and letting it as infinite(0)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=712597
Comment 13 Arnaud Vrac 2014-01-27 10:14:04 UTC
Created attachment 267282 [details] [review]
do not reduce single queue below current level
Comment 14 Arnaud Vrac 2014-01-27 10:14:53 UTC
Hi Thiago, thanks for the fix. Unfortunately you fix sometimes results in a pipeline stall. In the logs I have:

queue 1: visible 8/8, bytes 14336/2097152, time 256000000/0
queue 1 is filled, bumping its max visible to 9
Queue 1 is filled, signalling overrun
queue 1: visible 8/8, bytes 14336/2097152, time 256000000/0

The last line should print visible 8/9.

The issue happens when playbin tries to update the queue limits, and with the fix above the maximum number of buffers is reset to the current level. This cancels the grow by 1 that was just applied, so the queue gets full and nothing can be added.

The patch I've posted above should fix the issue.
Comment 15 Sebastian Dröge (slomo) 2014-01-27 18:58:08 UTC
commit a93615aea1a85d5059de43614d5e6148f6ae4054
Author: Arnaud Vrac <avrac@freebox.fr>
Date:   Fri Jan 24 19:19:08 2014 +0100

    multiqueue: do not reduce single queue below current level
    
    When the single queue size was just bumped by 1 to allow more buffers to
    be added, the buffers limit could be reduced to the current level when
    setting the max-size-buffers property. This would result in a stall
    since the queue would not grow anymore at this point.
    
    Prevent this by not reducing a single queue size below the current
    number of buffers + 1.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=712597
Comment 16 Tim-Philipp Müller 2014-01-27 19:12:50 UTC
Does anyone feel like writing some more unit tests for multiqueue by any chance? So much activity/patching/fixing, yet so few tests :)