[vlc-devel] [PATCH] pulse: follow PulseAudio write requests

[Rémi Denis-Courmont]

Le mardi 19 mars 2019, 16:05:04 EET Thomas Guillem a écrit :
> Hello,
> 
> On Tue, Mar 19, 2019, at 14:47, Rémi Denis-Courmont wrote:
> > Le mardi 19 mars 2019, 14:59:01 EET Thomas Guillem a écrit :
> > > Wait from Play() if the server don't ask for more data (like every other
> > > aout modules).
> > 
> > So far this could not be a problem because PA would always allocate a
> > larger buffer than VLC core ever will use. Did something change to break
> > that assumption?
> 
> You are right, it was really hard to trigger an overflow before but:
>  - It is easy to trigger it you increase file or network caching
>  - Some buggy input could trigger it (but since it's buggy, do we care about
> a flush triggered by an overflow?) - When testing a corner case of gapless
> playback (will send it soon in the ML): when playing lot of files that have
> a lower size than the input cache: in that case, we won't be paced by the
> input, but by the aout. - We will need it for 5.0 (always pace from
> aout/vout).
> 
> > > Note: tlength (target length of the buffer) is increased from 120ms to
> > > 2s.
> > > Before this commit, the average audio delay (time_get) was around 500ms
> > > (file caching and input dependant). If we don't increase it, the delay
> > > of
> > > the stream will be arround 120ms (since we now follow PulseAudio write
> > > requests).
> > 
> > Are you sure this is safe and minreq is enough to keep low latency cases
> > working?
> 
> tlength is already set like that in case of low latency:
> 
>         /* Setup low latency in order to quickly react to ambisonics
>          * filters viewpoint changes. */
>         flags |= PA_STREAM_ADJUST_LATENCY;
>         attr.tlength = pa_usec_to_bytes(3 * AOUT_MIN_PREPARE_TIME, &ss);
> 
> > > Setting tlength to 2sec will restore the previous time_get
> > > behavior: delay of arround 500ms, and always lower than 2sec (when
> > > file-caching is increased). This fixes potential overflow.
> > > ---
> > > 
> > >  modules/audio_output/pulse.c | 27 ++++++++++++++++++++++++++-
> > >  1 file changed, 26 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/modules/audio_output/pulse.c b/modules/audio_output/pulse.c
> > > index cc4af1e8c0..961fa7129a 100644
> > > --- a/modules/audio_output/pulse.c
> > > +++ b/modules/audio_output/pulse.c
> > > @@ -69,6 +69,7 @@ typedef struct
> > > 
> > >      pa_time_event *trigger; /**< Deferred stream trigger */
> > >      pa_cvolume cvolume; /**< actual sink input volume */
> > >      vlc_tick_t last_date; /**< Play system timestamp of last buffer */
> > > 
> > > +    size_t write_request; /**< Write request from PulseAudio */
> > > 
> > >      pa_volume_t volume_force; /**< Forced volume (stream must be NULL)
> > >      */
> > >      pa_stream_flags_t flags_force; /**< Forced flags (stream must be
> > >      NULL)
> > > 
> > > */ @@ -357,6 +358,15 @@ static void stream_underflow_cb(pa_stream *s,
> > > void
> > > *userdata) (void) s;
> > > 
> > >  }
> > > 
> > > +static void stream_request_cb(pa_stream *s, size_t nbytes, void
> > > *userdata)
> > > +{
> > > +    audio_output_t *aout = userdata;
> > > +    aout_sys_t *sys = aout->sys;
> > > +
> > > +    sys->write_request = nbytes;
> > > +    pa_threaded_mainloop_signal(sys->mainloop, 0);
> > > +}
> > > +
> > > 
> > >  static int stream_wait(pa_stream *stream, pa_threaded_mainloop
> > >  *mainloop)
> > >  {
> > >  
> > >      pa_stream_state_t state;
> > > 
> > > @@ -491,6 +501,13 @@ static void Play(audio_output_t *aout, block_t
> > > *block,
> > > vlc_tick_t date) void *ptr;
> > > 
> > >          size_t len = block->i_buffer;
> > > 
> > > +        while (sys->write_request == 0)
> > > +            pa_threaded_mainloop_wait(sys->mainloop);
> > > +
> > > +        if (len > sys->write_request)
> > > +            len = sys->write_request;
> > > +        sys->write_request -= len;
> > > +
> > 
> > Does this not need a loop to operate properly?
> 
> Its is already in a loop:
> while (block->i_buffer > 0) {
>     ...
>     block->i_buffer -= len;
> }
> 

OK then

-- 
Rémi Denis-Courmont