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

[Kővágó Zoltán]

2015-06-17 15:37 keltezéssel, Markus Armbruster írta:
> "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?

It's on by default.

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

They will be ignored.

The audio system currently work like this: when an audio frontend wants 
to open an output with some format (frequency, channels, format) it 
checks fixed-settings. If it's false, it will just open the stream with 
the frontend specified settings. If it's true, it'll convert it into the 
format specified by @frequency, @channels, @format, then pass this 
converted/recoded stream to the backend.

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