[pulseaudio-discuss] [PATCH] "pipe-sink" added option "use_system_clock_for_timing"

[Tanu Kaskinen]

On Wed, 2017-12-13 at 22:14 +0100, Samo Pogačnik wrote:
> Thank you for the remarks. I tried to correct all issues and
> below is a new patch.

Thanks. I recommend you send patches with "git send-email" in the
future. That way the formatting will always be right (I had some
trouble getting git accept the patch), the patch will have a commit
message and the author and date will be correct.

> +static int process_render_use_timing(struct userdata *u, pa_usec_t now) {
> +    int ret = 0;
> +    size_t dropped = 0;
> +    size_t consumed = 0;
> +
> +    pa_assert(u);
> +
> +    /* This is the configured latency. Sink inputs connected to us
> +    might not have a single frame more than the maxrequest value
> +    queued. Hence: at maximum read this many bytes from the sink
> +    inputs. */

I could have complained about this earlier: this comment is a bit hard
to understand. What does "this" refer to? (Apparently to max_request or
block_usec.) Either remove the comment or rewrite it so that it's easy
to understand.

> +
> +    /* Fill the buffer up the latency size */
> +    while (u->timestamp < now + u->block_usec) {
> +        ssize_t written = 0;
> +        pa_memchunk chunk;
> +
> +        pa_sink_render(u->sink, u->sink->thread_info.max_request, &chunk);
> +
> +        pa_assert(chunk.length > 0);
> +
> +        if ((written = pipe_sink_write(u, &chunk)) < 0) {
> +            written = -1 - written;
> +            ret = -1;
> +        }

Why don't you return immediately here? It doesn't make sense to me to
continue after writing has failed.

If you return immediately, pipe_sink_write() can then simply return -1.

> +
> +        pa_memblock_unref(chunk.memblock);
> +
> +        u->timestamp += pa_bytes_to_usec(chunk.length, &u->sink->sample_spec);
> +
> +        dropped = chunk.length - written;
> +
> +        if (dropped != 0) {
> +            if (u->bytes_dropped == 0) {
> +                pa_log_debug("Pipe-sink just dropped %zu bytes", dropped);
> +                u->bytes_dropped = dropped;
> +            } else
> +                u->bytes_dropped += dropped;
> +        } else {
> +            if (u->bytes_dropped != 0) {
> +                pa_log_debug("Pipe-sink continuously dropped %zu bytes", u->bytes_dropped);
> +                u->bytes_dropped = 0;
> +            }
> +        }

This still doesn't handle the case of two subsequent partial writes
correctly. The code treats that case as one continuous drop, but since
the second write writes something, it's not a continuous drop.

I think the logic should be something like this:

if (u->bytes_dropped != 0 && dropped != chunk.length) {
    pa_log_debug("Pipe-sink continuously dropped %zu bytes", u->bytes_dropped);
    u->bytes_dropped = 0;
}

if (u->bytes_dropped == 0 && dropped != 0)
    pa_log_debug("Pipe-sink just dropped %zu bytes", dropped);

u->bytes_dropped += dropped;

-- 
Tanu

https://www.patreon.com/tanuk