[pulseaudio-discuss] [PATCH] "pipe-sink" module timig based upon "null-sink" module mechanics.

[Tanu Kaskinen]

On Fri, 2017-12-01 at 19:22 +0100, Samo Pogačnik wrote:
> Dne 30.11.2017 (čet) ob 20:44 +0200 je Tanu Kaskinen napisal(a):
> > Thanks for the patch! Since this changes the pipe-sink behaviour
> > rather
> > drastically, other people using this module might not like this. You
> > could add a new module argument: "use_system_clock_for_timing". If
> > the
> > option is not set, the old behaviour would be used.
> > 
> > More comments below:
> > 
> 
> Thank you for your comments.
> 
> Regarding your initial observation about the change, would it be
> sensible to make a separate "pipe-sink" module (i.e. pipe-timed-sink)
> or do you prefer a single module with additional option?

I would prefer a single module with an additional option.

> > > 
> > > +
> > > +do_nothing:
> > > +
> > > +    pa_sink_process_rewind(u->sink, 0);
> > > +}
> > > +
> > > +static void process_render(struct userdata *u, pa_usec_t now) {
> > > +    size_t ate = 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. */
> > > +
> > > +    /* Fill the buffer up the latency size */
> > > +    while (u->timestamp < now + u->block_usec) {
> > >          ssize_t l;
> > >          void *p;
> > >  
> > > +        pa_sink_render(u->sink, u->sink->thread_info.max_request,
> > > &u->memchunk);
> > > +
> > > +        pa_assert(u->memchunk.length > 0);
> > > +
> > >          p = pa_memblock_acquire(u->memchunk.memblock);
> > >          l = pa_write(u->fd, (uint8_t*) p + u->memchunk.index, u-
> > > > memchunk.length, &u->write_type);
> > > 
> > >          pa_memblock_release(u->memchunk.memblock);
> > >  
> > >          pa_assert(l != 0);
> > >  
> > > -        if (l < 0) {
> > > -
> > > -            if (errno == EINTR)
> > > -                continue;
> > > -            else if (errno == EAGAIN)
> > > -                return 0;
> > > -            else {
> > > -                pa_log("Faied to write data to FIFO: %s",
> > > pa_cstrerror(errno));
> > > -                return -1;
> > > -            }
> > 
> > You shouldn't just remove error handling. Also, if pa_write() writes
> > only part of the rendered audio, you probably shouldn't just drop the
> > unwritten data, or at least the drop-outs should be logged.
> > 
> 
> I agree about logging about drop-outs in some limited way, since that
> may be a constant situation when the pipe is full and there is no
> reader.

Yeah, it's not good to log every dropped chunk. Maybe log only the
first dropped chunk and later the total dropped amount when the pipe
starts accepting writes again.

> Regarding this topic, i also have a test implementation of self-
> draining the pipe before each first write into the pipe after entering
> the "running" state. If there is no real reader, each "start play"
> flushes all potentially old data first. What is your opinion about
> that?

Sounds good, but you should treat "idle" and "running" as the same
state in this flushing logic. Changing from "idle" to "running"
shouldn't cause flushing.

-- 
Tanu

https://www.patreon.com/tanuk