Re: [Qemu-devel] [PATCH 03/51] qapi: qapi for audio backends

[Eric Blake <eblake@redhat.com>]

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

On 01/14/2016 06:45 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 backend specific option (as all other backends
>   currently ignore it)
> 
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---

> +
> +##
> +# @AudiodevNoOptions
> +#
> +# The none, coreaudio, sdl and spice audio backend have no options.
> +#
> +# Since: 2.6
> +##
> +{ 'struct': 'AudiodevNoOptions',
> +  'data': { } }

I've got pending patches that will let us write a flat union as:

... 'data': { 'none': {},
              'coreaudio': {},
              'foo': 'FooType', ...

but until that lands, your explicit empty-type placeholder is fine.

> +##
> +{ 'struct': 'AudiodevCommonOptions',
> +  'data': {
> +    'id':            'str',
> +    'driver':        'AudiodevDriver',
> +    'in':            'AudiodevPerDirectionOptions',
> +    'out':           'AudiodevPerDirectionOptions',
> +    '*timer-period': 'int' } }
> +
> +##
> +# @AudiodevBackendOptions
> +#
> +# Options of an audio backend.
> +#
> +# Since: 2.6
> +##
> +{ 'union': 'Audiodev',
> +  'base': 'AudiodevCommonOptions',

My pending patches will also allow an anonymous base class, so that we
don't have to specify an intermediate AudiodevCommonOptions class.  Not
a show-stopper, though.

Overall looks rather sane (I think we've done the bulk of the reviewing
last summer), although I didn't read it closely enough for a R-b tag at
this time.

-- 
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 --]