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

[Samo Pogačnik]

Dne 15.12.2017 (pet) ob 04:01 +0200 je Tanu Kaskinen napisal(a):
> 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.
Sorry for the trouble, i will improve my git skills.

> 
> > 
> > +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.
I though this must be good comment, since it comes from a sample
module. However, i fully accept your remark and i would just remove the
comment for now. Do you agree? 

> 
> > 
> > +
> > +    /* 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.
Here i tried to be as accurate as possible regarding the drops counter,
but if we agree, that drop counting becomes irrelevant upon real fifo
error, then i would not bother returning number of potentially written
bytes within pipe_sink_write() before error condition occurred. What do
you say?
 
> 
> > 
> > +
> > +        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;
> 
Silly me. Thank you for the suggestion.

regards, Samo