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
