>
> Can you ensure the commit message text is line wrapped at approx
> 72 characters.
>
Noted, Thanks.
Just to confirm, you are claiming both copyright ownership for Red Hat
> *and* personal copyright ownership ?
>
I just made some enquiry and given that this was developed inside of work
context I would use copyright ownership for Red Hat and author: Dorinda
Bassey
As a style point, QEMU standard is for 4-space indents in
> C code.
>
noted
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.
>
I agree, although pipewire seems to be lengthy but I don't mind using it.
Regards,
Dorinda.
On Wed, Feb 15, 2023 at 12:26 PM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> 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 :|
>
>
