Re: [Qemu-devel] [PATCH v2 1/6] qapi: qapi for audio backends

[Markus Armbruster]

Copying Eric for additional QAPI schema expertise.

My questions inline, pretty sure they show my ignorance.

"Kővágó, Zoltán" <address@hidden> writes:

> This patch adds structures into qapi to replace the existing configuration
> structures used by audio backends currently. This qapi will be the base of the
> -audiodev command line parameter (that replaces the old environment variables
> based config).
>
> This is not a 1:1 translation of the old options, I've tried to make them much
> more consistent (e.g. almost every backend had an option to specify buffer 
> size,
> but the name was different for every backend, and some backends required 
> usecs,
> while some other required frames, samples or bytes). Also tried to reduce the
> number of abbreviations used by the config keys.
>
> Some of the more important changes:
> * use `in` and `out` instead of `ADC` and `DAC`, as the former is more user
>   friendly imho
> * moved buffer settings into the global setting area (so it's the same for all
>   backends that support it. Backends that can't change buffer size will simply
>   ignore them). Also using usecs, as it's probably more user friendly than
>   samples or bytes.
> * try-poll is now an alsa and oss backend specific option (as all other 
> backends
>   currently ignore it)
>
> Signed-off-by: Kővágó, Zoltán <address@hidden>
>
> ---
>
> Changes from v1 patch:
> * every time-related field now take usecs (and removed -usecs, -millis 
> suffixes)
> * fixed inconsisten optional marking, language issues
>
> Changes from v2 RFC patch:
> * in, out are no longer optional
> * try-poll: moved to alsa and oss (as no other backend used them)
> * voices: added (env variables had this option)
> * dsound: removed primary buffer related fields
>
> Changes from v1 RFC patch:
> * fixed style issues
> * moved definitions into a separate file
> * documented undocumented options (hopefully)
> * removed plive option. It was useless even years ago so it can probably 
> safely
>   go away: 
> https://lists.nongnu.org/archive/html/qemu-devel/2012-03/msg02427.html
> * removed verbose, debug options. Backends should use trace events instead.
> * removed *_retries options from dsound. It's a kludge.
> * moved buffer_usecs and buffer_count to the global config options. Some 
> driver
>   might ignore it (as they do not expose API to change them).
> * wav backend: removed frequecy, format, channels as 
> AudiodevPerDirectionOptions
>   already have them.
>
> Makefile         |   4 +-
>  qapi-schema.json |   3 +
>  qapi/audio.json  | 223 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+), 2 deletions(-)
>  create mode 100644 qapi/audio.json
>
> diff --git a/Makefile b/Makefile
> index 3f97904..ac566fa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -257,8 +257,8 @@ $(SRC_PATH)/qga/qapi-schema.json 
> $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>               "  GEN   $@")
>  
>  qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
> -               $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
> -               $(SRC_PATH)/qapi/event.json
> +               $(SRC_PATH)/qapi/audio.json  $(SRC_PATH)/qapi/block.json \
> +               $(SRC_PATH)/qapi/block-core.json $(SRC_PATH)/qapi/event.json
>  
>  qapi-types.c qapi-types.h :\
>  $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 106008c..e751ea3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5,6 +5,9 @@
>  # QAPI common definitions
>  { 'include': 'qapi/common.json' }
>  
> +# QAPI audio definitions
> +{ 'include': 'qapi/audio.json' }
> +
>  # QAPI block definitions
>  { 'include': 'qapi/block.json' }
>  
> diff --git a/qapi/audio.json b/qapi/audio.json
> new file mode 100644
> index 0000000..2851689
> --- /dev/null
> +++ b/qapi/audio.json
> @@ -0,0 +1,223 @@
> +# -*- mode: python -*-
> +#
> +# Copyright (C) 2015 Zoltán Kővágó <address@hidden>
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
> +# See the COPYING file in the top-level directory.
> +
> +##
> +# @AudiodevNoOptions
> +#
> +# The none, coreaudio, sdl and spice audio backend have no options.
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevNoOptions',
> +  'data': { } }
> +
> +##
> +# @AudiodevAlsaPerDirectionOptions
> +#
> +# Options of the alsa backend that are used for both playback and recording.
> +#
> +# @dev: #optional the name of the alsa device to use

Who picks the default, QEMU or ALSA?

> +#
> +# @try-poll: #optional attempt to use poll mode

What happens when the attempt fails?

> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevAlsaPerDirectionOptions',
> +  'data': {
> +    '*dev':      'str',
> +    '*try-poll': 'bool' } }
> +
> +##
> +# @AudiodevAlsaOptions
> +#
> +# Options of the alsa audio backend.
> +#
> +# @in: options of the capture stream
> +#
> +# @out: options of the playback stream
> +#
> +# @threshold: #optional set the threshold (in frames) when playback starts
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevAlsaOptions',
> +  'data': {
> +    'in':         'AudiodevAlsaPerDirectionOptions',
> +    'out':        'AudiodevAlsaPerDirectionOptions',
> +    '*threshold': 'int' } }

Do we have an established term for a "direction"?

If not: the use with 'in' and 'out' tempts me to name the type
AudiodevAlsaIOOptions.

Just rambling, use what you think is best.

> +
> +##
> +# @AudiodevDsoundOptions
> +#
> +# Options of the dsound audio backend.
> +#
> +# @latency: #optional add extra latency to playback (in microseconds)

We already use seconds, milliseconds and nanoseconds.  You make the zoo
complete.

> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevDsoundOptions',
> +  'data': {
> +    '*latency': 'int' } }
> +
> +##
> +# @AudiodevOssPerDirectionOptions
> +#
> +# Options of the oss backend that are used for both playback and recording.
> +#
> +# @dev: #optional path of the oss device

Who picks the default, QEMU or OSS?

For ALSA, you documented @dev to be "the name", here it's "path".
Intentional difference?

> +#
> +# @try-poll: #optional attempt to use poll mode
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevOssPerDirectionOptions',
> +  'data': {
> +    '*dev':      'str',
> +    '*try-poll': 'bool' } }

Same as for ALSA.  Keeping them separate is fine with me.

> +
> +##
> +# @AudiodevOssOptions
> +#
> +# Options of the oss audio backend.
> +#
> +# @in: options of the capture stream
> +#
> +# @out: options of the playback stream
> +#
> +# @mmap: #optional try using memory mapped access

What happens when the attempt fails?

Should this be called try-mmap?

> +#
> +# @exclusive: #optional open device in exclusive mode (vmix won't work)
> +#
> +# @dsp-policy: #optional set the timing policy of the device, -1 to use 
> fragment
> +#              mode (option ignored on some platforms)

What are the possible values besides -1?

> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevOssOptions',
> +  'data': {
> +    'in':          'AudiodevOssPerDirectionOptions',
> +    'out':         'AudiodevOssPerDirectionOptions',
> +    '*mmap':       'bool',
> +    '*exclusive':  'bool',
> +    '*dsp-policy': 'int' } }
> +
> +##
> +# @AudiodevPaOptions
> +#
> +# Options of the pa (PulseAudio) audio backend.
> +#
> +# @server: #optional PulseAudio server address
> +#
> +# @sink: #optional sink device name
> +#
> +# @source: #optional source device name

Who picks the defaults, QEMU or PA?

> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevPaOptions',
> +  'data': {
> +    '*server': 'str',
> +    '*sink':   'str',
> +    '*source': 'str' } }
> +
> +##
> +# @AudiodevWavOptions
> +#
> +# Options of the wav audio backend.
> +#
> +# @path: #optional path of the wav file to record
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevWavOptions',
> +  'data': {
> +    '*path': 'str' } }

Who picks the default?

> +
> +
> +##
> +# @AudiodevBackendOptions
> +#
> +# A discriminated record of audio backends.
> +#
> +# Since: 2.4
> +##
> +{ 'union': 'AudiodevBackendOptions',
> +  'data': {
> +    'none':      'AudiodevNoOptions',
> +    'alsa':      'AudiodevAlsaOptions',
> +    'coreaudio': 'AudiodevNoOptions',
> +    'dsound':    'AudiodevDsoundOptions',
> +    'oss':       'AudiodevOssOptions',
> +    'pa':        'AudiodevPaOptions',
> +    'sdl':       'AudiodevNoOptions',
> +    'spice':     'AudiodevNoOptions',
> +    'wav':       'AudiodevWavOptions' } }
> +
> +##
> +# @AudioFormat
> +#
> +# An enumeration of possible audio formats.
> +#
> +# Since: 2.4
> +##
> +{ 'enum': 'AudioFormat',
> +  'data': [ 'u8', 's8', 'u16', 's16', 'u32', 's32' ] }
> +
> +##
> +# @AudiodevPerDirectionOptions
> +#
> +# General audio backend options that are used for both playback and 
> recording.
> +#
> +# @fixed-settings: #optional use fixed settings for host DAC/ADC
> +#
> +# @frequency: #optional frequency to use when using fixed settings
> +#
> +# @channels: #optional number of channels when using fixed settings
> +#
> +# @format: #optional sample format to use when using fixed settings

Are these guys used when @fixed-settings is off?

> +#
> +# @buffer: #optional the buffer size (in microseconds)

@buffer suggests this is a buffer, not a buffer length given as time
span.  @buffer-len?

> +#
> +# @buffer-count: #optional number of buffers
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevPerDirectionOptions',
> +  'data': {
> +    '*fixed-settings': 'bool',
> +    '*frequency':      'int',
> +    '*channels':       'int',
> +    '*voices':         'int',
> +    '*format':         'AudioFormat',
> +    '*buffer':         'int',
> +    '*buffer-count':   'int' } }
> +
> +##
> +# @Audiodev
> +#
> +# Captures the configuration of an audio backend.
> +#
> +# @id: identifier of the backend
> +#
> +# @in: options of the capture stream
> +#
> +# @out: options of the playback stream
> +#
> +# @timer-period: #optional timer period (in microseconds, 0: use lowest
> +#                possible)
> +#
> +# @opts: audio backend specific options
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'Audiodev',
> +  'data': {
> +    '*id':           'str',
> +    'in':            'AudiodevPerDirectionOptions',
> +    'out':           'AudiodevPerDirectionOptions',
> +    '*timer-period': 'int',
> +    'opts':          'AudiodevBackendOptions' } }

Have you considered making this a flat union, similar ro
BlockdevOptions?

Don't get deceived by the number of my questions, this is solid work.