Hi Volker,
Thanks for the patch, I've tested the patch and it works. I don't hear the
choppy audio with this option "qemu-system-x86_64 -device ich9-intel-hda
-device hda-duplex,audiodev=audio0 -audiodev
pipewire,id=audio0,out.frequency=96000,in.frequency=96000 ...."
I don't understand how the req == 0 case can work at all.
>
how this works is that b->requested could be zero when no suggestion is
provided. For playback streams, this field contains the suggested amount of
data to provide. hence the reason for this check.
I suggest to use the same option names as the pulseaudio backend.
> out.latency is the effective Pipewire buffer size.
>
Ack.
Thanks,
Dorinda.
On Sat, Mar 11, 2023 at 5:19 PM Volker Rümelin <vr_q...@t-online.de> wrote:
> > Based-on:<20230306171020.381116-1-dbas...@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_q...@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:
>
>
