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 13:48 keltezéssel, Markus Armbruster írta:
>> "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:
[...]
>>>>> +##
>>>>> +# @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?
>
> Not really right now. The default is a NULL pointer (pulseaudio api
> wise), so unless we add an arbitrary keyword (like default), it's not
> possible to ask explicitly for the default. (But omitting them will
> choose the default, of course.)

Treating an empty string like NULL should get us a way to ask for the
default, and a way to document the default concisely, like (default '')
plus a suitable explanation what '' means.

I'm not saying you should do that.  I'm saying whatever you do, document
what happens when an optional parameter is absent :)

>>>>> +#
>>>>> +# 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?
>
> No, they all have defaults: 44100 Hz, 2 channels and s16 format.

Okay, this sort of explains why you have @fixed-settings.

My first thought was that @fixed-settings is redundant, because we can
have any of @frequency, @channels, @format imply fixed settings.  Except
that doesn't let you ask for the *default* fixed settings, as you have
to specify at least one.

What's the default for @fixed-settings?

What if I specify frequency, channels or format together with explicit
fixed-settings: false?

>                                                                  I
> guess I should also document it...

Yes, please.

>>>>> +#
>>>>> +# @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' } }
[...]