|
| From: | Dorinda Bassey |
| Subject: | Re: [PATCH] DO-NOT-MERGE: pipewire sample code |
| Date: | Mon, 13 Mar 2023 13:28:00 +0100 |
I don't understand how the req == 0 case can work at all.
I suggest to use the same option names as the pulseaudio backend.
out.latency is the effective Pipewire buffer size.
> Based-on:<20230306171020.381116-1-dbassey@redhat.com" target="_blank">20230306171020.381116-1-dbassey@redhat.com>
> ([PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU)
>
> This is sample code for the review of the pipewire backed. The
> code actually works.
>
> An email with explanations for the changes will follow.
>
> Signed-off-by: Volker Rümelin<vr_qemu@t-online.de>
> ---
> audio/pwaudio.c | 67 +++++++++++++++++++++++++++++++++----------------
> qapi/audio.json | 10 +++-----
> 2 files changed, 49 insertions(+), 28 deletions(-)
>
> diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> index d357761152..8e2a38938f 100644
> --- a/audio/pwaudio.c
> +++ b/audio/pwaudio.c
> @@ -23,7 +23,6 @@
> #define AUDIO_CAP "pipewire"
> #define RINGBUFFER_SIZE (1u << 22)
> #define RINGBUFFER_MASK (RINGBUFFER_SIZE - 1)
> -#define BUFFER_SAMPLES 512
>
> #include "audio_int.h"
>
> @@ -48,6 +47,7 @@ typedef struct PWVoice {
> struct pw_stream *stream;
> struct spa_hook stream_listener;
> struct spa_audio_info_raw info;
> + uint32_t highwater_mark;
> uint32_t frame_size;
> struct spa_ringbuffer ring;
> uint8_t buffer[RINGBUFFER_SIZE];
> @@ -82,7 +82,7 @@ playback_on_process(void *data)
> void *p;
> struct pw_buffer *b;
> struct spa_buffer *buf;
> - uint32_t n_frames, req, index, n_bytes;
> + uint32_t req, index, n_bytes;
> int32_t avail;
>
> if (!v->stream) {
> @@ -105,8 +105,7 @@ playback_on_process(void *data)
> if (req == 0) {
> req = 4096 * v->frame_size;
> }
I don't understand how the req == 0 case can work at all. The downstream
audio device is the thinnest point in the playback stream. We can't
write more audio frames than the audio device will consume.
> - n_frames = SPA_MIN(req, buf->datas[0].maxsize);
> - n_bytes = n_frames * v->frame_size;
> + n_bytes = SPA_MIN(req, buf->datas[0].maxsize);
See Marc-André's review.
>
> /* get no of available bytes to read data from buffer */
>
> @@ -270,6 +269,30 @@ done_unlock:
> return l;
> }
>
> +static size_t qpw_buffer_get_free(HWVoiceOut *hw)
> +{
> + PWVoiceOut *pw = (PWVoiceOut *)hw;
> + PWVoice *v = &pw->v;
> + pwaudio *c = v->g;
> + const char *error = NULL;
> + int32_t filled, avail;
> + uint32_t index;
> +
> + pw_thread_loop_lock(c->thread_loop);
> + if (pw_stream_get_state(v->stream, &error) != PW_STREAM_STATE_STREAMING) {
> + /* wait for stream to become ready */
> + avail = 0;
> + goto done_unlock;
> + }
> +
> + filled = spa_ringbuffer_get_write_index(&v->ring, &index);
> + avail = v->highwater_mark - filled;
> +
> +done_unlock:
> + pw_thread_loop_unlock(c->thread_loop);
> + return avail;
> +}
> +
A pcm_ops buffer_get_free function is necessary. Otherwise the gus and
via-ac97 audio devices will not work properly for the -audiodev
pipewire,id=audio0,out.mixing-engine=off case. They need to know in
advance how many bytes they can write.
Also, without the buffer_get_free function, the generic audio buffer
will increase the playback latency.
> static size_t
> qpw_write(HWVoiceOut *hw, void *data, size_t len)
> {
> @@ -277,20 +300,18 @@ qpw_write(HWVoiceOut *hw, void *data, size_t len)
> PWVoice *v = &pw->v;
> pwaudio *c = v->g;
> const char *error = NULL;
> - const int periods = 3;
> - size_t l;
> int32_t filled, avail;
> uint32_t index;
>
> pw_thread_loop_lock(c->thread_loop);
> if (pw_stream_get_state(v->stream, &error) != PW_STREAM_STATE_STREAMING) {
> /* wait for stream to become ready */
> - l = 0;
> + len = 0;
> goto done_unlock;
> }
> - filled = spa_ringbuffer_get_write_index(&v->ring, &index);
>
> - avail = BUFFER_SAMPLES * v->frame_size * periods - filled;
> + filled = spa_ringbuffer_get_write_index(&v->ring, &index);
> + avail = v->highwater_mark - filled;
>
> trace_pw_write(filled, avail, index, len);
>
> @@ -312,11 +333,10 @@ qpw_write(HWVoiceOut *hw, void *data, size_t len)
> index & RINGBUFFER_MASK, data, len);
> index += len;
> spa_ringbuffer_write_update(&v->ring, index);
> - l = len;
>
> done_unlock:
> pw_thread_loop_unlock(c->thread_loop);
> - return l;
> + return len;
> }
>
> static int
> @@ -420,8 +440,13 @@ create_stream(pwaudio *c, PWVoice *v, const char *name)
> const struct spa_pod *params[2];
> uint8_t buffer[1024];
> struct spa_pod_builder b;
> + struct pw_properties *props;
>
> - v->stream = pw_stream_new(c->core, name, NULL);
> + props = pw_properties_new(NULL, NULL);
> + pw_properties_setf(props, PW_KEY_NODE_LATENCY, "%" PRIu64 "/%u",
> + (uint64_t)v->g->dev->timer_period * v->info.rate
> + * 3 / 4 / 1000000, v->info.rate);
> + v->stream = pw_stream_new(c->core, name, props);
The PW_KEY_NODE_LATENCY property is a hint for Pipewire that we need
updates faster than timer_period. I selected 75% of timer-period. Then
there's a good chance the audio frontends can write or read new audio
packets every timer-period. It doesn't matter that Pipewire calls the
callbacks faster in most cases.
If it turns out that Pipewire often can't even approximately fulfill
this hint, we will additionally need a jitter buffer implementation to
split the larger Pipewire audio packets into timer-period sized packets.
>
> if (v->stream == NULL) {
> goto error;
> @@ -563,7 +588,11 @@ qpw_init_out(HWVoiceOut *hw, struct audsettings *as, void *drv_opaque)
> audio_pcm_init_info(&hw->info, &obt_as);
>
> /* report the buffer size to qemu */
> - hw->samples = BUFFER_SAMPLES;
> + hw->samples = audio_buffer_frames(
> + qapi_AudiodevPipewirePerDirectionOptions_base(ppdo), &obt_as, 46440);
> + v->highwater_mark = MIN(RINGBUFFER_SIZE,
> + (ppdo->has_latency ? ppdo->latency : 46440)
> + * (uint64_t)v->info.rate / 1000000 * v->frame_size);
>
The reported buffer size should be much larger than BUFFER_SAMPLES. This
gives the audio frontends a chance to catch up if they missed
timer-periods or if they have to fill the pipewire backend buffer
quickly after playback starts. The exact size is not critical, but to be
command line compatible with the pulseaudio backend, I suggest to use
46ms. A large hw->samples value doesn't increase the playback latency.
v->highwater_mark is the effective pipewire backend buffer size. At a
audio frame rate of 44100 frames/s, the code without this patch uses a
buffer size of BUFFER_SAMPLES * periods / 44100 frames/s = 512 frames *
3 / 44100 frames/s = 35ms. On my computer the buffer size has to be 30ms
at minimum. I suggest to add a good margin and use a default of 46ms.
This buffer is a larger contributor to the playback latency.
> pw_thread_loop_unlock(c->thread_loop);
> return 0;
> @@ -606,7 +635,8 @@ qpw_init_in(HWVoiceIn *hw, struct audsettings *as, void *drv_opaque)
> audio_pcm_init_info(&hw->info, &obt_as);
>
> /* report the buffer size to qemu */
> - hw->samples = BUFFER_SAMPLES;
> + hw->samples = audio_buffer_frames(
> + qapi_AudiodevPipewirePerDirectionOptions_base(ppdo), &obt_as, 46440);
>
See qpw_init_out().
> pw_thread_loop_unlock(c->thread_loop);
> return 0;
> @@ -695,15 +725,8 @@ qpw_audio_init(Audiodev *dev)
> pw = g_new0(pwaudio, 1);
> pw_init(NULL, NULL);
>
> - AudiodevPipewireOptions *popts;
> trace_pw_audio_init("Initialize Pipewire context\n");
> assert(dev->driver == AUDIODEV_DRIVER_PIPEWIRE);
> - popts = &dev->u.pipewire;
> -
> - if (!popts->has_latency) {
> - popts->has_latency = true;
> - popts->latency = 15000;
> - }
>
> pw->dev = dev;
> pw->thread_loop = pw_thread_loop_new("Pipewire thread loop", NULL);
> @@ -781,7 +804,7 @@ static struct audio_pcm_ops qpw_pcm_ops = {
> .init_out = qpw_init_out,
> .fini_out = qpw_fini_out,
> .write = qpw_write,
> - .buffer_get_free = audio_generic_buffer_get_free,
> + .buffer_get_free = qpw_buffer_get_free,
> .run_buffer_out = audio_generic_run_buffer_out,
> .enable_out = qpw_enable_out,
>
> diff --git a/qapi/audio.json b/qapi/audio.json
> index 9a0d7d9ece..d49a8a670b 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -337,6 +337,7 @@
> # create multiple Pipewire devices or run multiple qemu
> # instances (default: audiodev's id, since 7.1)
> #
> +# @latency: Pipewire backend buffer size in microseconds (default 46440)
> #
> # Since: 8.0
> ##
> @@ -344,7 +345,8 @@
> 'base': 'AudiodevPerDirectionOptions',
> 'data': {
> '*name': 'str',
> - '*stream-name': 'str' } }
> + '*stream-name': 'str',
> + '*latency': 'uint32' } }
>
I suggest to use the same option names as the pulseaudio backend.
out.latency is the effective Pipewire buffer size.
With best regards,
Volker
> ##
> # @AudiodevPipewireOptions:
> @@ -355,16 +357,12 @@
> #
> # @out: options of the playback stream
> #
> -# @latency: add latency to playback in microseconds
> -# (default 15000)
> -#
> # Since: 8.0
> ##
> { 'struct': 'AudiodevPipewireOptions',
> 'data': {
> '*in': 'AudiodevPipewirePerDirectionOptions',
> - '*out': 'AudiodevPipewirePerDirectionOptions',
> - '*latency': 'uint32' } }
> + '*out': 'AudiodevPipewirePerDirectionOptions' } }
>
> ##
> # @AudiodevSdlPerDirectionOptions:
| [Prev in Thread] | Current Thread | [Next in Thread] |