Re: [Qemu-devel] [PATCH 07/12] qapi: qapi for audio backends

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5458 bytes --]

On 06/12/2015 06:33 AM, Kővágó, Zoltán wrote:
> 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 <DirtY.iCE.hu@gmail.com>
> 
> ---

> +++ b/qapi/audio.json
> @@ -0,0 +1,217 @@
> +# -*- mode: python -*-
> +
> +##
> +# @AudiodevNoneOptions

Might be nice to include copyright/license, but this is not the first
.json file missing it.

> +#
> +# The none, coreaudio, sdl and spice audio backend has no options.

s/has/have/

> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevNoneOptions',
> +  'data': { } }

Maybe s/None/No/ (since it is shared by backends that have no options),
but I can live with it as-is (since it is used by the 'none' backend).


> +##
> +# @AudiodevAlsaOptions
> +#
> +# Options of the alsa audio backend.
> +#
> +# @in: #optional options of the capture stream
> +#
> +# @out: #optional options of the playback stream

Marked optional here...

> +#
> +# @threshold: #optional set the threshold (in frames) when playback starts
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevAlsaOptions',
> +  'data': {
> +    'in':         'AudiodevAlsaPerDirectionOptions',
> +    'out':        'AudiodevAlsaPerDirectionOptions',

...but not here.

> +    '*threshold': 'int' } }
> +
> +##
> +# @AudiodevDsoundOptions
> +#
> +# Options of the dsound audio backend.
> +#
> +# @latency-millis: #optional add extra latency to playback
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevDsoundOptions',
> +  'data': {
> +    '*latency-millis': 'int' } }

Style question - should we just call this 'latency', and document the
milliseconds unit in the description? But having the name latency_millis
in C code might not be all that bad, so you may not want to change this one.


> +##
> +# @AudiodevOssOptions
> +#
> +# Options of the oss audio backend.
> +#
> +# @in: #optional options of the capture stream
> +#
> +# @out: #optional options of the playback stream
> +#
> +# @mmap: #optional try using memory mapped access
> +#
> +# @exclusive: #optional open device in exclusive mode (vmix wont work)

s/wont/won't/

> +#
> +# @dsp-policy: #optional set the timing policy of the device, -1 to use fragment
> +#              mode (option ignored on some platforms)
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevOssOptions',
> +  'data': {
> +    'in':          'AudiodevOssPerDirectionOptions',
> +    'out':         'AudiodevOssPerDirectionOptions',

Again, inconsistent on the optional marking.


> +##
> +# @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 fortmat to use when using fixed settings

s/fortmat/format/

> +#
> +# @buffer-usecs: #optional the buffer size in microseconds
> +#
> +# @buffer-count: #optional nuber of buffers
> +#

s/nuber/number/

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

Another mismatch in optional marking.

> +    '*timer-period': 'int',
> +    'opts':          'AudiodevBackendOptions' } }
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]