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

[Samo Pogačnik]

Dne 29.12.2017 (pet) ob 15:40 +0200 je Tanu Kaskinen napisal(a):
> On Sat, 2017-12-16 at 16:57 +0100, Samo Pogačnik wrote:
> > 
> > +static ssize_t pipe_sink_write(struct userdata *u, pa_memchunk
> > *pchunk) {
> > +    size_t index, length;
> > +    ssize_t count = 0;
> > +    void *p;
> > +
> > +    pa_assert(u);
> > +    pa_assert(pchunk);
> > +
> > +    index = pchunk->index;
> > +    length = pchunk->length;
> > +    p = pa_memblock_acquire(pchunk->memblock);
> > +
> > +    for (;;) {
> > +        ssize_t l;
> > +
> > +        l = pa_write(u->fd, (uint8_t*) p + index, length, &u-
> > >write_type);
> > +
> > +        pa_assert(l != 0);
> > +
> > +        if (l < 0) {
> > +            if (errno == EAGAIN)
> > +                break;
> > +            else if (errno != EINTR) {
> > +                if (!u->fifo_error) {
> > +                    pa_log("Failed to write data to FIFO: %s",
> > pa_cstrerror(errno));
> > +                    u->fifo_error = true;
> > +                }
> > +                count = l - count;
> process_render_use_timing() assumes that l is -1, so you should
> replace
> l with -1 here.
> 
Agreed.

> > 
> > +                break;
> > +            }
> > +        } else {
> > +            count += l;
> > +            index += l;
> > +            length -= l;
> > +
> > +            if (length <= 0) {
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    pa_memblock_release(pchunk->memblock);
> > +
> > +    if (u->fifo_error && count >= 0) {
> > +        pa_log("Recovered from FIFO error");
> > +        u->fifo_error = false;
> > +    }
> This logic is not right. If in the first loop iteration pa_write()
> does
> a partial write and then an error occurs during the second iteration,
> the code incorrectly thinks that we have recovered from the error.
> The
> recovery happens when pa_write() returns a positive number for the
> first time.
> 
This case is correctly covered, since count always get negative value
in the case of a fifo error, so immediate recovery can not occure via
this condition.

> Also, use pa_log_debug() or pa_log_info(). pa_log() is an alias for
> pa_log_error(), and this is not an error message.
> 
Ok.

> > 
> > +    return count;
> > +}
> > +
> > +static int process_render_use_timing(struct userdata *u, pa_usec_t
> > now) {
> The function return type can be changed to void.
> 
Can switch return type back to void - i just left the code prepared for
potential error case, that would require full pipe_sink failure -
module unload.

regards and thanks for the reply,
Samo