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

[Thomas Guillem]

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


> 
> >          if (pa_stream_begin_write(s, &ptr, &len))
> >              vlc_pa_error(aout, "cannot begin write", sys->context);
> > 
> > @@ -540,6 +557,8 @@ static void Flush(audio_output_t *aout)
> > 
> >      pa_threaded_mainloop_lock(sys->mainloop);
> > 
> > +    sys->write_request = 0;
> > +
> >      pa_operation *op = pa_stream_flush(s, NULL, NULL);
> >      if (op != NULL)
> >          pa_operation_unref(op);
> > @@ -555,6 +574,9 @@ static void Drain(audio_output_t *aout)
> >      pa_stream *s = sys->stream;
> > 
> >      pa_threaded_mainloop_lock(sys->mainloop);
> > +
> > +    sys->write_request = 0;
> > +
> >      pa_operation *op = pa_stream_drain(s, NULL, NULL);
> >      if (op != NULL)
> >          pa_operation_unref(op);
> > @@ -778,7 +800,7 @@ static int Start(audio_output_t *aout,
> > audio_sample_format_t *restrict fmt) /* PulseAudio goes berserk if the
> > target length (tlength) is not * significantly longer than 2 periods
> > (minreq), or when the period length * is unspecified and the target length
> > is short. */
> > -    attr.tlength = pa_usec_to_bytes(3 * AOUT_MIN_PREPARE_TIME, &ss);
> > +    attr.tlength = pa_usec_to_bytes(AOUT_MAX_PREPARE_TIME, &ss);
> >      attr.prebuf = 0; /* trigger manually */
> >      attr.minreq = pa_usec_to_bytes(AOUT_MIN_PREPARE_TIME, &ss);
> >      attr.fragsize = 0; /* not used for output */
> > @@ -790,6 +812,7 @@ static int Start(audio_output_t *aout,
> > audio_sample_format_t *restrict fmt) pa_cvolume_set(cvolume, ss.channels,
> > sys->volume_force);
> >      }
> > 
> > +    sys->write_request = 0;
> >      sys->trigger = NULL;
> >      pa_cvolume_init(&sys->cvolume);
> >      sys->last_date = VLC_TICK_INVALID;
> > @@ -919,6 +942,7 @@ static int Start(audio_output_t *aout,
> > audio_sample_format_t *restrict fmt) pa_stream_set_started_callback(s,
> > stream_started_cb, aout);
> >      pa_stream_set_suspended_callback(s, stream_suspended_cb, aout);
> >      pa_stream_set_underflow_callback(s, stream_underflow_cb, aout);
> > +    pa_stream_set_write_callback(s, stream_request_cb, aout);
> > 
> >      if (pa_stream_connect_playback(s, sys->sink_force, &attr, flags,
> >                                     cvolume, NULL) < 0
> > @@ -976,6 +1000,7 @@ static void Stop(audio_output_t *aout)
> >      pa_stream_set_started_callback(s, NULL, NULL);
> >      pa_stream_set_suspended_callback(s, NULL, NULL);
> >      pa_stream_set_underflow_callback(s, NULL, NULL);
> > +    pa_stream_set_write_callback(s, NULL, NULL);
> > 
> >      pa_stream_unref(s);
> >      sys->stream = NULL;
> 
> 
> -- 
> Rémi Denis-Courmont
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel