On Thu, 2011-06-09 at 05:42 -0400, xing wang wrote: > hi all, > > i've a finding about silence delay, the background is looking like > this post : "[pulseaudio-discuss] [PATCH] core: Drop empty gaps in the > memblockq when playing data from it." > (http://www.mail-archive.com/[email protected]/msg09579.html) > > with tsched=1 and using default 2s buffer size,,while alsa-driver > provides a even bigger buffer(5s),after the first rewind of "latency > change" before starting playback, the buffer had been shrinked to a > smaller value according to app's request, this trigger sink-input's > rewinding , read-index become a negative value,,nearly -buffer_size. > > in pa_sink_input_cb() , pa_memblockq_peek() will return silence before > the readindex reach 0 from the negative value. that caused obvious > delay.
I'm not convinced that causes any delay. I tried to reproduce the problem myself, and while my sound card (or its driver) is capable of giving only 185ms buffer so I can't really observe the by listening, adding some debug prints (patch attached) showed that yes, the read index gets negative, but when the client data arrives and the "end of underrun" rewind happens, the write index gets reset to be the same as the read index (ie. negative). Since the write index is not ahead of the read index, there shouldn't be any silence because of the negative indexes. This is different from the case that you linked to - there the write index did not get reset to the read index. That said, I can't think of any case where it would make sense to rewind beyond the beginning of a stream (or any other memblockq), so I've attached a patch that limits pa_memblockq_rewind() so that it never rewinds to negative read index values. I'm not sure if it breaks anything, but at least basic playback and volume control seems to work without glitches... You can try it if it helps in your case. Maybe the patch should be even committed to master? If the patch doesn't break anything, it should make these two fixes redundant: http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=6bd34156b130c07b130de10111a12ef6dab18b52 http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=495c1ed2361f7bab5eaa6978d2fda624a2944bb9 As I say in the commit message, if we're trying to limit the read index to be always non-negative, then the write index should probably be limited too. And then the index types could be changed from signed to unsigned... I wonder if Lennart made them signed for a purpose? -- Tanu
>From 5be14975db4f39fbb540714d9c1118d9e47c29b9 Mon Sep 17 00:00:00 2001 From: Tanu Kaskinen <[email protected]> Date: Thu, 9 Jun 2011 15:38:08 +0300 Subject: [PATCH 1/2] sink-input: (DEBUG) Add render_memblockq read and write index printing. --- src/pulsecore/sink-input.c | 15 ++++++++++++--- 1 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c index 553e3d9..58258ef 100644 --- a/src/pulsecore/sink-input.c +++ b/src/pulsecore/sink-input.c @@ -767,7 +767,8 @@ void pa_sink_input_peek(pa_sink_input *i, size_t slength /* in sink frames */, p pa_assert(chunk); pa_assert(volume); -/* pa_log_debug("peek"); */ + pa_log("peek"); + pa_log("render_memblockq: read_index = %li write_index = %li", (long) pa_memblockq_get_read_index(i->thread_info.render_memblockq), (long) pa_memblockq_get_write_index(i->thread_info.render_memblockq)); pa_assert(i->thread_info.state == PA_SINK_INPUT_RUNNING || i->thread_info.state == PA_SINK_INPUT_CORKED || @@ -921,6 +922,8 @@ void pa_sink_input_peek(pa_sink_input *i, size_t slength /* in sink frames */, p pa_cvolume_mute(volume, i->sink->sample_spec.channels); else *volume = i->thread_info.soft_volume; + + pa_log("render_memblockq: read_index = %li write_index = %li", (long) pa_memblockq_get_read_index(i->thread_info.render_memblockq), (long) pa_memblockq_get_write_index(i->thread_info.render_memblockq)); } /* Called from thread context */ @@ -932,9 +935,12 @@ void pa_sink_input_drop(pa_sink_input *i, size_t nbytes /* in sink sample spec * pa_assert(pa_frame_aligned(nbytes, &i->sink->sample_spec)); pa_assert(nbytes > 0); -/* pa_log_debug("dropping %lu", (unsigned long) nbytes); */ + pa_log("dropping %lu", (unsigned long) nbytes); + pa_log("render_memblockq: read_index = %li write_index = %li", (long) pa_memblockq_get_read_index(i->thread_info.render_memblockq), (long) pa_memblockq_get_write_index(i->thread_info.render_memblockq)); pa_memblockq_drop(i->thread_info.render_memblockq, nbytes); + + pa_log("render_memblockq: read_index = %li write_index = %li", (long) pa_memblockq_get_read_index(i->thread_info.render_memblockq), (long) pa_memblockq_get_write_index(i->thread_info.render_memblockq)); } /* Called from thread context */ @@ -947,7 +953,8 @@ void pa_sink_input_process_rewind(pa_sink_input *i, size_t nbytes /* in sink sam pa_assert(PA_SINK_INPUT_IS_LINKED(i->thread_info.state)); pa_assert(pa_frame_aligned(nbytes, &i->sink->sample_spec)); -/* pa_log_debug("rewind(%lu, %lu)", (unsigned long) nbytes, (unsigned long) i->thread_info.rewrite_nbytes); */ + pa_log("rewind(nbytes = %lu, rewrite_nbytes = %lu)", (unsigned long) nbytes, (unsigned long) i->thread_info.rewrite_nbytes); + pa_log("render_memblockq: read_index = %li write_index = %lu", (long) pa_memblockq_get_read_index(i->thread_info.render_memblockq), (long) pa_memblockq_get_write_index(i->thread_info.render_memblockq)); lbq = pa_memblockq_get_length(i->thread_info.render_memblockq); @@ -1008,6 +1015,8 @@ void pa_sink_input_process_rewind(pa_sink_input *i, size_t nbytes /* in sink sam i->thread_info.rewrite_nbytes = 0; i->thread_info.rewrite_flush = FALSE; i->thread_info.dont_rewind_render = FALSE; + + pa_log("render_memblockq: read_index = %li write_index = %li", (long) pa_memblockq_get_read_index(i->thread_info.render_memblockq), (long) pa_memblockq_get_write_index(i->thread_info.render_memblockq)); } /* Called from thread context */ -- 1.7.5.3
>From 19d6149d8948889bb1f5871f78cf6a556ecc24c9 Mon Sep 17 00:00:00 2001 From: Tanu Kaskinen <[email protected]> Date: Thu, 9 Jun 2011 16:42:48 +0300 Subject: [PATCH 2/2] memblockq: Limit rewinding so that it never results in a negative read index. For completeness, maybe it would make sense to also limit seeking so that the write index never gets negative... --- src/pulsecore/memblockq.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-) diff --git a/src/pulsecore/memblockq.c b/src/pulsecore/memblockq.c index c76ca84..b0cbc4d 100644 --- a/src/pulsecore/memblockq.c +++ b/src/pulsecore/memblockq.c @@ -646,7 +646,12 @@ void pa_memblockq_rewind(pa_memblockq *bq, size_t length) { /* This is kind of the inverse of pa_memblockq_drop() */ - bq->read_index -= (int64_t) length; + if (bq->read_index >= (int64_t) length) + bq->read_index -= (int64_t) length; + else { + pa_log_debug("Limiting rewinding to %li bytes to avoid negative read index.", (long) bq->read_index); + bq->read_index = 0; + } read_index_changed(bq, old); } -- 1.7.5.3
_______________________________________________ pulseaudio-discuss mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
