Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
On Monday, March 13, 2023 8:06:15 PM CET Dorinda Bassey wrote:
> >
> > Are you sure about sizeof(n_bytes) here? That's 4. ;-)
> >
> my bad!
>
> >
> > Volker's point was that "silence" is the center of the wave range. With
> > signed
> > range that's zero, yes, but with unsigned range that's 2^(bitdepth) / 2.
> >
> > So you need to memset() the correct value to generate "silence".
> >
> I understand now, Thanks. I guess it should work for signed range, so I
> would do:
>
> @@ -117,7 +117,9 @@ playback_on_process(void *data)
> }
>
> if (avail == 0) {
> -memset(p, 0, n_bytes);
> +memset(p, 0, (int32_t) n_bytes);
No, that would not fix anything. You are actually making several false
assumptions here.
Anyway, in audio/audio.c there is a function which does it correctly:
audio_pcm_info_clear_buf(struct audio_pcm_info *info, void *buf, int len)
So you probably just want to call this function instead to generate silence
correctly. Keep in mind though that it's `len` argument is in sample points,
not in bytes.
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Am 13.03.23 um 14:11 schrieb Dorinda Bassey:
Hi Volker,
To hear this,
start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
-device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
pipewire,id=audio0,out.mixing-engine=off ...
I hear the clipped audio stream with these options. IMO, I don't think
memset is responsible for that behaviour, I still hear the harsh sound
with "-audiodev pa". I also tried using an alternative like:
Hi Dorinda,
when you test -audiodev pa with a pulseaudio server, audio playback is
fine. Audio playback with -audiodev pa with the pipewire-pulse server is
clipped. This is a pipewire bug.
With best regards,
Volker
@@ -117,7 +118,7 @@ playback_on_process(void *data)
}
if (avail == 0) {
- memset(p, 0, n_bytes);
+ p = g_malloc0(sizeof(n_bytes));
} else {
The clipped audio issue is still persistent.
Thanks,
Dorinda.
On Sun, Mar 12, 2023 at 9:01 AM Volker Rümelin
wrote:
> +/* output data processing function to read stuffs from the
buffer */
> +static void
> +playback_on_process(void *data)
> +{
> + PWVoice *v = (PWVoice *) data;
> + void *p;
> + struct pw_buffer *b;
> + struct spa_buffer *buf;
> + uint32_t n_frames, req, index, n_bytes;
> + int32_t avail;
> +
> + if (!v->stream) {
> + return;
> + }
> +
> + /* obtain a buffer to read from */
> + b = pw_stream_dequeue_buffer(v->stream);
> + if (b == NULL) {
> + error_report("out of buffers: %s", strerror(errno));
> + return;
> + }
> +
> + buf = b->buffer;
> + p = buf->datas[0].data;
> + if (p == NULL) {
> + return;
> + }
> + req = b->requested * v->frame_size;
> + if (req == 0) {
> + req = 4096 * v->frame_size;
> + }
> + n_frames = SPA_MIN(req, buf->datas[0].maxsize);
> + n_bytes = n_frames * v->frame_size;
> +
> + /* get no of available bytes to read data from buffer */
> +
> + avail = spa_ringbuffer_get_read_index(>ring, );
> +
> + if (!v->enabled) {
> + avail = 0;
> + }
> +
> + if (avail == 0) {
> + memset(p, 0, n_bytes);
memset() doesn't work for unsigned samples. For unsigned samples, a
stream of zeros is silence with a DC offset. When Pipewire mixes this
stream with another, the result is a clipped audio stream. To hear
this,
start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
-device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
pipewire,id=audio0,out.mixing-engine=off ... and start playback
with the
hda device.
With best regards,
Volker
> + } else {
> + if (avail < (int32_t) n_bytes) {
> + n_bytes = avail;
> + }
> +
> + spa_ringbuffer_read_data(>ring,
> + v->buffer, RINGBUFFER_SIZE,
> + index & RINGBUFFER_MASK, p,
n_bytes);
> +
> + index += n_bytes;
> + spa_ringbuffer_read_update(>ring, index);
> + }
> +
> + buf->datas[0].chunk->offset = 0;
> + buf->datas[0].chunk->stride = v->frame_size;
> + buf->datas[0].chunk->size = n_bytes;
> +
> + /* queue the buffer for playback */
> + pw_stream_queue_buffer(v->stream, b);
> +}
> +
>
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
>
> Are you sure about sizeof(n_bytes) here? That's 4. ;-)
>
my bad!
>
> Volker's point was that "silence" is the center of the wave range. With
> signed
> range that's zero, yes, but with unsigned range that's 2^(bitdepth) / 2.
>
> So you need to memset() the correct value to generate "silence".
>
I understand now, Thanks. I guess it should work for signed range, so I
would do:
@@ -117,7 +117,9 @@ playback_on_process(void *data)
}
if (avail == 0) {
-memset(p, 0, n_bytes);
+memset(p, 0, (int32_t) n_bytes);
CMIIW
Thanks,
Dorinda.
On Mon, Mar 13, 2023 at 2:37 PM Christian Schoenebeck <
qemu_...@crudebyte.com> wrote:
> On Monday, March 13, 2023 2:11:11 PM CET Dorinda Bassey wrote:
> > Hi Volker,
> >
> >
> > > To hear this,
> > > start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
> > > -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
> > > pipewire,id=audio0,out.mixing-engine=off ...
> > >
> > I hear the clipped audio stream with these options. IMO, I don't think
> > memset is responsible for that behaviour, I still hear the harsh sound
> with
> > "-audiodev pa". I also tried using an alternative like:
> >
> > @@ -117,7 +118,7 @@ playback_on_process(void *data)
> > }
> >
> > if (avail == 0) {
> > -memset(p, 0, n_bytes);
> > +p = g_malloc0(sizeof(n_bytes));
> > } else {
> >
> > The clipped audio issue is still persistent.
>
> Are you sure about sizeof(n_bytes) here? That's 4. ;-)
>
> Volker's point was that "silence" is the center of the wave range. With
> signed
> range that's zero, yes, but with unsigned range that's 2^(bitdepth) / 2.
>
> So you need to memset() the correct value to generate "silence".
>
> Best regards,
> Christian Schoenebeck
>
>
>
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
On Monday, March 13, 2023 2:11:11 PM CET Dorinda Bassey wrote:
> Hi Volker,
>
>
> > To hear this,
> > start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
> > -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
> > pipewire,id=audio0,out.mixing-engine=off ...
> >
> I hear the clipped audio stream with these options. IMO, I don't think
> memset is responsible for that behaviour, I still hear the harsh sound with
> "-audiodev pa". I also tried using an alternative like:
>
> @@ -117,7 +118,7 @@ playback_on_process(void *data)
> }
>
> if (avail == 0) {
> -memset(p, 0, n_bytes);
> +p = g_malloc0(sizeof(n_bytes));
> } else {
>
> The clipped audio issue is still persistent.
Are you sure about sizeof(n_bytes) here? That's 4. ;-)
Volker's point was that "silence" is the center of the wave range. With signed
range that's zero, yes, but with unsigned range that's 2^(bitdepth) / 2.
So you need to memset() the correct value to generate "silence".
Best regards,
Christian Schoenebeck
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Hi Volker,
> To hear this,
> start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
> -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
> pipewire,id=audio0,out.mixing-engine=off ...
>
I hear the clipped audio stream with these options. IMO, I don't think
memset is responsible for that behaviour, I still hear the harsh sound with
"-audiodev pa". I also tried using an alternative like:
@@ -117,7 +118,7 @@ playback_on_process(void *data)
}
if (avail == 0) {
-memset(p, 0, n_bytes);
+p = g_malloc0(sizeof(n_bytes));
} else {
The clipped audio issue is still persistent.
Thanks,
Dorinda.
On Sun, Mar 12, 2023 at 9:01 AM Volker Rümelin wrote:
> > +/* output data processing function to read stuffs from the buffer */
> > +static void
> > +playback_on_process(void *data)
> > +{
> > +PWVoice *v = (PWVoice *) data;
> > +void *p;
> > +struct pw_buffer *b;
> > +struct spa_buffer *buf;
> > +uint32_t n_frames, req, index, n_bytes;
> > +int32_t avail;
> > +
> > +if (!v->stream) {
> > +return;
> > +}
> > +
> > +/* obtain a buffer to read from */
> > +b = pw_stream_dequeue_buffer(v->stream);
> > +if (b == NULL) {
> > +error_report("out of buffers: %s", strerror(errno));
> > +return;
> > +}
> > +
> > +buf = b->buffer;
> > +p = buf->datas[0].data;
> > +if (p == NULL) {
> > +return;
> > +}
> > +req = b->requested * v->frame_size;
> > +if (req == 0) {
> > +req = 4096 * v->frame_size;
> > +}
> > +n_frames = SPA_MIN(req, buf->datas[0].maxsize);
> > +n_bytes = n_frames * v->frame_size;
> > +
> > +/* get no of available bytes to read data from buffer */
> > +
> > +avail = spa_ringbuffer_get_read_index(>ring, );
> > +
> > +if (!v->enabled) {
> > +avail = 0;
> > +}
> > +
> > +if (avail == 0) {
> > +memset(p, 0, n_bytes);
>
> memset() doesn't work for unsigned samples. For unsigned samples, a
> stream of zeros is silence with a DC offset. When Pipewire mixes this
> stream with another, the result is a clipped audio stream. To hear this,
> start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
> -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
> pipewire,id=audio0,out.mixing-engine=off ... and start playback with the
> hda device.
>
> With best regards,
> Volker
>
> > +} else {
> > +if (avail < (int32_t) n_bytes) {
> > +n_bytes = avail;
> > +}
> > +
> > +spa_ringbuffer_read_data(>ring,
> > +v->buffer, RINGBUFFER_SIZE,
> > +index & RINGBUFFER_MASK, p,
> n_bytes);
> > +
> > +index += n_bytes;
> > +spa_ringbuffer_read_update(>ring, index);
> > +}
> > +
> > +buf->datas[0].chunk->offset = 0;
> > +buf->datas[0].chunk->stride = v->frame_size;
> > +buf->datas[0].chunk->size = n_bytes;
> > +
> > +/* queue the buffer for playback */
> > +pw_stream_queue_buffer(v->stream, b);
> > +}
> > +
> >
>
>
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
+/* output data processing function to read stuffs from the buffer */
+static void
+playback_on_process(void *data)
+{
+PWVoice *v = (PWVoice *) data;
+void *p;
+struct pw_buffer *b;
+struct spa_buffer *buf;
+uint32_t n_frames, req, index, n_bytes;
+int32_t avail;
+
+if (!v->stream) {
+return;
+}
+
+/* obtain a buffer to read from */
+b = pw_stream_dequeue_buffer(v->stream);
+if (b == NULL) {
+error_report("out of buffers: %s", strerror(errno));
+return;
+}
+
+buf = b->buffer;
+p = buf->datas[0].data;
+if (p == NULL) {
+return;
+}
+req = b->requested * v->frame_size;
+if (req == 0) {
+req = 4096 * v->frame_size;
+}
+n_frames = SPA_MIN(req, buf->datas[0].maxsize);
+n_bytes = n_frames * v->frame_size;
+
+/* get no of available bytes to read data from buffer */
+
+avail = spa_ringbuffer_get_read_index(>ring, );
+
+if (!v->enabled) {
+avail = 0;
+}
+
+if (avail == 0) {
+memset(p, 0, n_bytes);
memset() doesn't work for unsigned samples. For unsigned samples, a
stream of zeros is silence with a DC offset. When Pipewire mixes this
stream with another, the result is a clipped audio stream. To hear this,
start QEMU with qemu-system-x86_64 -machine pcspk-audiodev=audio0
-device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev
pipewire,id=audio0,out.mixing-engine=off ... and start playback with the
hda device.
With best regards,
Volker
+} else {
+if (avail < (int32_t) n_bytes) {
+n_bytes = avail;
+}
+
+spa_ringbuffer_read_data(>ring,
+v->buffer, RINGBUFFER_SIZE,
+index & RINGBUFFER_MASK, p, n_bytes);
+
+index += n_bytes;
+spa_ringbuffer_read_update(>ring, index);
+}
+
+buf->datas[0].chunk->offset = 0;
+buf->datas[0].chunk->stride = v->frame_size;
+buf->datas[0].chunk->size = n_bytes;
+
+/* queue the buffer for playback */
+pw_stream_queue_buffer(v->stream, b);
+}
+
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Hi Dorinda, I've started to write down my suggestions and comments. After more than one page of text, I think that without sample code, the text is not very understandable. Therefore I will write three mails. In this mail I describe the problem with the QEMU Pipewire audio backend. My next mail is a patch for the v7 QEMU pipewire backend with my suggestions how I would change the code. I can then reply to my own mail with comments, explaining my changes. I tested the pipewire audio backend with the QEMU hda device. Audio playback doesn't work well on my computer. I hear dropouts every few seconds. +#define BUFFER_SAMPLES512 BUFFER_SAMPLES can't be a fixed size. To hear the problem please start QEMU with qemu-system-x86_64 -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev pipewire,id=audio0,out.frequency=96000,in.frequency=96000 ... . The pipewire backend tells QEMU to use a buffer size of 512 audio frames. The buffer can hold data for 512 frames / 96000 frames/s = 5.3ms. With a timer-period of 10ms there is no way to transport enough frames to the pipewire buffer. Just using a larger BUFFER_SAMPLES value also doesn't work. When I start QEMU with qemu-system-x86_64 -device ich9-intel-hda -device hda-duplex,audiodev=audio0 -audiodev pipewire,id=audio0,timer-period=2000 ... the maximum audio frame throughput increases as when using a larger BUFFER_SAMPLES value. But the higher maximum throughput makes the dropout issue more audible. This has to do with how often pipewire calls playback_on_process() and capture_on_process(). On my system pipewire calls playback_on_process() and capture_on_process() every 21ms and sometimes the callback is delayed by 42ms. At a guest audio frame rate of 44100 frames/s the QEMU hda device drops its buffer if data wasn't read for longer than 23ms on average. With a timer period of 2ms, the PWVoice buffer fills quicker and the time to the next pipewire update is longer. This increases the chance that the hda device drops its buffer. With a timer period of 10ms, the PWVoice buffer fills slower. This is similar to a jitter buffer, but a good jitter buffer implementation would work better. My next mail with the sample code will give pipewire a hint to call the callbacks faster than timer-period and then remove BUFFER_SAMPLES. With best regards, Volker
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Hi Marc-André,
Thank you for your feedback.
Given that we have only one loop, I would set the thread name to NULL,
> (which will use "pw-thread-loop" by default)
>
I think it's preferable to be explicit and clear about the thread name.
It's not clear to me the reason being that it's only one loop.
static void *
> qpw_audio_init(Audiodev *dev)
> {
> AudiodevPipewireOptions *popts;
> g_autofree pwaudio *pw = g_new0(pwaudio, 1);
>
> assert(dev->driver == AUDIODEV_DRIVER_PIPEWIRE);
> trace_pw_audio_init();
> pw_init(NULL, NULL);
>
> popts = >u.pipewire;
> if (!popts->has_latency) {
> popts->has_latency = true;
> popts->latency = 15000;
> }
>
> pw->dev = dev;
> pw->thread_loop = pw_thread_loop_new(NULL, NULL);
> if (!pw->thread_loop) {
> goto fail;
> }
> pw->context =
> pw_context_new(pw_thread_loop_get_loop(pw->thread_loop), NULL, 0);
> if (!pw->context) {
> goto fail;
> }
> if (pw_thread_loop_start(pw->thread_loop) < 0) {
> goto fail;
> }
> pw_thread_loop_lock(pw->thread_loop);
> pw->core = pw_context_connect(pw->context, NULL, 0);
> if (!pw->core) {
> pw_thread_loop_unlock(pw->thread_loop);
> goto fail;
> }
> pw_core_add_listener(pw->core, >core_listener, _events, pw);
> pw_thread_loop_unlock(pw->thread_loop);
>
> return g_steal_pointer();
>
> fail:
> error_report("Failed to initialize Pipewire: %s", strerror(errno));
> if (pw->thread_loop) {
> /* can be called even when the loop is not running */
> pw_thread_loop_stop(pw->thread_loop);
> }
> g_clear_pointer(>context, pw_context_destroy);
> g_clear_pointer(>thread_loop, pw_thread_loop_destroy);
> return NULL;
> }
This looks much better, Thanks. however in the error handling I would do
something like
fail:
AUD_log(AUDIO_CAP, "Failed to initialize PW context");
if (pw->thread_loop) {
pw_thread_loop_stop(pw->thread_loop);
g_clear_pointer(>thread_loop, pw_thread_loop_destroy);
}
if (pw->context) {
g_clear_pointer(>context, pw_context_destroy);
}
return NULL;
}
The core is always NULL when reaching error conditions. Whether this
> function must be called with the loop lock is unclear to me. Anyway,
> it should not be necessary.
>
I agree it's not needed here, this could lead to unexpected behaviour.
That doesn't seem necessary. The doc doesn't say much, but none of the
> examples or pw_stream_new_simple() implementation remove the hook on
> destroy. I think you can remove that callback.
>
In this case removing the hook ensures that the listener's callback
functions are not called after the listener has been destroyed, this would
help avoid potential issues. I would leave it there.
Can this happen? If you get an event callback for a stream, it should be
> alive.
>
I think in general it's good to have this check before attempting to
manipulate the buffer so as to avoid pointer errors. Just in case it
happens.
What is this for? worth a comment at least.
>
This represents a common buffer size for audio processing, and to ensure
that the buffer is large enough to hold a significant amount of audio data.
I had explained earlier in V2 of the patch that "For playback streams, this
size allows for more efficient streaming of audio data." Indeed, I should
add a comment. Thanks.
req is in bytes already, maxsize as well.
>
Could you please clarify a bit what you mean here?
So here, you multiply by frame_size^2, looking wrong to me.
>
basically to ensure that the buffer is large enough to hold the maximum
amount of data processed in a single transfer operation.
The ringbuffer maybe full, in which case playback may suffer a large
> delay. I think we should maintain a small fixed-delay ringbuffer, or
> find a different solution. See below
>
I don't completely understand what you mean here. IMO a larger buffer may
be more appropriate in case we need to process large amounts of data
quickly and have sufficient memory available, CMIIW.
I suggest removing that callback, it doesn't seem useful either.
>
Please clarify, which callback?
Ok, here I think you tried to mimic jackaudio.c, by using a default of
> 3 * 512 frames ring/fifo (seems quite arbitrary but if it works...)
>
yes.
However, you don't actually maintain the ringbuffer size, it will keep
> growing, just with smaller writes,? I think you could use this value
> as the ringbuffer size. The potential additional delay would then be
> 3*512/samplerate (ex 32ms at 48khz).
>
Could you explain a bit more why this suggestion?
Have you considered pw_stream_set_active() ?
>
FWIU this is used to pause or activate a stream. I think that v->enabled =
enable; is sufficient but I'm open to your suggestion, I just need to know
why and how do you suggest i use it?
This option is not taken into account. It seems you should set the
> stream PW_KEY_NODE_LATENCY, which must be a
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Am 08.03.23 um 11:39 schrieb Marc-André Lureau: Volker, Wim, it would be nice if you could review/comment too! thanks Hi, last weekend I replaced pulseaudio with pipewire on my host computer and tested the QEMU pipewire backend. It doesn't work well on my computer, but with a few changes it becomes usable. I hope to have some time tomorrow evening to write down my suggestions and comments. With best regards, Volker -- Marc-André Lureau
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Hi
On Mon, Mar 6, 2023 at 9:11 PM Dorinda Bassey wrote:
>
> This commit adds a new audiodev backend to allow QEMU to use Pipewire as
> both an audio sink and source. This backend is available on most systems
>
> Add Pipewire entry points for QEMU Pipewire audio backend
> Add wrappers for QEMU Pipewire audio backend in qpw_pcm_ops()
> qpw_write function returns the current state of the stream to pwaudio
> and Writes some data to the server for playback streams using pipewire
> spa_ringbuffer implementation.
> qpw_read function returns the current state of the stream to pwaudio and
> reads some data from the server for capture streams using pipewire
> spa_ringbuffer implementation. These functions qpw_write and qpw_read
> are called during playback and capture.
> Added some functions that convert pw audio formats to QEMU audio format
> and vice versa which would be needed in the pipewire audio sink and
> source functions qpw_init_in() & qpw_init_out().
> These methods that implement playback and recording will create streams
> for playback and capture that will start processing and will result in
> the on_process callbacks to be called.
> Built a connection to the Pipewire sound system server in the
> qpw_audio_init() method.
>
> Signed-off-by: Dorinda Bassey
> ---
> v7:
> use qemu tracing tool
>
> audio/audio.c | 3 +
> audio/audio_template.h| 4 +
> audio/meson.build | 1 +
> audio/pwaudio.c | 814 ++
> audio/trace-events| 7 +
> meson.build | 8 +
> meson_options.txt | 4 +-
> qapi/audio.json | 45 ++
> qemu-options.hx | 17 +
> scripts/meson-buildoptions.sh | 8 +-
> 10 files changed, 908 insertions(+), 3 deletions(-)
> create mode 100644 audio/pwaudio.c
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 4290309d18..aa55e41ad8 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2069,6 +2069,9 @@ void audio_create_pdos(Audiodev *dev)
> #ifdef CONFIG_AUDIO_PA
> CASE(PA, pa, Pa);
> #endif
> +#ifdef CONFIG_AUDIO_PIPEWIRE
> +CASE(PIPEWIRE, pipewire, Pipewire);
> +#endif
> #ifdef CONFIG_AUDIO_SDL
> CASE(SDL, sdl, Sdl);
> #endif
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 42b4712acb..0f02afb921 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -355,6 +355,10 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> TYPE)(Audiodev *dev)
> case AUDIODEV_DRIVER_PA:
> return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
> #endif
> +#ifdef CONFIG_AUDIO_PIPEWIRE
> +case AUDIODEV_DRIVER_PIPEWIRE:
> +return
> qapi_AudiodevPipewirePerDirectionOptions_base(dev->u.pipewire.TYPE);
> +#endif
> #ifdef CONFIG_AUDIO_SDL
> case AUDIODEV_DRIVER_SDL:
> return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> diff --git a/audio/meson.build b/audio/meson.build
> index 074ba9..65a49c1a10 100644
> --- a/audio/meson.build
> +++ b/audio/meson.build
> @@ -19,6 +19,7 @@ foreach m : [
>['sdl', sdl, files('sdlaudio.c')],
>['jack', jack, files('jackaudio.c')],
>['sndio', sndio, files('sndioaudio.c')],
> + ['pipewire', pipewire, files('pwaudio.c')],
>['spice', spice, files('spiceaudio.c')]
> ]
>if m[1].found()
> diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> new file mode 100644
> index 00..d357761152
> --- /dev/null
> +++ b/audio/pwaudio.c
> @@ -0,0 +1,814 @@
> +/*
> + * QEMU Pipewire audio driver
> + *
> + * Copyright (c) 2023 Red Hat Inc.
> + *
> + * Author: Dorinda Bassey
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "audio.h"
> +#include
> +#include "qemu/error-report.h"
> +#include
> +#include
> +#include
> +
> +#include
> +#include "trace.h"
> +
> +#define AUDIO_CAP "pipewire"
> +#define RINGBUFFER_SIZE(1u << 22)
> +#define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
> +#define BUFFER_SAMPLES512
> +
> +#include "audio_int.h"
> +
> +enum {
> +MODE_SINK,
> +MODE_SOURCE
> +};
> +
> +typedef struct pwaudio {
> +Audiodev *dev;
> +struct pw_thread_loop *thread_loop;
> +struct pw_context *context;
> +
> +struct pw_core *core;
> +struct spa_hook core_listener;
> +int seq;
> +} pwaudio;
> +
> +typedef struct PWVoice {
> +pwaudio *g;
> +bool enabled;
> +struct pw_stream *stream;
> +struct spa_hook stream_listener;
> +struct spa_audio_info_raw info;
> +uint32_t frame_size;
> +struct spa_ringbuffer ring;
> +uint8_t buffer[RINGBUFFER_SIZE];
> +
> +uint32_t mode;
> +struct pw_properties *props;
> +} PWVoice;
> +
> +typedef struct PWVoiceOut {
> +HWVoiceOut hw;
> +PWVoice v;
> +} PWVoiceOut;
> +
> +typedef struct PWVoiceIn {
> +HWVoiceIn hw;
> +PWVoice v;
> +} PWVoiceIn;
> +
> +static void
>
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Hi Marc-André,
Do you mind summarizing your review on the buffering section, and leave
comments so i can address everything once and for all? This will help
reduce the number of versions that needs to be done.
Thanks,
Dorinda.
On Tue, Mar 7, 2023 at 3:41 PM Marc-André Lureau
wrote:
> Hi Dorinda
>
> On Mon, Mar 6, 2023 at 9:11 PM Dorinda Bassey wrote:
> >
> > This commit adds a new audiodev backend to allow QEMU to use Pipewire as
> > both an audio sink and source. This backend is available on most systems
> >
> > Add Pipewire entry points for QEMU Pipewire audio backend
> > Add wrappers for QEMU Pipewire audio backend in qpw_pcm_ops()
> > qpw_write function returns the current state of the stream to pwaudio
> > and Writes some data to the server for playback streams using pipewire
> > spa_ringbuffer implementation.
> > qpw_read function returns the current state of the stream to pwaudio and
> > reads some data from the server for capture streams using pipewire
> > spa_ringbuffer implementation. These functions qpw_write and qpw_read
> > are called during playback and capture.
> > Added some functions that convert pw audio formats to QEMU audio format
> > and vice versa which would be needed in the pipewire audio sink and
> > source functions qpw_init_in() & qpw_init_out().
> > These methods that implement playback and recording will create streams
> > for playback and capture that will start processing and will result in
> > the on_process callbacks to be called.
> > Built a connection to the Pipewire sound system server in the
> > qpw_audio_init() method.
> >
> > Signed-off-by: Dorinda Bassey
>
> Some more comments before I start looking more closely at buffering.
> thanks
>
> > ---
> > v7:
> > use qemu tracing tool
> >
> > audio/audio.c | 3 +
> > audio/audio_template.h| 4 +
> > audio/meson.build | 1 +
> > audio/pwaudio.c | 814 ++
> > audio/trace-events| 7 +
> > meson.build | 8 +
> > meson_options.txt | 4 +-
> > qapi/audio.json | 45 ++
> > qemu-options.hx | 17 +
> > scripts/meson-buildoptions.sh | 8 +-
> > 10 files changed, 908 insertions(+), 3 deletions(-)
> > create mode 100644 audio/pwaudio.c
> >
> > diff --git a/audio/audio.c b/audio/audio.c
> > index 4290309d18..aa55e41ad8 100644
> > --- a/audio/audio.c
> > +++ b/audio/audio.c
> > @@ -2069,6 +2069,9 @@ void audio_create_pdos(Audiodev *dev)
> > #ifdef CONFIG_AUDIO_PA
> > CASE(PA, pa, Pa);
> > #endif
> > +#ifdef CONFIG_AUDIO_PIPEWIRE
> > +CASE(PIPEWIRE, pipewire, Pipewire);
> > +#endif
> > #ifdef CONFIG_AUDIO_SDL
> > CASE(SDL, sdl, Sdl);
> > #endif
> > diff --git a/audio/audio_template.h b/audio/audio_template.h
> > index 42b4712acb..0f02afb921 100644
> > --- a/audio/audio_template.h
> > +++ b/audio/audio_template.h
> > @@ -355,6 +355,10 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> TYPE)(Audiodev *dev)
> > case AUDIODEV_DRIVER_PA:
> > return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
> > #endif
> > +#ifdef CONFIG_AUDIO_PIPEWIRE
> > +case AUDIODEV_DRIVER_PIPEWIRE:
> > +return
> qapi_AudiodevPipewirePerDirectionOptions_base(dev->u.pipewire.TYPE);
> > +#endif
> > #ifdef CONFIG_AUDIO_SDL
> > case AUDIODEV_DRIVER_SDL:
> > return
> qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> > diff --git a/audio/meson.build b/audio/meson.build
> > index 074ba9..65a49c1a10 100644
> > --- a/audio/meson.build
> > +++ b/audio/meson.build
> > @@ -19,6 +19,7 @@ foreach m : [
> >['sdl', sdl, files('sdlaudio.c')],
> >['jack', jack, files('jackaudio.c')],
> >['sndio', sndio, files('sndioaudio.c')],
> > + ['pipewire', pipewire, files('pwaudio.c')],
> >['spice', spice, files('spiceaudio.c')]
> > ]
> >if m[1].found()
> > diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> > new file mode 100644
> > index 00..d357761152
> > --- /dev/null
> > +++ b/audio/pwaudio.c
> > @@ -0,0 +1,814 @@
> > +/*
> > + * QEMU Pipewire audio driver
> > + *
> > + * Copyright (c) 2023 Red Hat Inc.
> > + *
> > + * Author: Dorinda Bassey
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/module.h"
> > +#include "audio.h"
> > +#include
> > +#include "qemu/error-report.h"
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include "trace.h"
> > +
> > +#define AUDIO_CAP "pipewire"
> > +#define RINGBUFFER_SIZE(1u << 22)
> > +#define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
> > +#define BUFFER_SAMPLES512
> > +
> > +#include "audio_int.h"
> > +
> > +enum {
> > +MODE_SINK,
> > +MODE_SOURCE
> > +};
> > +
> > +typedef struct pwaudio {
> > +Audiodev *dev;
> > +struct pw_thread_loop *thread_loop;
> > +struct pw_context *context;
> > +
> > +struct pw_core *core;
> > +
Re: [PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
Hi Dorinda
On Mon, Mar 6, 2023 at 9:11 PM Dorinda Bassey wrote:
>
> This commit adds a new audiodev backend to allow QEMU to use Pipewire as
> both an audio sink and source. This backend is available on most systems
>
> Add Pipewire entry points for QEMU Pipewire audio backend
> Add wrappers for QEMU Pipewire audio backend in qpw_pcm_ops()
> qpw_write function returns the current state of the stream to pwaudio
> and Writes some data to the server for playback streams using pipewire
> spa_ringbuffer implementation.
> qpw_read function returns the current state of the stream to pwaudio and
> reads some data from the server for capture streams using pipewire
> spa_ringbuffer implementation. These functions qpw_write and qpw_read
> are called during playback and capture.
> Added some functions that convert pw audio formats to QEMU audio format
> and vice versa which would be needed in the pipewire audio sink and
> source functions qpw_init_in() & qpw_init_out().
> These methods that implement playback and recording will create streams
> for playback and capture that will start processing and will result in
> the on_process callbacks to be called.
> Built a connection to the Pipewire sound system server in the
> qpw_audio_init() method.
>
> Signed-off-by: Dorinda Bassey
Some more comments before I start looking more closely at buffering.
thanks
> ---
> v7:
> use qemu tracing tool
>
> audio/audio.c | 3 +
> audio/audio_template.h| 4 +
> audio/meson.build | 1 +
> audio/pwaudio.c | 814 ++
> audio/trace-events| 7 +
> meson.build | 8 +
> meson_options.txt | 4 +-
> qapi/audio.json | 45 ++
> qemu-options.hx | 17 +
> scripts/meson-buildoptions.sh | 8 +-
> 10 files changed, 908 insertions(+), 3 deletions(-)
> create mode 100644 audio/pwaudio.c
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 4290309d18..aa55e41ad8 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2069,6 +2069,9 @@ void audio_create_pdos(Audiodev *dev)
> #ifdef CONFIG_AUDIO_PA
> CASE(PA, pa, Pa);
> #endif
> +#ifdef CONFIG_AUDIO_PIPEWIRE
> +CASE(PIPEWIRE, pipewire, Pipewire);
> +#endif
> #ifdef CONFIG_AUDIO_SDL
> CASE(SDL, sdl, Sdl);
> #endif
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index 42b4712acb..0f02afb921 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -355,6 +355,10 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
> TYPE)(Audiodev *dev)
> case AUDIODEV_DRIVER_PA:
> return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
> #endif
> +#ifdef CONFIG_AUDIO_PIPEWIRE
> +case AUDIODEV_DRIVER_PIPEWIRE:
> +return
> qapi_AudiodevPipewirePerDirectionOptions_base(dev->u.pipewire.TYPE);
> +#endif
> #ifdef CONFIG_AUDIO_SDL
> case AUDIODEV_DRIVER_SDL:
> return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> diff --git a/audio/meson.build b/audio/meson.build
> index 074ba9..65a49c1a10 100644
> --- a/audio/meson.build
> +++ b/audio/meson.build
> @@ -19,6 +19,7 @@ foreach m : [
>['sdl', sdl, files('sdlaudio.c')],
>['jack', jack, files('jackaudio.c')],
>['sndio', sndio, files('sndioaudio.c')],
> + ['pipewire', pipewire, files('pwaudio.c')],
>['spice', spice, files('spiceaudio.c')]
> ]
>if m[1].found()
> diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> new file mode 100644
> index 00..d357761152
> --- /dev/null
> +++ b/audio/pwaudio.c
> @@ -0,0 +1,814 @@
> +/*
> + * QEMU Pipewire audio driver
> + *
> + * Copyright (c) 2023 Red Hat Inc.
> + *
> + * Author: Dorinda Bassey
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "audio.h"
> +#include
> +#include "qemu/error-report.h"
> +#include
> +#include
> +#include
> +
> +#include
> +#include "trace.h"
> +
> +#define AUDIO_CAP "pipewire"
> +#define RINGBUFFER_SIZE(1u << 22)
> +#define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
> +#define BUFFER_SAMPLES512
> +
> +#include "audio_int.h"
> +
> +enum {
> +MODE_SINK,
> +MODE_SOURCE
> +};
> +
> +typedef struct pwaudio {
> +Audiodev *dev;
> +struct pw_thread_loop *thread_loop;
> +struct pw_context *context;
> +
> +struct pw_core *core;
> +struct spa_hook core_listener;
> +int seq;
> +} pwaudio;
> +
> +typedef struct PWVoice {
> +pwaudio *g;
> +bool enabled;
> +struct pw_stream *stream;
> +struct spa_hook stream_listener;
> +struct spa_audio_info_raw info;
> +uint32_t frame_size;
> +struct spa_ringbuffer ring;
> +uint8_t buffer[RINGBUFFER_SIZE];
> +
> +uint32_t mode;
> +struct pw_properties *props;
> +} PWVoice;
> +
> +typedef struct PWVoiceOut {
> +HWVoiceOut hw;
> +PWVoice v;
> +} PWVoiceOut;
> +
> +typedef struct PWVoiceIn {
> +
[PATCH v7] audio/pwaudio.c: Add Pipewire audio backend for QEMU
This commit adds a new audiodev backend to allow QEMU to use Pipewire as
both an audio sink and source. This backend is available on most systems
Add Pipewire entry points for QEMU Pipewire audio backend
Add wrappers for QEMU Pipewire audio backend in qpw_pcm_ops()
qpw_write function returns the current state of the stream to pwaudio
and Writes some data to the server for playback streams using pipewire
spa_ringbuffer implementation.
qpw_read function returns the current state of the stream to pwaudio and
reads some data from the server for capture streams using pipewire
spa_ringbuffer implementation. These functions qpw_write and qpw_read
are called during playback and capture.
Added some functions that convert pw audio formats to QEMU audio format
and vice versa which would be needed in the pipewire audio sink and
source functions qpw_init_in() & qpw_init_out().
These methods that implement playback and recording will create streams
for playback and capture that will start processing and will result in
the on_process callbacks to be called.
Built a connection to the Pipewire sound system server in the
qpw_audio_init() method.
Signed-off-by: Dorinda Bassey
---
v7:
use qemu tracing tool
audio/audio.c | 3 +
audio/audio_template.h| 4 +
audio/meson.build | 1 +
audio/pwaudio.c | 814 ++
audio/trace-events| 7 +
meson.build | 8 +
meson_options.txt | 4 +-
qapi/audio.json | 45 ++
qemu-options.hx | 17 +
scripts/meson-buildoptions.sh | 8 +-
10 files changed, 908 insertions(+), 3 deletions(-)
create mode 100644 audio/pwaudio.c
diff --git a/audio/audio.c b/audio/audio.c
index 4290309d18..aa55e41ad8 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2069,6 +2069,9 @@ void audio_create_pdos(Audiodev *dev)
#ifdef CONFIG_AUDIO_PA
CASE(PA, pa, Pa);
#endif
+#ifdef CONFIG_AUDIO_PIPEWIRE
+CASE(PIPEWIRE, pipewire, Pipewire);
+#endif
#ifdef CONFIG_AUDIO_SDL
CASE(SDL, sdl, Sdl);
#endif
diff --git a/audio/audio_template.h b/audio/audio_template.h
index 42b4712acb..0f02afb921 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -355,6 +355,10 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_,
TYPE)(Audiodev *dev)
case AUDIODEV_DRIVER_PA:
return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
#endif
+#ifdef CONFIG_AUDIO_PIPEWIRE
+case AUDIODEV_DRIVER_PIPEWIRE:
+return
qapi_AudiodevPipewirePerDirectionOptions_base(dev->u.pipewire.TYPE);
+#endif
#ifdef CONFIG_AUDIO_SDL
case AUDIODEV_DRIVER_SDL:
return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
diff --git a/audio/meson.build b/audio/meson.build
index 074ba9..65a49c1a10 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -19,6 +19,7 @@ foreach m : [
['sdl', sdl, files('sdlaudio.c')],
['jack', jack, files('jackaudio.c')],
['sndio', sndio, files('sndioaudio.c')],
+ ['pipewire', pipewire, files('pwaudio.c')],
['spice', spice, files('spiceaudio.c')]
]
if m[1].found()
diff --git a/audio/pwaudio.c b/audio/pwaudio.c
new file mode 100644
index 00..d357761152
--- /dev/null
+++ b/audio/pwaudio.c
@@ -0,0 +1,814 @@
+/*
+ * QEMU Pipewire audio driver
+ *
+ * Copyright (c) 2023 Red Hat Inc.
+ *
+ * Author: Dorinda Bassey
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "audio.h"
+#include
+#include "qemu/error-report.h"
+#include
+#include
+#include
+
+#include
+#include "trace.h"
+
+#define AUDIO_CAP "pipewire"
+#define RINGBUFFER_SIZE(1u << 22)
+#define RINGBUFFER_MASK(RINGBUFFER_SIZE - 1)
+#define BUFFER_SAMPLES512
+
+#include "audio_int.h"
+
+enum {
+MODE_SINK,
+MODE_SOURCE
+};
+
+typedef struct pwaudio {
+Audiodev *dev;
+struct pw_thread_loop *thread_loop;
+struct pw_context *context;
+
+struct pw_core *core;
+struct spa_hook core_listener;
+int seq;
+} pwaudio;
+
+typedef struct PWVoice {
+pwaudio *g;
+bool enabled;
+struct pw_stream *stream;
+struct spa_hook stream_listener;
+struct spa_audio_info_raw info;
+uint32_t frame_size;
+struct spa_ringbuffer ring;
+uint8_t buffer[RINGBUFFER_SIZE];
+
+uint32_t mode;
+struct pw_properties *props;
+} PWVoice;
+
+typedef struct PWVoiceOut {
+HWVoiceOut hw;
+PWVoice v;
+} PWVoiceOut;
+
+typedef struct PWVoiceIn {
+HWVoiceIn hw;
+PWVoice v;
+} PWVoiceIn;
+
+static void
+stream_destroy(void *data)
+{
+PWVoice *v = (PWVoice *) data;
+spa_hook_remove(>stream_listener);
+v->stream = NULL;
+}
+
+/* output data processing function to read stuffs from the buffer */
+static void
+playback_on_process(void *data)
+{
+PWVoice *v = (PWVoice *) data;
+void *p;
+struct pw_buffer *b;
+struct spa_buffer *buf;
+uint32_t n_frames,
