[FFmpeg-devel] [PATCH] lavd/pulse_audio_enc: add buffer length control options

[Lukasz M]

On 24 November 2013 21:31, Stefano Sabatini <stefasab at gmail.com> wrote:

> On date Sunday 2013-11-24 02:35:33 +0100, Lukasz Marek encoded:
> > Add options to control the length of the PulseAudio buffer.
> >
> > Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> > ---
> >  doc/outdevs.texi              |    9 +++++++++
> >  libavdevice/pulse_audio_enc.c |   17 +++++++++++++++++
> >  libavdevice/version.h         |    2 +-
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/outdevs.texi b/doc/outdevs.texi
> > index c737225..1255d30 100644
> > --- a/doc/outdevs.texi
> > +++ b/doc/outdevs.texi
> > @@ -164,6 +164,15 @@ by default it is set to the specified output name.
> >  Specify the device to use. Default device is used when not provided.
> >  List of output devices can be obtained with command @command{pactl list
> sinks}.
> >
> > + at item buffer_length
> > + at item buffer_duration
> > +Control the length and duration of the PulseAudio buffer. A small buffer
> > +gives more control, but requires more frequent updates.
>
> > +buffer_length specifies length in bytes while buffer_duration specifies
> length in milliseconds.
>
> @option{buffer_length} ... @option{buffer_duration} ...
>
> > +When both options are provided then the highest value is used (duration
> is recalculated to bytes using stream parameters).
> > +If they are set to 0 (which is default), the device will use the
> default PulseAudio duration value.
> > +By default PulseAudio set buffer duration to around 2 seconds.
> > +
> >  @end table
> >
> >  @subsection Examples
> > diff --git a/libavdevice/pulse_audio_enc.c
> b/libavdevice/pulse_audio_enc.c
> > index a2abc4a..192a046 100644
> > --- a/libavdevice/pulse_audio_enc.c
> > +++ b/libavdevice/pulse_audio_enc.c
> > @@ -35,6 +35,8 @@ typedef struct PulseData {
> >      const char *device;
> >      pa_simple *pa;
> >      int64_t timestamp;
> > +    unsigned buffer_length;
> > +    unsigned buffer_duration;
> >  } PulseData;
> >
> >  static av_cold int pulse_write_header(AVFormatContext *h)
> > @@ -59,6 +61,19 @@ static av_cold int pulse_write_header(AVFormatContext
> *h)
> >              stream_name = "Playback";
> >      }
> >
> > +    if (s->buffer_duration) {
> > +        int64_t bytes = s->buffer_duration;
> > +        bytes *= st->codec->channels * st->codec->sample_rate *
> > +                 av_get_bytes_per_sample(st->codec->sample_fmt);
> > +        bytes /= 1000;
> > +        attr.tlength = FFMAX(s->buffer_length, av_clip64(bytes, 0,
> 0xFFFFFFFE));
>
> 0xFFFFFFFE -> UINT32_MAX-1 ?
>

changed


> why -1?
>

PulseAudio will use its defaults when UINT32_MAX is passed.


>
> > +        av_log(s, AV_LOG_DEBUG,
> > +               "Buffer duration: %ums recalculated into %"PRId64" bytes
> buffer.\n",
> > +               s->buffer_duration, bytes);
> > +        av_log(s, AV_LOG_DEBUG, "Real buffer length is %u bytes\n",
> attr.tlength);
> > +    } else if (s->buffer_length)
> > +        attr.tlength = s->buffer_length;
> > +
> >      ss.format = ff_codec_id_to_pulse_format(st->codec->codec_id);
> >      ss.rate = st->codec->sample_rate;
> >      ss.channels = st->codec->channels;
> > @@ -142,6 +157,8 @@ static const AVOption options[] = {
> >      { "name",          "set application name",   OFFSET(name),
>  AV_OPT_TYPE_STRING, {.str = LIBAVFORMAT_IDENT},  0, 0, E },
> >      { "stream_name",   "set stream description", OFFSET(stream_name),
> AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, E },
> >      { "device",        "set device name",        OFFSET(device),
>  AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, E },
>
> > +    { "buffer_length",   "set buffer length in bytes",
> OFFSET(buffer_length),   AV_OPT_TYPE_INT, {.i64 = 0}, 0, 0xFFFFFFFE, E },
>
> nit++: buffer_size if probably more clear, since "length" is usually
> associated to a string length.
>

changed.


>
> Also 0xFFFFFFFE should be replaced by a symbolic constants for better
> readability. Also probably if you want to store an uint32_t you should
> probably use TYPE_INT64.
>
> > +    { "buffer_duration", "set buffer length in msecs",
> OFFSET(buffer_duration), AV_OPT_TYPE_INT, {.i64 = 0}, 0, UINT_MAX, E },
>
> could overflow in case value > INT_MAX (in general with TYPE_INT you
> should use max <= INT_MAX).
>

I changed both max limits set to INT_MAX. It is more than enough for audio
buffer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavd-pulse_audio_enc-add-buffer-size-control-options.patch
Type: text/x-patch
Size: 3975 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131126/311eb0c1/attachment.bin>