diff mbox series

DO-NOT-MERGE: pipewire sample code

Message ID 20230311120826.5584-1-vr_qemu@t-online.de
State New
Headers show
Series DO-NOT-MERGE: pipewire sample code | expand

Commit Message

Volker Rümelin March 11, 2023, 12:08 p.m. UTC
Based-on: <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(-)

Comments

Volker Rümelin March 11, 2023, 4:19 p.m. UTC | #1
> Based-on:<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:
Dorinda Bassey March 13, 2023, 12:28 p.m. UTC | #2
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_qemu@t-online.de> wrote:

> > Based-on:<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:
>
>
Volker Rümelin March 13, 2023, 8:05 p.m. UTC | #3
Am 13.03.23 um 13:28 schrieb Dorinda Bassey:
> 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.

Hi Dorinda,

there has to be a control mechanism that ensures that our write rate on 
average is exactly the frame rate that the down stream audio device 
writes to the DAC. My question was how can this work if we always write 
4096 frames.

The answer is, that after a 4096 frames write, the callback is delayed 
by 4096 frames / 44100 frames/s = 93ms. This ensures that our write rate 
is exactly 44100 frames/s.

This means a fixed 4096 frames write is wrong for the req == 0 case. We 
have to write 75% of timer-period frames.

If you want to test this yourself, just ignore req and assume it's 0.

With best regards,
Volker

>
>     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_qemu@t-online.de> 
> wrote:
>
>     > Based-on:<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.
>
Dorinda Bassey March 14, 2023, 11:50 a.m. UTC | #4
Hi Volker,

Thank you for the clarification. I see the problem now.
So is it safe to say that:

@@ -104,8 +104,9 @@ playback_on_process(void *data)
     /* calculate the total no of bytes to read data from buffer */
     req = b->requested * v->frame_size;
     if (req == 0) {
-        req = 4096 * v->frame_size;
+        req = v->g->dev->timer_period * v->info.rate * v->frame_size * 1 /
2 / 1000000;
     }

I used 50% of the timer-period and the frame_size which would give a close
margin to what the downstream audio device writes to the DAC.

Thanks,
Dorinda.

On Mon, Mar 13, 2023 at 9:06 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> Am 13.03.23 um 13:28 schrieb Dorinda Bassey:
> > 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.
>
> Hi Dorinda,
>
> there has to be a control mechanism that ensures that our write rate on
> average is exactly the frame rate that the down stream audio device
> writes to the DAC. My question was how can this work if we always write
> 4096 frames.
>
> The answer is, that after a 4096 frames write, the callback is delayed
> by 4096 frames / 44100 frames/s = 93ms. This ensures that our write rate
> is exactly 44100 frames/s.
>
> This means a fixed 4096 frames write is wrong for the req == 0 case. We
> have to write 75% of timer-period frames.
>
> If you want to test this yourself, just ignore req and assume it's 0.
>
> With best regards,
> Volker
>
> >
> >     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_qemu@t-online.de>
> > wrote:
> >
> >     > Based-on:<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.
> >
>
>
Volker Rümelin March 14, 2023, 7:26 p.m. UTC | #5
Am 14.03.23 um 12:50 schrieb Dorinda Bassey:
> Hi Volker,
>
> Thank you for the clarification. I see the problem now.
> So is it safe to say that:
>
> @@ -104,8 +104,9 @@ playback_on_process(void *data)
>      /* calculate the total no of bytes to read data from buffer */
>      req = b->requested * v->frame_size;
>      if (req == 0) {
> -        req = 4096 * v->frame_size;
> +        req = v->g->dev->timer_period * v->info.rate * v->frame_size 
> * 1 / 2 / 1000000;
>      }
>
> I used 50% of the timer-period and the frame_size which would give a 
> close margin to what the downstream audio device writes to the DAC.

Hi,

50% is fine by me. But you should rearrange the term. The multiplication 
by v->frame_size should come last, otherwise it's not guaranteed that 
req is a multiple of v->frame_size. I would cast timer_period to 
uint64_t. Then timer_period * info.rate has a 32bit * 32bit => 64bit 
result. 20000 us * 256000 frames/s already has more than 32 bits.

+        req = (uint64_t)v->g->dev->timer_period * v->info.rate * 1 / 2 
/ 1000000 * v->frame_size;

With best regards,
Volker

>
> Thanks,
> Dorinda.
>
> On Mon, Mar 13, 2023 at 9:06 PM Volker Rümelin <vr_qemu@t-online.de> 
> wrote:
>
>     Am 13.03.23 um 13:28 schrieb Dorinda Bassey:
>     > 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.
>
>     Hi Dorinda,
>
>     there has to be a control mechanism that ensures that our write
>     rate on
>     average is exactly the frame rate that the down stream audio device
>     writes to the DAC. My question was how can this work if we always
>     write
>     4096 frames.
>
>     The answer is, that after a 4096 frames write, the callback is
>     delayed
>     by 4096 frames / 44100 frames/s = 93ms. This ensures that our
>     write rate
>     is exactly 44100 frames/s.
>
>     This means a fixed 4096 frames write is wrong for the req == 0
>     case. We
>     have to write 75% of timer-period frames.
>
>     If you want to test this yourself, just ignore req and assume it's 0.
>
>     With best regards,
>     Volker
>
>     >
>     >     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_qemu@t-online.de>
>     > wrote:
>     >
>     >     > Based-on:<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.
>     >
>
diff mbox series

Patch

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;
     }
-    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);
 
     /* 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;
+}
+
 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);
 
     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);
 
     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);
 
     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' } }
 
 ##
 # @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: