qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH] qapi for audio backends


From: Kővágó Zoltán
Subject: Re: [Qemu-devel] [RFC PATCH] qapi for audio backends
Date: Wed, 03 Jun 2015 21:57:10 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

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 <address@hidden>
---
  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 via email to

[Prev in Thread] Current Thread [Next in Thread]