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

[Markus Armbruster]

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

> 2015-06-17 09:46 keltezéssel, Markus Armbruster írta:
>> 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?
>
> It defaults to "default", which tells alsa to use the default device...

Then make it

    # @dev: #optional the name of the alsa device to use (default 'default')

>>
>>> +#
>>> +# @try-poll: #optional attempt to use poll mode
>>
>> What happens when the attempt fails?
>
> It falls back to non polling (timer based) mode.

Okay, assuming that's the user interface we want.

>>> +#
>>> +# 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.
>
> I don't know, so far nobody rambled about my naming...
>
>>
>>> +
>>> +##
>>> +# @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.
>
> Hmm. The audio backend previously used microseconds, milliseconds,
> Hertz, frames, samples and bytes as time measurement, now at least
> everything use microseconds. Milliseconds precision is not enough,
> while nanosecond precision doesn't really make sense imho.

I'm just poking fun at our inability to pick a time unit for QAPI/QMP.
No objection to your use of microseconds.

>>> +#
>>> +# 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?
>
> It defaults to "/dev/dsp".
>
>> For ALSA, you documented @dev to be "the name", here it's "path".
>> Intentional difference?
>
> Yes. For oss you have to provide a filesystem path of the audio device
> (like /dev/dsp), for alsa you provide an alsa specific name, like
> "default", "hw:1,0", "hdmi:CARD=NVidia,DEV=1", whatever.

I don't like calling a filename a path, even though we already do in
many places.  Let's do:

    # @dev: #optional file name of the oss device (default '/dev/dsp')

>>> +#
>>> +# @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.
>
> This is the same as alsa's try-poll. They're separate because only
> these two backend use this setting.
>>
>>> +
>>> +##
>>> +# @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?
>
> It should fall back to non-mmapped access (should because when I
> checked, it was buggy, at least on linux with alsa emulated oss on
> pulseaudio alsa device...)

Okay, assuming that's the user interface we want.

>> Should this be called try-mmap?
> Agree.
>
>>> +#
>>> +# @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?
>
> It should be a number between 0 and 10 (according to this page:
> http://manuals.opensound.com/developer/SNDCTL_DSP_POLICY.html)

Please cover that in your comment.

>>> +#
>>> +# 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?
>
> PA

Is there a way to explicitly ask for the PA default?  Something like
source=default?

>>> +#
>>> +# 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?
>
> It defaults to "qemu.wav"

Make it

    # @path: #optional path of the wav file to record (default 'qemu.wav')

>>> +
>>> +
>>> +##
>>> +# @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?
>
> No.

If @fixed-settings, are the other three all required?  If not, what are
their defaults?

>>> +#
>>> +# @buffer: #optional the buffer size (in microseconds)
>>
>> @buffer suggests this is a buffer, not a buffer length given as time
>> span.  @buffer-len?
>
> Ok. (It used to be called buffer-usecs before I changed everything to
> microseconds.)
>
>>
>>> +#
>>> +# @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?
>
> Not really. If you qapi masters out there think it's better, then I
> will convert it.

Related: discussion about flattening in review of PATCH 2.

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