On Wed, Feb 15, 2023 at 09:51:02AM +0100, 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.
>
> Added Pipewire entry points for QEMU Pipewire audio backend
> Added 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.
Can you ensure the commit message text is line wrapped at approx
72 characters.
> diff --git a/audio/pwaudio.c b/audio/pwaudio.c
> new file mode 100644
> index 0000000000..89040ac99e
> --- /dev/null
> +++ b/audio/pwaudio.c
> @@ -0,0 +1,816 @@
> +/*
> + * QEMU Pipewire audio driver
> + *
> + * Copyright (c) 2023, Red Hat Inc, Dorinda Bassey <dbas...@redhat.com>
Just to confirm, you are claiming both copyright ownership for Red Hat
*and* personal copyright ownership ?
I ask because most of the time the employer will have exclusive copyright
ownership for anything created in the course of employment. A few countries
have local law, however, that fineses this allowing employees to retain
copyright, or if you developed stuff outside of work context.
> +
> +#include "qemu/osdep.h"
> +#include "qemu/module.h"
> +#include "audio.h"
> +#include <errno.h>
> +#include <spa/param/audio/format-utils.h>
> +#include <spa/utils/ringbuffer.h>
> +#include <spa/utils/result.h>
> +
> +#include <pipewire/pipewire.h>
> +
> +#define AUDIO_CAP "pipewire"
> +#define RINGBUFFER_SIZE (1u << 22)
> +#define RINGBUFFER_MASK (RINGBUFFER_SIZE - 1)
> +#define BUFFER_SAMPLES 128
> +
> +#include "audio_int.h"
> +
> +enum {
> + MODE_SINK,
> + MODE_SOURCE
> +};
As a style point, QEMU standard is for 4-space indents in
C code.
> diff --git a/meson_options.txt b/meson_options.txt
> index e5f199119e..f42605a8ac 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -21,7 +21,7 @@ option('tls_priority', type : 'string', value : 'NORMAL',
> option('default_devices', type : 'boolean', value : true,
> description: 'Include a default selection of devices in emulators')
> option('audio_drv_list', type: 'array', value: ['default'],
> - choices: ['alsa', 'coreaudio', 'default', 'dsound', 'jack', 'oss',
> 'pa', 'sdl', 'sndio'],
> + choices: ['alsa', 'coreaudio', 'default', 'dsound', 'jack', 'oss',
> 'pa', 'pw', 'sdl', 'sndio'],
I appreciate you probably just followed the example of pulseaudio, abbreviated
to 'pa', but I'm not a fan of the existing usage there. So lets be more verbose
and say 'pipewire' so users are clear on what this is.
> description: 'Set audio driver list')
> option('block_drv_rw_whitelist', type : 'string', value : '',
> description: 'set block driver read-write whitelist (by default
> affects only QEMU, not tools like qemu-img)')
> @@ -253,6 +253,8 @@ option('oss', type: 'feature', value: 'auto',
> description: 'OSS sound support')
> option('pa', type: 'feature', value: 'auto',
> description: 'PulseAudio sound support')
> +option('pw', type: 'feature', value: 'auto',
> + description: 'Pipewire sound support')
> option('sndio', type: 'feature', value: 'auto',
> description: 'sndio sound support')
>
> diff --git a/qapi/audio.json b/qapi/audio.json
> index 4e54c00f51..6c17d08ab8 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -324,6 +324,48 @@
> '*out': 'AudiodevPaPerDirectionOptions',
> '*server': 'str' } }
>
> +##
> +# @AudiodevPwPerDirectionOptions:
> +#
> +# Options of the Pipewire backend that are used for both playback and
> +# recording.
> +#
> +# @name: name of the sink/source to use
> +#
> +# @stream-name: name of the Pipewire stream created by qemu. Can be
> +# used to identify the stream in Pipewire when you
> +# create multiple Pipewire devices or run multiple qemu
> +# instances (default: audiodev's id, since 7.1)
> +#
> +#
> +# Since: 7.2
> +##
> +{ 'struct': 'AudiodevPwPerDirectionOptions',
> + 'base': 'AudiodevPerDirectionOptions',
> + 'data': {
> + '*name': 'str',
> + '*stream-name': 'str' } }
I tend to think we could afford to say "Pipewire" instead
of "Pw" in the data types too.
> +
> +##
> +# @AudiodevPwOptions:
> +#
> +# Options of the Pipewire audio backend.
> +#
> +# @in: options of the capture stream
> +#
> +# @out: options of the playback stream
> +#
> +# @latency: add latency to playback in microseconds
> +# (default 44100)
> +#
> +# Since: 7.2
> +##
> +{ 'struct': 'AudiodevPwOptions',
> + 'data': {
> + '*in': 'AudiodevPwPerDirectionOptions',
> + '*out': 'AudiodevPwPerDirectionOptions',
> + '*latency': 'uint32' } }
> +
> ##
> # @AudiodevSdlPerDirectionOptions:
> #
> @@ -416,6 +458,7 @@
> { 'name': 'jack', 'if': 'CONFIG_AUDIO_JACK' },
> { 'name': 'oss', 'if': 'CONFIG_AUDIO_OSS' },
> { 'name': 'pa', 'if': 'CONFIG_AUDIO_PA' },
> + { 'name': 'pw', 'if': 'CONFIG_AUDIO_PW' },
> { 'name': 'sdl', 'if': 'CONFIG_AUDIO_SDL' },
> { 'name': 'sndio', 'if': 'CONFIG_AUDIO_SNDIO' },
> { 'name': 'spice', 'if': 'CONFIG_SPICE' },
> @@ -456,6 +499,8 @@
> 'if': 'CONFIG_AUDIO_OSS' },
> 'pa': { 'type': 'AudiodevPaOptions',
> 'if': 'CONFIG_AUDIO_PA' },
> + 'pw': { 'type': 'AudiodevPwOptions',
> + 'if': 'CONFIG_AUDIO_PW' },
> 'sdl': { 'type': 'AudiodevSdlOptions',
> 'if': 'CONFIG_AUDIO_SDL' },
> 'sndio': { 'type': 'AudiodevSndioOptions',
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 88e93c6103..4fc73af804 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -779,6 +779,11 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
> " in|out.name= source/sink device name\n"
> " in|out.latency= desired latency in microseconds\n"
> #endif
> +#ifdef CONFIG_AUDIO_PW
> + "-audiodev pw,id=id[,prop[=value][,...]]\n"
> + " in|out.name= source/sink device name\n"
> + " latency= desired latency in microseconds\n"
> +#endif
Again, lets call the driver 'pipewire' rather than just 'pw'
> #ifdef CONFIG_AUDIO_SDL
> "-audiodev sdl,id=id[,prop[=value][,...]]\n"
> " in|out.buffer-count= number of buffers\n"
> @@ -942,6 +947,18 @@ SRST
> Desired latency in microseconds. The PulseAudio server will try
> to honor this value but actual latencies may be lower or higher.
>
> +``-audiodev pw,id=id[,prop[=value][,...]]``
> + Creates a backend using Pipewire. This backend is available on
> + most systems.
> +
> + Pipewire specific options are:
> +
> + ``latency=latency``
> + Add extra latency to playback in microseconds
> +
> + ``in|out.name=sink``
> + Use the specified source/sink for recording/playback.
> +
> ``-audiodev sdl,id=id[,prop[=value][,...]]``
> Creates a backend using SDL. This backend is available on most
> systems, but you should use your platform's native backend if
I'll leave actual review of the backend functionality to someone
else who is familiar with the audio subsystem.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
