qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Kővágó Zoltán" <dirty.ice.hu@gmail.com>
To: Eric Blake <eblake@redhat.com>, qemu-devel@nongnu.org
Cc: Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [Qemu-devel] [RFC PATCH] qapi for audio backends
Date: Wed, 03 Jun 2015 21:57:10 +0200	[thread overview]
Message-ID: <556F5C16.5030308@gmail.com> (raw)
In-Reply-To: <556F52C6.6000705@redhat.com>

2015-06-03 21:17 keltezéssel, Eric Blake írta:
> On 06/03/2015 10:48 AM, Kővágó, Zoltán wrote:
>> This is a proposal to add structures into qapi-schema.json to replace the
>> existing configuration structures used by audio backends currently. I'm going to
>> use it to implement a new way to specify audio backend options (an -audiodev
>> command line option, instead of environment variables. This will also allow us
>> to use multiple audio backends in one qemu instance), so the structure used here
>> will be the basis of the command line syntax.
>>
>> This is currently more or less a direct translation of the current audio backend
>> options. I've changed some names, trying to accomplish a more consistent naming
>> scheme. I wouldn't be surprised if there were options that doesn't work if you
>> set their value to anything other than the default (for example, log to monitor
>> can crash qemu, QEMU_DSOUND_RESTOURE_RETRIES has a typo, so probably nobody used
>> it, etc). I've also tried to reduce copy-paste, when the same set of options can
>> be given to output and input (QEMU_*_DAC_* and QEMU_*_ADC_* options), also using
>> in and out instead of ADC and DAC, as in the world of SPDIF and HDMI it's
>> completely possible that your computer has nothing to do with analog converters.
>> Plus a non technician user probably has no idea what ADC and DAC stands for.
>>
>> Any comment is appreciated.
>>
>> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
>> ---
>>   qapi-schema.json | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 330 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 0662a9b..ff67d5a 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3769,3 +3769,333 @@
>>   # Since: 2.1
>>   ##
>>   { 'command': 'rtc-reset-reinjection' }
>> +
>> +##
>> +# @AudiodevNoneOptions
>> +#
>> +# The dummy audio backend has no options.
>> +#
>> +# Since: XXX
>
> It's okay to tentatively put 2.4 here, if you are aiming for 2.4.  If
> you think it will be a big enough project to miss the current release
> window, put 2.5.
>
>> +##
>> +{ 'struct': 'AudiodevNoneOptions',
>> +  'data': { } }
>> +
>> +##
>> +# @AudiodevAlsaPerDirectionOptions
>> +#
>> +# Options of the alsa backend that are used for both playback and recording.
>> +#
>> +# @dev: #optional the name of the alsa device to use.
>> +#
>> +# @period_size_usec: #optional the period size in microseconds. Must not be
>> +#                    specified with @period_size_frames.
>> +#
>> +# @period_size_frames: #optional the period size in frames. Must not be
>> +#                      specified with @period_size_usec.
>> +#
>> +# @buffer_size_usec: #optional the buffer size in microseconds. Must not be
>> +#                    specified with @buffer_size_frames.
>> +#
>> +# @buffer_size_frames: #optional the buffer size in frames. Must not be
>> +#                      specified with @buffer_size_usec.
>
> Can we name these with s/_/-/?  We've documented that QMP prefers dash
> unless there is compelling reason or consistency to worry about, and I
> don't see the compelling reason here.

There's no particular reason other than when I looked through 
qapi-schema.json I've mostly seen underscores. Will fix this.

>
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevAlsaPerDirectionOptions',
>> +  'data': {
>> +    '*dev':                'str',
>> +    '*period_size_usec':   'int',
>> +    '*period_size_frames': 'int',
>> +    '*buffer_size_usec':   'int',
>> +    '*buffer_size_frames': 'int' } }
>> +
>> +##
>> +# @AudiodevAlsaOptions
>> +#
>> +# Options of the alsa audio backend.
>> +#
>> +# @in: #optional options of the capture stream.
>> +#
>> +# @out: #optional options of the playback stream.
>> +#
>> +# @threshold: #optional
>
> Document this.

This (and some other option) currently only has the documentation 
"(undocumented)", but I will try to figure out what they do...

>
>> +#
>> +# @verbose: #optional behave in a more verbose way
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevAlsaOptions',
>> +  'data': {
>> +    '*in':  'AudiodevAlsaPerDirectionOptions',
>> +    '*out': 'AudiodevAlsaPerDirectionOptions',
>> +    '*threshold': 'int',
>> +    '*verbose':   'bool' } }
>> +
>> +##
>> +# @AudiodevCoreaudioOptions
>> +#
>> +# Options of the coreaudio backend.
>> +#
>> +# @buffer_size: #optional size of the buffer in frames
>> +#
>> +# @buffer_count: #optional number of buffers
>
> Again, dashes would be nicer, if there is no compelling reason otherwise
> (I'll quit repeating it).
>
>> +#
>> +# Since: XXX
>
> (and I'll quit pointing out XXX in Since lines)
>
>> +##
>> +{ 'struct': 'AudiodevCoreaudioOptions',
>> +  'data': {
>> +    '*buffer_size':  'int',
>> +    '*buffer_count': 'int' } }
>> +
>> +##
>> +# @AudiodevDsoundPerDirectionOptions
>> +#
>> +# Options of the dsound backend that are used for both playback and recording.
>> +#
>> +# @bufsize: #optional
>
> Document this
>
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevDsoundPerDirectionOptions',
>> +  'data' : {
>> +    '*bufsize': 'int' } }
>> +
>> +##
>> +# @AudiodevDsoundOptions
>> +#
>> +# Options of the dsound audio backend.
>> +#
>> +# @in: #optional options of the capture stream.
>> +#
>> +# @out: #optional options of the playback stream.
>> +#
>> +# @lock_retries: #optional number of times to attempt locking the buffer
>> +#
>> +# @restore_retries: #optional number of times to attempt restoring the buffer
>> +#
>> +# @getstatus_retries: #optional number of times to attempt getting status of the
>
> Borders on being a long line (yes, it's exactly 80, but I tend to stick
> to < 80)
>
>> +#                     buffer
>> +#
>> +# @set_primary: #optional set the parameters of primary buffer
>> +#
>> +# @latency_millis: #optional
>> +#
>> +# @primary_freq: #optional primary buffer frequency
>> +#
>> +# @primary_channels: #optional primary buffer number of channels
>> +#
>> +# @primary_format: #optional primary buffer format
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevDsoundOptions',
>> +  'data': {
>> +    '*in':                'AudiodevDsoundPerDirectionOptions',
>> +    '*out':               'AudiodevDsoundPerDirectionOptions',
>> +    '*lock_retries':      'int',
>> +    '*restore_retries':   'int',
>> +    '*getstatus_retries': 'int',
>> +    '*set_primary':       'bool',
>> +    '*latency_millis':    'int',
>> +    '*primary_freq':      'int',
>> +    '*primary_channels':  'int',
>> +    '*primary_format':    'AudioFormat' } }
>> +
>> +##
>> +# @AudiodevOssPerDirectionOptions
>> +#
>> +# Options of the oss backend that are used for both playback and recording.
>> +#
>> +# @dev: #optional path of the oss device
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevOssPerDirectionOptions',
>> +  'data': {
>> +    '*dev': 'str' } }
>> +
>> +##
>> +# @AudiodevOssOptions
>> +#
>> +# Options of the oss audio backend.
>> +#
>> +# @in: #optional options of the capture stream.
>> +#
>> +# @out: #optional options of the playback stream.
>> +#
>> +# @fragsize: #optional fragment size in bytes
>> +#
>> +# @frags: #optional number of fragments
>> +#
>> +# @mmap: #optional try using memory mapped access
>> +#
>> +# @exclusive: #optional open device in exclusive mode (vmix wont work)
>> +#
>> +# @dsp_policy: #optional set the timing policy of the device, -1 to use fragment
>> +#              mode (option ignored on some platforms)
>> +#
>> +# @debug: #optional turn on some debugging messages
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevOssOptions',
>> +  'data': {
>> +    '*in':         'AudiodevOssPerDirectionOptions',
>> +    '*out':        'AudiodevOssPerDirectionOptions',
>> +    '*fragsize':   'int',
>> +    '*frags':      'int',
>> +    '*mmap':       'bool',
>> +    '*exclusive':  'bool',
>> +    '*dsp_policy': 'int',
>> +    '*debug':      'bool' } }
>> +
>> +##
>> +# @AudiodevPaOptions
>> +#
>> +# Options of the pa audio backend.
>> +#
>> +# @samples: #optional buffer size in samples
>> +#
>> +# @server: #optional PulseAudio server address
>
> Worth mentioning that 'pa' == PulseAudio earlier in the docs?
>
>> +#
>> +# @sink: #optional sink device name
>> +#
>> +# @source: #optional source device name
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevPaOptions',
>> +  'data': {
>> +    '*samples': 'int',
>> +    '*server':  'str',
>> +    '*sink':    'str',
>> +    '*source':  'str' } }
>> +
>> +##
>> +# @AudiodevSdlOptions
>> +#
>> +# Options of the sdl audio backend. (Note that most options are only changeable
>> +# through SDL's environment variables).
>> +#
>> +# @samples: #optional size of SDL buffer in samples
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevSdlOptions',
>> +  'data': {
>> +    '*samples': 'int' } }
>> +
>> +##
>> +# @AudiodevSpiceOptions
>> +#
>> +# The spice audio backend has no options.
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevSpiceOptions',
>> +  'data': { } }
>> +
>> +##
>> +# @AudiodevWavOptions
>> +#
>> +# Options of the wav audio backend.
>> +#
>> +# @frequency: #optional frequency of the wav file
>> +#
>> +# @format: #optional sample format of the wav file
>> +#
>> +# @channels: #optional number of channels of the wav file
>> +#
>> +# @path: #optional path of the wav file to record.
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevWavOptions',
>> +  'data': {
>> +    '*frequency': 'int',
>> +    '*format':    'AudioFormat',
>> +    '*channels':  'int',
>> +    '*path':       'str' } }
>
> Inconsistent indentation.
>
>> +
>> +
>> +##
>> +# @AudiodevBackendOptions
>> +#
>> +# A discriminated record of audio backends.
>> +#
>> +# Since: XXX
>> +##
>> +{ 'union': 'AudiodevBackendOptions',
>> +  'data': {
>> +    'none':      'AudiodevNoneOptions',
>> +    'alsa':      'AudiodevAlsaOptions',
>> +    'coreaudio': 'AudiodevCoreaudioOptions',
>> +    'dsound':    'AudiodevDsoundOptions',
>> +    'oss':       'AudiodevOssOptions',
>> +    'pa':        'AudiodevPaOptions',
>> +    'sdl':       'AudiodevSdlOptions',
>> +    'spice':     'AudiodevSpiceOptions',
>> +    'wav':       'AudiodevWavOptions' } }
>
> AudiodevNoneOptions and AudiodevSpiceOptions are identical; you could
> consolidate them into one struct.  I'd also (someday) like to get to the
> point of using anonymous structures in a union, particularly when the
> variant adds no additional fields, so that you could do:
>
> 'data': {
>    'none': {},
>    'alsa': 'AudiodevAlsaOptions', ...
>
> but don't know if that is worth doing any sooner than Markus can land
> his introspection patches.
>
>> +
>> +##
>> +# @AudioFormat
>> +#
>> +# An enumeration of possible audio formats.
>> +#
>> +# Since: XXX
>> +##
>> +{ '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 fortmat to use when using fixed settings
>> +#
>> +# @try_poll: #optional attempt to use poll mode
>> +#
>> +# Since: XXX
>> +##
>> +{ 'struct': 'AudiodevPerDirectionOptions',
>> +  'data': {
>> +    '*fixed_settings': 'bool',
>> +    '*frequency':      'int',
>> +    '*channels':       'int',
>> +    '*format':         'AudioFormat',
>> +    '*try_poll':       'bool' } }
>> +
>> +##
>> +# @Audiodev
>> +#
>> +# Captures the configuration of an audio backend.
>> +#
>> +# @id: identifier of the backend.
>
> Inconsistent on whether you end with '.' (but the whole file is already
> that inconsistent, so not too much of a worry)
>
>> +#
>> +# @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)
>> +#
>> +# @plive: #optional
>> +#
>> +# @opts: audio backend specific options
>> +#
>> +# Since XXX
>> +##
>> +{ 'struct': 'Audiodev',
>> +  'data': {
>> +    'id':            'str',
>> +    '*in':           'AudiodevPerDirectionOptions',
>> +    '*out':          'AudiodevPerDirectionOptions',
>> +    '*timer_period': 'int',
>> +    '*plive':        'bool',
>> +    'opts':          'AudiodevBackendOptions' } }
>>
>
> Overall looks fairly reasonable. Is it enough structures to be worth
> splitting into a new qapi/audio.json file and merely touch
> qapi-schema.json to add an include?
>

Maybe. Given that there are a few quite short includes already, it 
should be ok...

  reply	other threads:[~2015-06-03 19:57 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-03 16:48 [Qemu-devel] [RFC PATCH] qapi for audio backends Kővágó, Zoltán
2015-06-03 19:17 ` Eric Blake
2015-06-03 19:57   ` Kővágó Zoltán [this message]
2015-06-04  7:43 ` Gerd Hoffmann
2015-06-04 13:33   ` Kővágó Zoltán

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=556F5C16.5030308@gmail.com \
    --to=dirty.ice.hu@gmail.com \
    --cc=eblake@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).