GNOME Bugzilla – Bug 712597
multiqueue: regression: buffering of live radio stream never finishes
Last modified: 2014-01-27 19:12:50 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.
note that I have filled this bug in the gstreamer category, since the bug fix probably needs to be done in queue2
This worked fine in 1.0, so it's a regression we should fix.
FWIW it plays, just takes a very long time to start
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
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
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?
(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.
(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.
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)
Review of attachment 266352 [details] [review]: Looks good to me and makes much more sense than what was there before IMHO :)
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
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
Created attachment 267282 [details] [review] do not reduce single queue below current level
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.
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
Does anyone feel like writing some more unit tests for multiqueue by any chance? So much activity/patching/fixing, yet so few tests :)