[FFmpeg-devel] [PATCH] avdevice/pulse_audio_dec: reimplement using the non simple API

[Lukasz Marek]

On 8 July 2014 05:50, Michael Niedermayer <michaelni at gmx.at> wrote:

> This fixes timestamps
>
> Based-on: code from pulseaudio
> Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> ---
>  libavdevice/pulse_audio_dec.c |  284
> +++++++++++++++++++++++++++++++++--------
>  1 file changed, 232 insertions(+), 52 deletions(-)
>
> diff --git a/libavdevice/pulse_audio_dec.c b/libavdevice/pulse_audio_dec.c
> index 414d73b..756ef4c 100644
> --- a/libavdevice/pulse_audio_dec.c
> +++ b/libavdevice/pulse_audio_dec.c
> @@ -1,6 +1,8 @@
>  /*
>   * Pulseaudio input
>   * Copyright (c) 2011 Luca Barbato <lu_zero at gentoo.org>
> + * Copyright 2004-2006 Lennart Poettering
> + * Copyright (c) 2014 Michael Niedermayer <michaelni at gmx.at>
>   *
>   * This file is part of FFmpeg.
>   *
> @@ -19,19 +21,15 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
>   */
>
> -/**
> - * @file
> - * PulseAudio input using the simple API.
> - * @author Luca Barbato <lu_zero at gentoo.org>
> - */
> -
>  #include <pulse/simple.h>
>

You can remove this header.


>  #include <pulse/rtclock.h>
>  #include <pulse/error.h>
>  #include "libavformat/avformat.h"
>  #include "libavformat/internal.h"
>  #include "libavutil/opt.h"
> +#include "libavutil/time.h"
>  #include "pulse_audio_common.h"
> +#include "timefilter.h"
>
>  #define DEFAULT_CODEC_ID AV_NE(AV_CODEC_ID_PCM_S16BE,
> AV_CODEC_ID_PCM_S16LE)
>
> @@ -44,17 +42,103 @@ typedef struct PulseData {
>      int  channels;
>      int  frame_size;
>      int  fragment_size;
> -    pa_simple *s;
> -    int64_t pts;
> -    int64_t frame_duration;
> +
> +    pa_threaded_mainloop *mainloop;
> +    pa_context *context;
> +    pa_stream *stream;
> +
> +    TimeFilter *timefilter;
> +    int last_period;
>  } PulseData;
>
> +
> +#define CHECK_SUCCESS_GOTO(p, rerror, expression, label)        \
>

p is not used.


> +    do {                                                        \
> +        if (!(expression)) {                                    \
> +            rerror = -1;                                        \
> +            goto label;                                         \
> +        }                                                       \
> +    } while(0);
> +
> +#define CHECK_DEAD_GOTO(p, rerror, label)                               \
> +    do {                                                                \
> +        if (!(p)->context ||
> !PA_CONTEXT_IS_GOOD(pa_context_get_state((p)->context)) || \
> +            !(p)->stream ||
> !PA_STREAM_IS_GOOD(pa_stream_get_state((p)->stream))) { \
> +            if (((p)->context && pa_context_get_state((p)->context) ==
> PA_CONTEXT_FAILED) || \
> +                ((p)->stream && pa_stream_get_state((p)->stream) ==
> PA_STREAM_FAILED)) { \
> +                rerror = -1;                                            \
> +            } else                                                      \
> +                rerror = -1;                                            \
>

Is this "if else" really needed?
If -1 is error code than maybe AVERROR_EXTERNAL?, here and above.


> +            goto label;                                                 \
> +        }                                                               \
> +    } while(0);
> +
>
[...]

> +static av_cold int pulse_close(AVFormatContext *s)
> +{
> +    PulseData *pd = s->priv_data;
> +
> +    if (pd->mainloop)
> +        pa_threaded_mainloop_stop(pd->mainloop);
> +
> +    if (pd->stream)
> +        pa_stream_unref(pd->stream);
> +
> +    if (pd->context) {
> +        pa_context_disconnect(pd->context);
> +        pa_context_unref(pd->context);
> +    }
> +
> +    if (pd->mainloop)
> +        pa_threaded_mainloop_free(pd->mainloop);
>

You can set to NULL pd->mainloop, pd->context and pd->stream to make this
function idiot proof.


> +
> +    ff_timefilter_destroy(pd->timefilter);
> +    pd->timefilter = NULL;
> +
> +    return 0;
> +}
> +
>  static av_cold int pulse_read_header(AVFormatContext *s)
>  {
>      PulseData *pd = s->priv_data;
>      AVStream *st;
>      char *device = NULL;
> -    int ret, sample_bytes;
> +    int ret;
>      enum AVCodecID codec_id =
>          s->audio_codec_id == AV_CODEC_ID_NONE ? DEFAULT_CODEC_ID :
> s->audio_codec_id;
>      const pa_sample_spec ss = { ff_codec_id_to_pulse_format(codec_id),
> @@ -75,77 +159,173 @@ static av_cold int pulse_read_header(AVFormatContext
> *s)
>      if (strcmp(s->filename, "default"))
>          device = s->filename;
>
> -    pd->s = pa_simple_new(pd->server, pd->name,
> -                          PA_STREAM_RECORD,
> -                          device, pd->stream_name, &ss,
> -                          NULL, &attr, &ret);
> +    if (!(pd->mainloop = pa_threaded_mainloop_new())) {
> +        pulse_close(s);
> +        return -1;
>

Maybe AVERROR_EXTERNAL?


> +    }
> +
> +    if (!(pd->context =
> pa_context_new(pa_threaded_mainloop_get_api(pd->mainloop), pd->name))) {
> +        pulse_close(s);
> +        return -1;
>

Maybe AVERROR_EXTERNAL?


> +    }
> +
> +    pa_context_set_state_callback(pd->context, context_state_cb, pd);
> +
> +    if (pa_context_connect(pd->context, pd->server, 0, NULL) < 0) {
> +        pulse_close(s);
> +        return AVERROR(pa_context_errno(pd->context));
> +    }
> +
> +    pa_threaded_mainloop_lock(pd->mainloop);
>

[...]


> -    pd->pts += pd->frame_duration;
> +    if (pa_stream_get_latency(pd->stream, &latency, &negative) >= 0) {
> +        enum AVCodecID codec_id =
> +            s->audio_codec_id == AV_CODEC_ID_NONE ? DEFAULT_CODEC_ID :
> s->audio_codec_id;
> +        int frame_size = ((av_get_bits_per_sample(codec_id) >> 3) *
> pd->channels);
> +        int frame_duration = read_length / frame_size;
>
> -    return 0;
> -}
>
> -static av_cold int pulse_close(AVFormatContext *s)
> -{
> -    PulseData *pd = s->priv_data;
> -    pa_simple_free(pd->s);
> -    pd->s = NULL;
> +        dts -= latency;
>

you should take negative variable into account too. As far as I remember it
may be set for record streams when monitor device is recorded.


> +        pkt->pts = ff_timefilter_update(pd->timefilter, dts,
> pd->last_period);
> +
> +        pd->last_period = frame_duration;
> +    } else {
> +        av_log(s, AV_LOG_WARNING, "pa_stream_get_latency() failed\n");
> +    }
> +
> +    memcpy(pkt->data, read_data, read_length);
> +    pa_stream_drop(pd->stream);
> +
> +    pa_threaded_mainloop_unlock(pd->mainloop);
>      return 0;
> +
> +unlock_and_fail:
> +    pa_threaded_mainloop_unlock(pd->mainloop);
> +    return ret;
>  }
>
>  static int pulse_get_device_list(AVFormatContext *h, AVDeviceInfoList
> *device_list)
>

Remark not necessarily for this patch, but:
Since both input and output device dropped simple API, configure may be
also updated to not require simpleAPI lib.
This should remove one shared library from linking.