On Wed, 18 May 2016 15:40:55 +0200 Pierre Ossman <[email protected]> wrote:
> I suspect the bug is in the memblockq code somewhere. It seems like it > wedges itself somehow. The "missing" state variable is something I'm > looking at with suspicion. > This wasn't the problem. The trace revealed that we do not handle changes to minreq properly. The scenario is: 1. Large latency, large buffer, large target fill, large minimum request. Silence in queue (i.e. buffer is full). 2. Buffer drains slightly, making it fall below target fill. It is however still below the minimum request, so nothing is sent to the client. 3. The client requests a reduced latency, buffer is reduced, target fill is reduced, minimum request is reduced. The buffer now greatly exceeds target fill as it was almost up to the previous target fill level. This means that the server will not be asking the client for more data for a while. 4. Some time later we've drained most of the excess and are almost back down to the target fill level. However the data requested in 2 is sufficiently large that we never fall back down below target fill. Hence we never start requesting for more data. And we already decided in 2 not to send a request for the first portion. I've attached a patch that moves the minreq handling in to memblockq, which should avoid this and any similar issues as there is now a single place where things are throttled for minreq. This will affect all callers of pa_memblockq_pop_missing() and perturb things a bit. I can't see it being fundamentally worse or better though, just different. Besides fixing the bugs of course. > Why do we even have that? Can't we derive that from the other > variables? Having redundant state is just asking for things to get out > of sync and for bugs to appear. > Patch added for this as well. The memblockq code makes my head spin, and killing of "missing" and santising "requested" makes it a bit easier to comprehend. Please review as I'd really like this fix to start trickling out to the distributions sooner rather than later. Rgds -- Pierre Ossman Software Development Cendio AB https://cendio.com Teknikringen 8 https://twitter.com/ThinLinc 583 30 Linköping https://facebook.com/ThinLinc Phone: +46-13-214600 https://plus.google.com/+CendioThinLinc A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?
>From 5f745b7b7059333e29cfcab1b564c7cf80c70d4d Mon Sep 17 00:00:00 2001 From: Pierre Ossman <[email protected]> Date: Thu, 19 May 2016 15:26:08 +0200 Subject: [PATCH 1/2] memblockq: move minreq handling in to memblockq Having it handled in the callers proved to be a poor fit as it became difficult to handle a shrinking minreq sanely. It could end up in a state where the request was never sent downstream to the client. --- src/pulsecore/memblockq.c | 4 ++++ src/pulsecore/protocol-native.c | 9 ++------- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/pulsecore/memblockq.c b/src/pulsecore/memblockq.c index d314d4e..aa4f03f 100644 --- a/src/pulsecore/memblockq.c +++ b/src/pulsecore/memblockq.c @@ -837,6 +837,10 @@ size_t pa_memblockq_pop_missing(pa_memblockq *bq) { if (bq->missing <= 0) return 0; + if (((size_t) bq->missing < bq->minreq) && + !pa_memblockq_prebuf_active(bq)) + return 0; + l = (size_t) bq->missing; bq->requested += bq->missing; diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c index 145db04..2b89ff8 100644 --- a/src/pulsecore/protocol-native.c +++ b/src/pulsecore/protocol-native.c @@ -1252,8 +1252,7 @@ out: /* Called from IO context */ static void playback_stream_request_bytes(playback_stream *s) { - size_t m, minreq; - int previous_missing; + size_t m; playback_stream_assert_ref(s); @@ -1273,11 +1272,7 @@ static void playback_stream_request_bytes(playback_stream *s) { pa_log("request_bytes(%lu)", (unsigned long) m); #endif - previous_missing = pa_atomic_add(&s->missing, (int) m); - minreq = pa_memblockq_get_minreq(s->memblockq); - - if (pa_memblockq_prebuf_active(s->memblockq) || - (previous_missing < (int) minreq && previous_missing + (int) m >= (int) minreq)) + if (pa_atomic_add(&s->missing, (int) m) <= 0) pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(s), PLAYBACK_STREAM_MESSAGE_REQUEST_DATA, NULL, 0, NULL, NULL); } -- 2.5.5
>From 438ac6b1fc23ff57e27526b5b739764a453efebe Mon Sep 17 00:00:00 2001 From: Pierre Ossman <[email protected]> Date: Thu, 19 May 2016 15:54:08 +0200 Subject: [PATCH 2/2] memblockq: remove internal "missing" state variable It was a very confusing state variable that required a lot of fiddling. It was also redundant in that it can be computed from the other variables, removing any risk of it getting out of sync. In the same spirit, make sure "requested" also always contains a sane value, even though it may not be used by every caller. --- src/pulsecore/memblockq.c | 51 +++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/pulsecore/memblockq.c b/src/pulsecore/memblockq.c index aa4f03f..63de392 100644 --- a/src/pulsecore/memblockq.c +++ b/src/pulsecore/memblockq.c @@ -53,7 +53,7 @@ struct pa_memblockq { bool in_prebuf; pa_memchunk silence; pa_mcalign *mcalign; - int64_t missing, requested; + size_t requested; char *name; pa_sample_spec sample_spec; }; @@ -251,10 +251,12 @@ static void write_index_changed(pa_memblockq *bq, int64_t old_write_index, bool delta = bq->write_index - old_write_index; - if (account) - bq->requested -= delta; - else - bq->missing -= delta; + if (account) { + if (delta > (int64_t)bq->requested) + bq->requested = 0; + else if (delta > 0) + bq->requested -= delta; + } #ifdef MEMBLOCKQ_DEBUG pa_log_debug("[%s] pushed/seeked %lli: requested counter at %lli, account=%i", bq->name, (long long) delta, (long long) bq->requested, account); @@ -262,15 +264,14 @@ static void write_index_changed(pa_memblockq *bq, int64_t old_write_index, bool } static void read_index_changed(pa_memblockq *bq, int64_t old_read_index) { +#ifdef MEMBLOCKQ_DEBUG int64_t delta; pa_assert(bq); delta = bq->read_index - old_read_index; - bq->missing += delta; -#ifdef MEMBLOCKQ_DEBUG - pa_log_debug("[%s] popped %lli: missing counter at %lli", bq->name, (long long) delta, (long long) bq->missing); + pa_log_debug("[%s] popped %lli", bq->name, (long long) delta); #endif } @@ -826,31 +827,37 @@ size_t pa_memblockq_get_prebuf(pa_memblockq *bq) { } size_t pa_memblockq_pop_missing(pa_memblockq *bq) { - size_t l; + int64_t length; + size_t missing; pa_assert(bq); -#ifdef MEMBLOCKQ_DEBUG - pa_log_debug("[%s] pop: %lli", bq->name, (long long) bq->missing); -#endif + /* Note that write_index might be before read_index, which means + * that we should ask for extra data to catch up. This also means + * that we cannot call pa_memblockq_length() as it doesn't return + * negative values. */ - if (bq->missing <= 0) + length = (bq->write_index - bq->read_index) + bq->requested; + + if (length > (int64_t)bq->tlength) return 0; - if (((size_t) bq->missing < bq->minreq) && - !pa_memblockq_prebuf_active(bq)) + missing = (int64_t)bq->tlength - length; + + if (missing == 0) return 0; - l = (size_t) bq->missing; + if ((missing < bq->minreq) && + !pa_memblockq_prebuf_active(bq)) + return 0; - bq->requested += bq->missing; - bq->missing = 0; + bq->requested += missing; #ifdef MEMBLOCKQ_DEBUG - pa_log_debug("[%s] sent %lli: request counter is at %lli", bq->name, (long long) l, (long long) bq->requested); + pa_log_debug("[%s] sent %lli: request counter is at %lli", bq->name, (long long) missing, (long long) bq->requested); #endif - return l; + return missing; } void pa_memblockq_set_maxlength(pa_memblockq *bq, size_t maxlength) { @@ -866,13 +873,11 @@ void pa_memblockq_set_maxlength(pa_memblockq *bq, size_t maxlength) { } void pa_memblockq_set_tlength(pa_memblockq *bq, size_t tlength) { - size_t old_tlength; pa_assert(bq); if (tlength <= 0 || tlength == (size_t) -1) tlength = bq->maxlength; - old_tlength = bq->tlength; bq->tlength = ((tlength+bq->base-1)/bq->base)*bq->base; if (bq->tlength > bq->maxlength) @@ -883,8 +888,6 @@ void pa_memblockq_set_tlength(pa_memblockq *bq, size_t tlength) { if (bq->prebuf > bq->tlength+bq->base-bq->minreq) pa_memblockq_set_prebuf(bq, bq->tlength+bq->base-bq->minreq); - - bq->missing += (int64_t) bq->tlength - (int64_t) old_tlength; } void pa_memblockq_set_minreq(pa_memblockq *bq, size_t minreq) { -- 2.5.5
_______________________________________________ pulseaudio-discuss mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
