Re: [Qemu-devel] [PATCH v4 01/14] qapi: qapi for audio backends

[Zoltán Kővágó]

On 2019-01-29 14:33, Markus Armbruster wrote:
> "Kővágó, Zoltán" <address@hidden> writes:
[...]
>> +
>> +##
>> +# @AudiodevPaOptions:
>> +#
>> +# Options of the pa (PulseAudio) audio backend.
>> +#
>> +# @server: PulseAudio server address (default: let PulseAudio choose)
>> +#
>> +# @sink: name of the sink to use
>> +#
>> +# @source: name of the source to use
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'AudiodevPaOptions',
>> +  'data': {
>> +    '*server': 'str',
>> +    '*sink':   'AudiodevPaPerDirectionOptions',
>> +    '*source': 'AudiodevPaPerDirectionOptions' } }
>> +
>> +##
>> +# @AudiodevWavOptions:
>> +#
>> +# Options of the wav audio backend.
>> +#
>> +# @path: name of the wav file to record (default 'qemu.wav')
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'struct': 'AudiodevWavOptions',
>> +  'data': {
>> +    '*path': 'str' } }
> 
> Pattern so far:
> 
> * AudiodevFooOptions 
> 
>   Foo           direction options       other options
>   Alsa          alsa-in, alsa-out       threshold
>   Dsound                                latency
>   Oss           oss-in, oss-out         try-map, exclusive, dsp-policy
>   Pa            sink, source            server
>   Wav                                   path
> 
> Any particular reason for naming the direction options differently?

I probably chose that name because in PulseAudio parlance you have
sources and sinks, and the current environment variable based setting of
pa doesn't use ADC/DAC to refer to source/sink, but now that you mention
it it probably makes sense to rename them to pa-in and pa-out, it's more
uniform that way.

[...]
>> +##
>> +# @Audiodev:
>> +#
>> +# Options of an audio backend.
>> +#
>> +# @id: identifier of the backend
>> +#
>> +# @driver: the backend driver to use
>> +#
>> +# @in: options of the capture stream
>> +#
>> +# @out: options of the playback stream
>> +#
>> +# @timer-period: timer period (in microseconds, 0: use lowest possible)
>> +#
>> +# Since: 4.0
>> +##
>> +{ 'union': 'Audiodev',
>> +  'base': {
>> +    'id':            'str',
>> +    'driver':        'AudiodevDriver',
>> +    '*in':           'AudiodevPerDirectionOptions',
>> +    '*out':          'AudiodevPerDirectionOptions',
>> +    '*timer-period': 'uint32' },
>> +  'discriminator': 'driver',
>> +  'data': {
>> +    'alsa':      'AudiodevAlsaOptions',
>> +    'dsound':    'AudiodevDsoundOptions',
>> +    'oss':       'AudiodevOssOptions',
>> +    'pa':        'AudiodevPaOptions',
>> +    'wav':       'AudiodevWavOptions' } }
> 
> 'none', 'coreaudio', 'sdl' and 'spice' have no driver-specific options.
> 
> Do all the generic options apply to all drivers?  Even 'none'?

'fixed-settings', 'frequency', 'channels', 'voices' and 'format' are
used by the mixeng thus they apply to all backends, even 'none'.
(Whether it makes any sense to set them is a different story.)

'buffer-len' and 'buffer-count' was introduced as a means to give a more
uniform interface to setting buffer sizes, because currently to set
buffer size:

* in case of alsa: you set microseconds or frames depending on a second
environment variable
* in case of coreaudio: you set it in frames
* in case of dsound, oss: you set it in milliseconds
* in case of pa, sdl: you set it in samples
* in case of spice, wav: you can't set it

I added 'buffer-len' and 'buffer-count' because they seemed like a
generic option that *most* backends would support, and I didn't want to
create extra backend specific options for them.  But if I combine it
with your inheritance-like proposal, it's probably not that bad.

> Do @in and @out apply to drivers 'dsound' and 'wav'?

@out applies to every driver.  @in shouldn't apply to drivers without
input support, but currently the base of mixeng is created for the input
too, it'll just fail if the sound card tries to create a capture stream...

> 
> Per-direction options are split between generic ones in @in and @out,
> and driver-specific ones in @alsa-in and @alsa-out / @oss-in and
> @oss-out / @sink and @source.
> 
> Perhaps the following would be tidier: make the generic per-direction
> options AudiodevPerDirectionOptions the base of driver-specific ones
> AudiodevAlsaPerDirectionOptions, AudiodevOssPerDirectionOptions,
> AudiodevPaPerDirectionOptions, and then
> 
>    { 'struct': 'AudiodevGenericOptions',
>      'data': {
>        '*in':     'AudiodevPerDirectionOptions',
>        '*out':    'AudiodevPerDirectionOptions' } }
> 
>    { 'union': 'Audiodev',
>      'base': {
>        'id':            'str',
>        'driver':        'AudiodevDriver',
>        '*timer-period': 'uint32' },
>      'discriminator': 'driver',
>      'data': {
>        'none':      'AudiodevGenericOptions',
>        'alsa':      'AudiodevAlsaOptions',
>        'coreaudio:  'AudiodevGenericOptions',
>        'dsound':    'AudiodevDsoundOptions',
>        'oss':       'AudiodevOssOptions',
>        'pa':        'AudiodevPaOptions',
>        'sdl:        'AudiodevGenericOptions',
>        'spice':     'AudiodevGenericOptions',
>        'wav':       'AudiodevWavOptions' } }

Neat, I'll play around with this a bit.

>> diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> index 3bbdfcee84..d5f9856be7 100644
>> --- a/qapi/qapi-schema.json
>> +++ b/qapi/qapi-schema.json
>> @@ -95,3 +95,4 @@
>>  { 'include': 'trace.json' }
>>  { 'include': 'introspect.json' }
>>  { 'include': 'misc.json' }
>> +{ 'include': 'audio.json' }
> 
> Looks pretty good.
> 

Regards,
Zoltan