On Sat, 2012-07-28 at 16:32 +0300, Tanu Kaskinen wrote: > on my free time. I'd like to work on the bugs too, but it's lower in > priority. So, if you want to get bugs fixed, the most reliable way to > achieve that is to submit patches. You seem to have spent quite a lot of > time studying the PulseAudio code already - would you be willing to > prepare proper patches for the bugs that you've found?
Not that much time - I investigated bugs 2 and 4 one evening without prior familiarity with PulseAudio code, then implemented the client-side workaround for 2 and noticed and analyzed bug 3 another day. Fixing things like bug 4 should be quicker for people who are already familiar with the program and have a vision of how things architecturally _should_ work (not just producing correct output in current case). Anyway, I now wrote patches for bugs 3 and 4 plus one bug I had not analyzed before (attached). > Answer to "do people test the low-latency case only": speaking for > myself, my audio needs are mostly covered by Totem and Iceweasel (flash > and html5), and I haven't had audio problems. I'm not sure what > latencies those request, so let's check... Totem wants 200 ms latency > and Iceweasel wants 500 ms (both flash and html5). I think 500 ms should be enough to very clearly notice the rewind bug (number 4). Maybe it's a simple implementation that doesn't use cork for accurate pause? BTW I think that's the same bug an mplayer2 user had reported as PulseAudio bug #48943; the talk about Nautilus in that bug report probably boils down to "do something else long enough for the sink to be suspended".
>From a38131acd83d10cf2692a19471f57b562f268544 Mon Sep 17 00:00:00 2001 From: Uoti Urpala <[email protected]> Date: Sat, 28 Jul 2012 18:24:30 +0300 Subject: [PATCH 1/3] core: fix underrun_for calculation when resampling pa_sink_input_seek() calculates output lenth (slength) and corresponding input length (ilength). During an underrun, the function generates slength bytes of silence and adds ilength to the underrun_for value. However, the ilength value may be shortened to match resampler limits, and there's no corresponding adjustment to slength. Thus, the length of the generated silence is longer than resampler output would have been, and underrun_for should be increased by more than the limited ilength. This error makes the user-visible since_underrun field in struct pa_timing_info too small. Fix by using the original value calculated before limiting in this case. --- src/pulsecore/sink-input.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c index f6f93b8..5affb6b 100644 --- a/src/pulsecore/sink-input.c +++ b/src/pulsecore/sink-input.c @@ -816,6 +816,8 @@ void pa_sink_input_peek(pa_sink_input *i, size_t slength /* in sink frames */, p } else ilength = slength; + // Length corresponding to slength (without extra limiting) + size_t ilength_full = ilength; if (ilength > block_size_max_sink_input) ilength = block_size_max_sink_input; @@ -843,7 +845,7 @@ void pa_sink_input_peek(pa_sink_input *i, size_t slength /* in sink frames */, p pa_memblockq_seek(i->thread_info.render_memblockq, (int64_t) slength, PA_SEEK_RELATIVE, TRUE); i->thread_info.playing_for = 0; if (i->thread_info.underrun_for != (uint64_t) -1) - i->thread_info.underrun_for += ilength; + i->thread_info.underrun_for += ilength_full; break; } -- 1.7.6.561.g3822
>From 2a76e113939e9d2d7dc7deda69c59762faa75ad6 Mon Sep 17 00:00:00 2001 From: Uoti Urpala <[email protected]> Date: Sat, 28 Jul 2012 23:26:21 +0300 Subject: [PATCH 2/3] core: fix broken rewinds after sink suspend pa_sink_input_request_rewind() set certain state in the sink_input object, asked the sink for a rewind, and relied on the sink then calling pa_sink_input_process_rewind() to clear the state set earlier. However, when uncorking a stream on a suspended sink, a rewind request occurs while the sink is still in suspend state. The sink ignores the request and the process_rewind() function it not called. In this case nothing would clear the state of the sink_input object, in particular sink_input->thread_info->rewrite_nbytes == -1. While this state was set, further calls to pa_sink_input_process_rewind() would not ask the sink to rewind. Thus rewinds for the stream would be permanently broken, at least until some external source (like another stream) triggers a rewind on the sink and it calls process_rewind. Rewrite the pa_sink_input_request_rewind logic so that it always asks the sink for a rewind; possibly asking twice should not be harmful as the sink already combines requests. This ensures such "deadlock" between the objects cannot occur. However, there is still the problem that the state from an ignored request may be combined with another request done significantly later; but at least this breaks at most one rewind and will not cause a catastrophic permanent broken state. Possibly this could be avoided by detecting a suspended sink and either resetting state or calling process_rewind directly, but I haven't tried to investigate that. --- src/pulsecore/sink-input.c | 36 ++++++++++++++++-------------------- 1 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c index 5affb6b..57bed3b 100644 --- a/src/pulsecore/sink-input.c +++ b/src/pulsecore/sink-input.c @@ -1923,8 +1923,6 @@ void pa_sink_input_request_rewind( if (i->thread_info.state == PA_SINK_INPUT_CORKED) return; - nbytes = PA_MAX(i->thread_info.rewrite_nbytes, nbytes); - #ifdef SINK_INPUT_DEBUG pa_log_debug("request rewrite %zu", nbytes); #endif @@ -1954,31 +1952,29 @@ void pa_sink_input_request_rewind( if (nbytes > i->thread_info.playing_for) nbytes = (size_t) i->thread_info.playing_for; - i->thread_info.rewrite_nbytes = nbytes; + i->thread_info.rewrite_nbytes = + PA_MAX(i->thread_info.rewrite_nbytes, nbytes); } else i->thread_info.rewrite_nbytes = (size_t) -1; } - i->thread_info.rewrite_flush = - i->thread_info.rewrite_flush || - (flush && i->thread_info.rewrite_nbytes != 0); - - i->thread_info.dont_rewind_render = - i->thread_info.dont_rewind_render || - dont_rewind_render; + i->thread_info.rewrite_flush |= flush && (!rewrite || nbytes != 0); - if (nbytes != (size_t) -1) { + i->thread_info.dont_rewind_render |= dont_rewind_render; - /* Transform to sink domain */ - if (i->thread_info.resampler) - nbytes = pa_resampler_result(i->thread_info.resampler, nbytes); + /* Transform to sink domain */ + if (i->thread_info.resampler) + nbytes = pa_resampler_result(i->thread_info.resampler, nbytes); - if (nbytes > lbq) - pa_sink_request_rewind(i->sink, nbytes - lbq); - else - /* This call will make sure process_rewind() is called later */ - pa_sink_request_rewind(i->sink, 0); - } + if (nbytes > lbq) + pa_sink_request_rewind(i->sink, nbytes - lbq); + else + /* This call will make sure process_rewind() is called later */ + /* Well, at least if the sink is not suspended. If it is, + * it'll ignore the call, and we'll keep stale data from this + * request in fields like rewrite_flush, possibly corrupting + * the next request done much later. */ + pa_sink_request_rewind(i->sink, 0); } /* Called from main context */ -- 1.7.6.561.g3822
>From 32b4bf780ab474e9066fe75400f15374368608e2 Mon Sep 17 00:00:00 2001 From: Uoti Urpala <[email protected]> Date: Sat, 28 Jul 2012 23:51:46 +0300 Subject: [PATCH 3/3] core: adjust playing_for and underrun_for at rewind A rewind may erase data that sink_input counted in playing_for or underrun_for earlier. Add code adjusting those values after a rewind. One visible symptom of this bug was problems recovering from an underrun. When a client calls pa_stream_write() with a large block of memory, the function can split that into smaller pieces before sending it to the server. When receiving new data for a stream that had silence queued due to underrun, the server would do a rewind to replace the queued-but-not-played silence with the new data. Because of the bug, this rewind itself would not change underrun_for. It's possible for multiple rewinds to be done without filling the sink buffer in between (which is what would eventually reset underrun_for). In this case, the server rapidly processing the split packets would rewind the stream for _each_ of them (as underrun_for would stay set), erasing valid audio as a result. --- src/pulsecore/sink-input.c | 25 +++++++++++++++++++++---- 1 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c index 57bed3b..977e9e7 100644 --- a/src/pulsecore/sink-input.c +++ b/src/pulsecore/sink-input.c @@ -967,6 +967,15 @@ void pa_sink_input_drop(pa_sink_input *i, size_t nbytes /* in sink sample spec * pa_memblockq_drop(i->thread_info.render_memblockq, nbytes); } +static void subtract_helper(size_t *p, size_t amount) +{ + if (*p == (size_t) -1) + return; + if (*p < amount) + *p = 0; + *p -= amount; +} + /* Called from thread context */ void pa_sink_input_process_rewind(pa_sink_input *i, size_t nbytes /* in sink sample spec */) { size_t lbq; @@ -993,8 +1002,12 @@ void pa_sink_input_process_rewind(pa_sink_input *i, size_t nbytes /* in sink sam /* We were asked to drop all buffered data, and rerequest new * data from implementor the next time push() is called */ + size_t s = pa_memblockq_get_length(i->thread_info.render_memblockq); + if (i->thread_info.resampler) + s = pa_resampler_request(i->thread_info.resampler, s); pa_memblockq_flush_write(i->thread_info.render_memblockq, TRUE); - + subtract_helper(&i->thread_info.underrun_for, s); + subtract_helper(&i->thread_info.playing_for, s); } else if (i->thread_info.rewrite_nbytes > 0) { size_t max_rewrite, amount; @@ -1017,12 +1030,16 @@ void pa_sink_input_process_rewind(pa_sink_input *i, size_t nbytes /* in sink sam called = TRUE; /* Convert back to to sink domain */ + size_t samount = amount; if (i->thread_info.resampler) - amount = pa_resampler_result(i->thread_info.resampler, amount); + samount = pa_resampler_result(i->thread_info.resampler, amount); - if (amount > 0) + if (samount > 0) { /* Ok, now update the write pointer */ - pa_memblockq_seek(i->thread_info.render_memblockq, - ((int64_t) amount), PA_SEEK_RELATIVE, TRUE); + pa_memblockq_seek(i->thread_info.render_memblockq, - ((int64_t) samount), PA_SEEK_RELATIVE, TRUE); + subtract_helper(&i->thread_info.underrun_for, amount); + subtract_helper(&i->thread_info.playing_for, amount); + } if (i->thread_info.rewrite_flush) pa_memblockq_silence(i->thread_info.render_memblockq); -- 1.7.6.561.g3822
_______________________________________________ pulseaudio-discuss mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
