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

[Samo Pogačnik]

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;
...


> > 
> > +    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?

regards,
Samo