[vlc-devel] [PATCH] pulse: avoid overflows

[Thomas Guillem]

On Mon, Feb 11, 2019, at 17:18, Rémi Denis-Courmont wrote:
> Le maanantaina 11. helmikuuta 2019, 18.11.04 EET Thomas Guillem a écrit :
> > Compare the maxlength and the delay in order to detect if we should pace
> > from the Play() function. Overflows never happened with the default VLC
> > configuration, but it can be triggered easily when the input buffering is
> > tweaked. Furthermore, this will allow interactive playback with a low
> > maxlength value.
> > 
> > Most common audio outputs modules (on Windows, Android, macOS) are already
> > pacing from the play callback.
> > 
> > This is also a small step for the input clock/buffering rework since we may
> > want to pace from the output in the future.
> > ---
> >  modules/audio_output/pulse.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/modules/audio_output/pulse.c b/modules/audio_output/pulse.c
> > index 8c024006af..d20553c92c 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 */
> > +    vlc_tick_t maxlength; /**< Maximum audio buffer length */
> > 
> >      pa_volume_t volume_force; /**< Forced volume (stream must be NULL) */
> >      pa_stream_flags_t flags_force; /**< Forced flags (stream must be NULL)
> > */ @@ -282,8 +283,10 @@ static void stream_state_cb(pa_stream *s, void
> > *userdata) static void stream_buffer_attr_cb(pa_stream *s, void *userdata)
> >  {
> >      audio_output_t *aout = userdata;
> > +    aout_sys_t *sys = aout->sys;
> >      const pa_buffer_attr *pba = pa_stream_get_buffer_attr(s);
> > 
> > +    sys->maxlength = pba->maxlength;
> >      msg_Dbg(aout, "changed buffer metrics: maxlength=%u, tlength=%u, "
> >              "prebuf=%u, minreq=%u",
> >              pba->maxlength, pba->tlength, pba->prebuf, pba->minreq);
> > @@ -491,6 +494,20 @@ static void Play(audio_output_t *aout, block_t *block,
> > vlc_tick_t date) void *ptr;
> >          size_t len = block->i_buffer;
> > 
> > +        /* Check if we should pace to prevent an overflow */
> > +        if (pa_stream_is_corked(s) <= 0)
> > +        {
> > +            assert(sys->maxlength != -1);
> > +            const pa_sample_spec *ss = pa_stream_get_sample_spec(s);
> > +            vlc_tick_t len_us = pa_usec_to_bytes(len, ss);
> > +            while (len_us + vlc_pa_get_latency(aout, sys->context, s)
> > +                   > sys->maxlength)
> > +            {
> > +                vlc_tick_t sleep_us = __MAX(len_us, sys->maxlength / 100);
> > +                vlc_tick_sleep(sleep_us);
> > +            }
> > +        }
> 
> This looks like it will underflow the buffer if the block is larger than the 
> current buffering. There should be an underflow callback to handle overflow.

haha, or maybe add a way to play a small part of the buffer.

> 
> But this will hold the audio controls during pacing. The audio output 
> threading first needs to be adjusted so that controls remain available while 
> play() is blocking.

Like having play() returning EGAIN (after modifying p_block offset) ?

I don't know if this change is for 4.0 or 5.0. If it's for 5.0, we should merge this commit anyway, in order to have the same behavior for all (or most commons) audio outputs (that is: waiting from play()).

> 
> > +
> >          if (pa_stream_begin_write(s, &ptr, &len))
> >              vlc_pa_error(aout, "cannot begin write", sys->context);
> > 
> > @@ -772,7 +789,7 @@ static int Start(audio_output_t *aout,
> > audio_sample_format_t *restrict fmt, assert(*period >=
> > AOUT_MAX_PTS_ADVANCE);
> > 
> >      struct pa_buffer_attr attr;
> > -    attr.maxlength = -1;
> > +    sys->maxlength = attr.maxlength = -1;
> >      /* 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. */
> 
> 
> -- 
> 雷米‧德尼-库尔蒙
> http://www.remlab.net/
> 
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel