GNOME Bugzilla – Bug 707156
multiqueue: Lowering single queue limits below automatically detected limits can cause the pipeline to starve
Last modified: 2014-01-15 03:22:47 UTC
Created attachment 253663 [details] [review] Patch This is hard to reproduce so hopefully the description is clear enough. If I understand the code correctly, this is what happens: I have a file that is not muxed very well so the max-size-buffers on single queues needs to be increased (which happens when the other single queue is empty). However later when decodebin gets overrun signal, it calls decodebin_set_queue_size, which sets max-size-buffers on mutliqueue, which in turn sets max-size-buffers on singlequeues to default values, disregarding the increased size; After this happens the single queue that have previously grown no longer accepts any buffers and pipeline deadlocks. Now I'm not deluding myself that attached patch is a proper solution, but it demonstrates the problem (and fixes it for me)
You mean because the single queues have grown due to the "all empty except for this" logic? I think it would make more sense to keep this grown value in any case unless a bigger size is set for max-size-buffers. Something to keep the "automatically grown" size around. The patch only seems like a workaround
Yes, that's what I meant. I never claimed this was a proper solution. I also think it would be better to fix it in multiqueue, perhaps the max-size on singlequeue would never be set to be smaller than currentsize+1, or something like that.
<slomo> wtay: https://bugzilla.gnome.org/show_bug.cgi?id=707156 do you have an opinion on that? :) <wtay> slomo, yes, I agree to not set the size smaller than current size <slomo> wtay: alright, i will do that then :) thanks <slomo> wtay: and the limits from the settings should be set in READY->PAUSED again? <wtay> slomo, limits from settings? <slomo> wtay: the ones from the properties <slomo> wtay: talking about multiqueue still :) <wtay> slomo, but that's in decodebin2, no, which recreates the multiqueue when it goes to READY, so how does it matter? <slomo> wtay: i'm thinking of fixing this in multiqueue... for decodebin it's not a problem, right <wtay> slomo, how would you do that? <slomo> wtay: in set_property() only update the singlequeue values if >= current, in READY->PAUSED reset all values in the singlequeues <wtay> slomo, I understood that during discovery we set very few limits on the multiqueue and then after discovery of the streams, we add some more constraints, but that's all in decodebin2 <wtay> oh really <wtay> that's unusual <slomo> that was my idea how to fix it in multique, not how it is currently btw ;) <slomo> so the problem is that multiqueue grows the singlqueues automatically in some cases, and if we set the limits in decodebin again we will ignore this automatic growth and set it back to a fixed value <slomo> which will break stuff <wtay> yes, so my idea was to get the current levels and then only update those that are larger <slomo> but decodebin does not know about the singlequeue levels, shouldn't that also all be done inside multiqueue instead? <wtay> that's true <wtay> whay change the levels at all, anyway? <slomo> to keep memory usage and the amount of buffering lower i guess <slomo> during pre-rolling we just allow more to have enough time to find all the streams <slomo> but then... if we lower the levels because of overrun nothing will usually happen anyway <wtay> but multiqueue already does that by itself now <slomo> because current >= limit , otherwise it would not overrun <slomo> does what by itself? <wtay> ah, in the case of no no-more-pads <wtay> it keeps the levels as low as possible but still have data in all queues <wtay> by bumping the max-size-buffers if a queue is empty and another overruns <wtay> so you end up with the lowest possible amount of data while still having enough to demux the file <slomo> yes <wtay> yes, there are also time and bytes levels but those are normally never hit <wtay> and if they are, it's because a really badly muxed file or because of no-more-pads <slomo> so you say we should just not have this preroll levels in decodebin? <wtay> the only possible reason would be for the demuxers without no-more-pads <wtay> so, have a high queue level to find the streams, then let it drain to something smaller why streaming <wtay> while <wtay> the problem is really finding what smaller limit is acceptable <wtay> when no-more-pads is signalled, this is exactly what multiqueue is using now, so there is no reason to change it <wtay> in the case no-more-pads is not signaled, we don't know.. <wtay> so, don't change the queue sizes if no-more-pads was signaled sounds better <slomo> sounds like a plan :) <slomo> or not <slomo> during prerolling we allow higher levels, but during prerolling we don't know if the demuxer emits no-more-pads or not <slomo> so we would always use the high levels for no-more-pads demuxers <wtay> yes, but that's all right <wtay> they won't just be hit, it'll use the max-size-buffers that has been found automatically <wtay> (this code is all from when we didn't try to find the optimal max-size-buffers) <slomo> hmm <slomo> i'm convinced <slomo> let's do that then :) <slomo> after a coffee <wtay> I guess you could also look at the current bytes and time levels and use the alternative levels only if they are smaller
Created attachment 253830 [details] [review] Patch for multiqueue This patch should make queue only set max visible buffers on single queue when the queue has not grown because of other empty queue (or the new size is >= current size)
slomo: matej_k: hmm, but what if it has grown because of an empty queue... but now no queue is empty anymore? Fair point. I'll try again.
Created attachment 253901 [details] [review] Patch Prevents MQ setting SQ visible level below its current level.
Comment on attachment 253901 [details] [review] Patch Almost I think... You should check if it is higher than mq->max_size.visible (from before overriding that value), and if it is not set it to the lower value. The queue might've grown to a higher value than what it currently contains.
I'l take a look. Weird thing is that I can't reproduce the deadlock with this patch. I'll change it though.
It fixes your case because in decodebin the max-size change happens from the overrun signal... when all queues are as full as possible and thus max_size==current_size
Created attachment 254012 [details] [review] Updated patch Something like this?
Or should I check max_size on the single queue? I'm a bit confused now.
Created attachment 254021 [details] [review] Updated patch Typo
No, exactly that IMO :) 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
Hello Matej, Could you please check that the patch at https://bugzilla.gnome.org/show_bug.cgi?id=712597 doesn't break your use case here?