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

[Tanu Kaskinen]

On Sat, 2017-12-30 at 18:18 +0100, Samo Pogačnik wrote:
> Dne 30.12.2017 (sob) ob 17:22 +0200 je Tanu Kaskinen napisal(a):
> > On Sat, 2017-12-30 at 14:00 +0100, Samo Pogačnik wrote:
> > > 
> > > 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:
> > > > > 
> > > > > 
> > > > > +                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.
> > 
> > Ah, indeed. My mistake.
> > 
> > I can still think of an (unlikely) case that doesn't work quite
> > right,
> > though:
> > 
> > Let's say an error has occurred and fifo_error is true. Then
> > pipe_sink_write() is called, and the first pa_write() succeeds, but
> > does only a partial write. The error should be considered recovered
> > at
> > this point, but your code doesn't set fifo_error = false. When trying
> > to write the rest of the chunk, an error happens. Now an error
> > message
> > should be logged, but since fifo_error is still true, that doesn't
> > happen.
> > 
> 
> That is true. If you agree, i'll move/change the recovery condition
> into the success block of the pa_write() return check like this:
> ...
>         } else {
> -->         if (u->fifo_error) {
> -->             pa_log("Recovered from FIFO error");
> -->             u->fifo_error = false;
> -->         }
>             count += l;
>             index += l;
>             length -= l;
> ...

That looks good.

> > >  
> > > +    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 with
> module unloading.
> What is your comment?

I don't think there will ever be such error cases, and even if I'm
wrong about that, it will be easy enough to change the return type back
to int. So please change the return type to void.

-- 
Tanu

https://www.patreon.com/tanuk