[pulseaudio-discuss] [PATCH] Third draft implementation of better drain reporting

[Tanu Kaskinen]

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