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
