Re: [PATCH] audio: Add sndio backend

[Eric Blake]

On 3/4/20 8:50 AM, Brad Smith wrote:
> Add a sndio backend.
>
> sndio is the native API used by OpenBSD, although it has been ported to
> other *BSD's and Linux (packages for Ubuntu, Debian, Void, Arch, etc.).
>
> The C code is from Alexandre Ratchov <address@hidden> and the rest of
> the bits are from me.
>
>
> Signed-off-by: Alexandre Ratchov <address@hidden>
> Signed-off-by: Brad Smith <address@hidden>
>
> diff --git a/audio/Makefile.objs b/audio/Makefile.objs

Your patch lacks the usual --- separator and diffstat that 'git 
format-patch' (and therefore 'git send-email') uses.  This makes it 
harder to see at a glance the approximate impact of your patch, and 
whether it touches a file I'm interested in.

> +++ b/qapi/audio.json
> @@ -248,6 +248,28 @@
>       '*out':    'AudiodevPaPerDirectionOptions',
>       '*server': 'str' } }
>   
> +##
> +# @AudiodevSndioOptions:
> +#
> +# Options of the sndio audio backend.
> +#
> +# @in: options of the capture stream
> +#
> +# @out: options of the playback stream
> +#
> +# @dev: the name of the sndio device to use (default 'default')
> +#
> +# @latency: play buffer size (in microseconds)
> +#
> +# Since: 4.0

You've missed 4.0 by a long shot; the next release is 5.0.

> +##
> +{ 'struct': 'AudiodevSndioOptions',
> +  'data': {
> +    '*in':        'AudiodevPerDirectionOptions',
> +    '*out':       'AudiodevPerDirectionOptions',
> +    '*dev':       'str',
> +    '*latency':   'uint32'} }
> +
>   ##
>   # @AudiodevWavOptions:
>   #
> @@ -287,7 +309,7 @@
>   ##
>   { 'enum': 'AudiodevDriver',
>     'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'oss', 'pa', 'sdl',
> -            'spice', 'wav' ] }
> +            'sndio', 'spice', 'wav' ] }

It's also customary to document enum options, something like:

# @sndio (since 5.0)

Furthermore, since:


> +++ b/qemu-options.hx
> @@ -566,6 +566,9 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
>   #ifdef CONFIG_AUDIO_SDL
>       "-audiodev sdl,id=id[,prop[=value][,...]]\n"
>   #endif
> +#ifdef CONFIG_AUDIO_SNDIO
> +    "-audiodev sndio,id=id[,prop[=value][,...]]\n"
> +#endif

the option is only useful based on configure-time decisions, it seems 
like the new enum element should be:

{ 'name': 'sndio', 'if': 'defined(CONFIG_AUDIO_SNDIO)' }

to make it introspectible whether support is compiled in to a given 
binary.  True, there are existing enum members that should do likewise, 
so maybe it's worth a cleanup patch first.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org