[pulseaudio-discuss] [PATCH] tunnel-source-new: counterpart to module-tunnel-sink-new

[Tanu Kaskinen]

On Thu, 2013-09-12 at 14:01 +0200, Alexander Couzens wrote:
> The old tunnel module duplicates functionality that is in libpulse,
> due to implementing the native protocol, and the protocol code in
> the old tunnel module tends to get broken every now and then, because
> people forget to update the tunnel module protocol implementation
> when changing the native protocol. module-tunnel-source-new avoids this
> problem by using libpulse to communicate with the remote server.
> ---
>  src/Makefile.am                        |   6 +
>  src/modules/module-tunnel-source-new.c | 552 +++++++++++++++++++++++++++++++++
>  2 files changed, 558 insertions(+)
>  create mode 100644 src/modules/module-tunnel-source-new.c

Thanks, patch applied with a couple of changes (see below).

> +/* called from io context to read samples from the stream into our source */
> +static void read_new_samples(struct userdata *u) {
> +    const void *p;
> +    size_t readable = 0;
> +    pa_memchunk memchunk;
> +
> +    pa_assert(u);
> +    u->new_data = false;
> +
> +    pa_memchunk_reset(&memchunk);
> +
> +    if (PA_UNLIKELY(!u->connected || pa_stream_get_state(u->stream) != PA_STREAM_READY))
> +        return;
> +
> +    readable = pa_stream_readable_size(u->stream);
> +    while (PA_LIKELY(readable > 0)) {

This PA_LIKELY doesn't do any good. The normal pattern is to evaluate
the condition twice: on the first pass the condition will evaluate to
true and on the second pass it will evaluate to false.

> +        size_t read = 0;
> +        if (PA_UNLIKELY(pa_stream_peek(u->stream, &p, &read) != 0)) {
> +            pa_log("pa_stream_peek() failed: %s", pa_strerror(pa_context_errno(u->context)));
> +            u->thread_mainloop_api->quit(u->thread_mainloop_api, TUNNEL_THREAD_FAILED_MAINLOOP);
> +            return;
> +        }
> +
> +        if (PA_LIKELY(p)) {
> +            /* we have valid data */
> +            memchunk.memblock = pa_memblock_new_fixed(u->module->core->mempool, (void *) p, read, true);
> +            memchunk.length = read;
> +            memchunk.index = 0;
> +
> +            pa_source_post(u->source, &memchunk);
> +            pa_memblock_unref_fixed(memchunk.memblock);
> +            pa_stream_drop(u->stream);
> +
> +            readable -= read;
> +        } else /* if (!p && read > 0) */ {
> +            /* we have a hole. generate silence */
> +            pa_silence_memchunk_get(
> +                        &u->module->core->silence_cache,
> +                        u->module->core->mempool,
> +                        &memchunk,
> +                        &u->source->sample_spec,
> +                        read);

I think this is not safe to call from the IO thread. The silence cache
doesn't seem to be thread safe, so it's only useful in the main thread.
We can use the memblock in u->source->silence instead.

There's no guarantee that either the memchunk returned by
pa_silence_memchunk_get() or the memblock in u->source->silence is large
enough. If it's not large enough, I think we can just call
pa_source_post() in a loop using the same memblock multiple times.

-- 
Tanu