On Mon, 2013-03-18 at 13:32 +0100, David Henningsson wrote:
> Ok, so using sink_input->render_memblockq.max_rewind looked like a good idea, but in
> fact it does not work with early requests, which alsa-plugins use.
> I haven't found any drawback from skipping this check, and it actually makes the patch simpler, too.
>
> I've tested it now with both aplay and paplay (and both in parallel!) and it seems to work.
> Anybody else who wants to run it for a test?
>
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
> src/modules/alsa/alsa-sink.c | 51 ++++++++++++++++++++++++----------
> src/pulsecore/protocol-native.c | 58 ++++++++++++++++++++++++++++-----------
> src/pulsecore/sink-input.c | 26 ++++++++++++++++--
> src/pulsecore/sink-input.h | 8 ++++++
> src/pulsecore/sink.c | 26 ++++++++++++++++++
> src/pulsecore/sink.h | 2 ++
> 6 files changed, 139 insertions(+), 32 deletions(-)
The patch looks good to me (apart from the commit message and the small
issue pointed out below).
> diff --git a/src/pulsecore/protocol-native.c b/src/pulsecore/protocol-native.c
> index a4ee920..ae467f4 100644
> --- a/src/pulsecore/protocol-native.c
> +++ b/src/pulsecore/protocol-native.c
> @@ -231,6 +231,7 @@ enum {
> CONNECTION_MESSAGE_REVOKE
> };
>
> +static bool sink_input_process_underrun_cb(pa_sink_input *i);
> static int sink_input_pop_cb(pa_sink_input *i, size_t length, pa_memchunk *chunk);
> static void sink_input_kill_cb(pa_sink_input *i);
> static void sink_input_suspend_cb(pa_sink_input *i, pa_bool_t suspend);
> @@ -1171,6 +1172,7 @@ static playback_stream* playback_stream_new(
>
> s->sink_input->parent.process_msg = sink_input_process_msg;
> s->sink_input->pop = sink_input_pop_cb;
> + s->sink_input->process_underrun = sink_input_process_underrun_cb;
> s->sink_input->process_rewind = sink_input_process_rewind_cb;
> s->sink_input->update_max_rewind = sink_input_update_max_rewind_cb;
> s->sink_input->update_max_request = sink_input_update_max_request_cb;
> @@ -1573,6 +1575,44 @@ static int sink_input_process_msg(pa_msgobject *o, int code, void *userdata, int
> return pa_sink_input_process_msg(o, code, userdata, offset, chunk);
> }
>
> +
> +static bool handle_input_underrun(playback_stream *s, bool force)
> +{
> + bool send_drain;
> +
> + if (pa_memblockq_is_readable(s->memblockq))
> + return false;
> +
> + if (!s->is_underrun)
> + pa_log_debug("%s %s of '%s'", force ? "Actual" : "Implicit",
> + s->drain_request ? "drain" : "underrun", pa_strnull(pa_proplist_gets(s->sink_input->proplist, PA_PROP_MEDIA_NAME)));
> +
> + send_drain = s->drain_request && (force || pa_sink_input_safe_to_remove(s->sink_input));
> +
> + if (send_drain) {
> + s->drain_request = false;
> + pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(s), PLAYBACK_STREAM_MESSAGE_DRAIN_ACK, PA_UINT_TO_PTR(s->drain_tag), 0, NULL, NULL);
> + pa_log_debug("Drain acknowledged of '%s'", pa_strnull(pa_proplist_gets(s->sink_input->proplist, PA_PROP_MEDIA_NAME)));
> + } else if (!s->is_underrun) {
> + pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(s), PLAYBACK_STREAM_MESSAGE_UNDERFLOW, NULL, pa_memblockq_get_read_index(s->memblockq), NULL, NULL);
> + }
> + s->is_underrun = true;
> + playback_stream_request_bytes(s);
> + return true;
> +}
> +
> +/* Called from thread context */
> +static bool sink_input_process_underrun_cb(pa_sink_input *i) {
> + playback_stream *s;
> +
> + pa_sink_input_assert_ref(i);
> + s = PLAYBACK_STREAM(i->userdata);
> + playback_stream_assert_ref(s);
> +
> + return handle_input_underrun(s, true);
> +}
> +
> +
> /* Called from thread context */
> static int sink_input_pop_cb(pa_sink_input *i, size_t nbytes, pa_memchunk *chunk) {
> playback_stream *s;
> @@ -1586,22 +1626,8 @@ static int sink_input_pop_cb(pa_sink_input *i, size_t nbytes, pa_memchunk *chunk
> pa_log("%s, pop(): %lu", pa_proplist_gets(i->proplist, PA_PROP_MEDIA_NAME), (unsigned long) pa_memblockq_get_length(s->memblockq));
> #endif
>
> - if (pa_memblockq_is_readable(s->memblockq))
> - s->is_underrun = FALSE;
> - else {
> - if (!s->is_underrun)
> - pa_log_debug("Underrun on '%s', %lu bytes in queue.", pa_strnull(pa_proplist_gets(i->proplist, PA_PROP_MEDIA_NAME)), (unsigned long) pa_memblockq_get_length(s->memblockq));
> -
> - if (s->drain_request && pa_sink_input_safe_to_remove(i)) {
> - s->drain_request = FALSE;
> - pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(s), PLAYBACK_STREAM_MESSAGE_DRAIN_ACK, PA_UINT_TO_PTR(s->drain_tag), 0, NULL, NULL);
> - } else if (!s->is_underrun)
> - pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(s), PLAYBACK_STREAM_MESSAGE_UNDERFLOW, NULL, pa_memblockq_get_read_index(s->memblockq), NULL, NULL);
> -
> - s->is_underrun = TRUE;
> -
> - playback_stream_request_bytes(s);
> - }
> + if (!handle_input_underrun(s, false))
> + s->is_underrun = false;
>
> /* This call will not fail with prebuf=0, hence we check for
> underrun explicitly above */
This comment refers to code that got moved elsewhere.
--
Tanu
