Re: [PATCH] audio/pwaudio.c: Add Pipewire audio backend for QEMU

["Daniel P. Berrangé" <berrange@redhat.com>]

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 <dbassey@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 :|