-audiodev option

This patch adds structures into qapi to replace the existing configuration structures used by audio backends currently. This qapi will be the base of the -audiodev command line parameter (that replaces the old environment variables based config).

This is not a 1:1 translation of the old options, I've tried to make them much more consistent (e.g. almost every backend had an option to specify buffer size, but the name was different for every backend, and some backends required usecs, while some other required frames, samples or bytes). Also tried to reduce the number of abbreviations used by the config keys.

Some of the more important changes:
* use in and out instead of ADC and DAC, as the former is more user friendly imho
* moved buffer settings into the global setting area (so it's the same for all backends that support it. Backends that can't change buffer size will simply ignore them). Also using usecs, as it's probably more user friendly than samples or bytes.
* try-poll is now an alsa and oss backend specific option (as all other backends currently ignore it)

Signed-off-by Kővágó, Zoltán <[email protected]>

---

Changes from v1 patch:
* every time-related field now take usecs (and removed -usecs, -millis suffixes)
* fixed inconsisten optional marking, language issues

Changes from v2 RFC patch:
* in, out are no longer optional
* try-poll: moved to alsa and oss (as no other backend used them)
* voices: added (env variables had this option)
* dsound: removed primary buffer related fields

Changes from v1 RFC patch:
* fixed style issues
* moved definitions into a separate file
* documented undocumented options (hopefully)
* removed plive option. It was useless even years ago so it can probably safely go away: https://lists.nongnu.org/archive/html/qemu-devel/2012-03/msg02427.html
* removed verbose, debug options. Backends should use trace events instead.
* removed *_retries options from dsound. It's a kludge.
* moved buffer_usecs and buffer_count to the global config options. Some driver might ignore it (as they do not expose API to change them).
* wav backend: removed frequecy, format, channels as AudiodevPerDirectionOptions already have them.

Makefile | 4 +-

 qapi-schema.json |   3 +
 qapi/audio.json  | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+), 2 deletions(-)
 create mode 100644 qapi/audio.json

Makefile +2 -2

index 3f97904..ac566fa 100644
@@ -257,8 +257,8 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
 		"  GEN   $@")
 
 qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
-               $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
-               $(SRC_PATH)/qapi/event.json
+               $(SRC_PATH)/qapi/audio.json  $(SRC_PATH)/qapi/block.json \
+               $(SRC_PATH)/qapi/block-core.json $(SRC_PATH)/qapi/event.json
 
 qapi-types.c qapi-types.h :\
 $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)

qapi-schema.json +3 -0

index 106008c..e751ea3 100644
@@ -5,6 +5,9 @@
 # QAPI common definitions
 { 'include': 'qapi/common.json' }
 
+# QAPI audio definitions
+{ 'include': 'qapi/audio.json' }
+
 # QAPI block definitions
 { 'include': 'qapi/block.json' }
 

qapi/audio.json new file +13 -0

new file mode 100644
index 0000000..2851689
@@ -0,0 +1,223 @@
+# -*- mode: python -*-
+#
+# Copyright (C) 2015 Zoltán Kővágó <[email protected]>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+##
+# @AudiodevNoOptions
+#
+# The none, coreaudio, sdl and spice audio backend have no options.
+#
+# Since: 2.4

qapi/audio.json new file +16 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }
+
+##
+# @AudiodevAlsaOptions
+#
+# Options of the alsa audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream

qapi/audio.json new file +11 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssOptions',
+  'data': {
+    'in':          'AudiodevOssPerDirectionOptions',
+    'out':         'AudiodevOssPerDirectionOptions',
+    '*mmap':       'bool',
+    '*exclusive':  'bool',
+    '*dsp-policy': 'int' } }
+

qapi/audio.json new file +11 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevPaOptions',
+  'data': {
+    '*server': 'str',
+    '*sink':   'str',
+    '*source': 'str' } }
+
+##
+# @AudiodevWavOptions

qapi/audio.json new file +33 -0

+
+
+##
+# @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

qapi/audio.json new file +30 -0

+#
+# @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' } }
+
+##
+# @Audiodev
+#
+# Captures the configuration of an audio backend.
+#
+# @id: identifier of the backend
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @timer-period: #optional timer period (in microseconds, 0: use lowest
+#                possible)
+#
+# @opts: audio backend specific options

Copying Eric for additional QAPI schema expertise.

My questions inline, pretty sure they show my ignorance.

Quoted 106 lines · Zoltán

This patch adds structures into qapi to replace the existing configuration structures used by audio backends currently. This qapi will be the base of the -audiodev command line parameter (that replaces the old environment variables based config).

This is not a 1:1 translation of the old options, I've tried to make them much more consistent (e.g. almost every backend had an option to specify buffer size, but the name was different for every backend, and some backends required usecs, while some other required frames, samples or bytes). Also tried to reduce the number of abbreviations used by the config keys.

Some of the more important changes:
* use in and out instead of ADC and DAC, as the former is more user friendly imho
* moved buffer settings into the global setting area (so it's the same for all backends that support it. Backends that can't change buffer size will simply ignore them). Also using usecs, as it's probably more user friendly than samples or bytes.
* try-poll is now an alsa and oss backend specific option (as all other backends currently ignore it)

Signed-off-by Kővágó, Zoltán <[email protected]>

---

Changes from v1 patch:
* every time-related field now take usecs (and removed -usecs, -millis suffixes)
* fixed inconsisten optional marking, language issues

Changes from v2 RFC patch:
* in, out are no longer optional
* try-poll: moved to alsa and oss (as no other backend used them)
* voices: added (env variables had this option)
* dsound: removed primary buffer related fields

Changes from v1 RFC patch:
* fixed style issues
* moved definitions into a separate file
* documented undocumented options (hopefully)
* removed plive option. It was useless even years ago so it can probably safely go away: https://lists.nongnu.org/archive/html/qemu-devel/2012-03/msg02427.html
* removed verbose, debug options. Backends should use trace events instead.
* removed *_retries options from dsound. It's a kludge.
* moved buffer_usecs and buffer_count to the global config options. Some driver might ignore it (as they do not expose API to change them).
* wav backend: removed frequecy, format, channels as AudiodevPerDirectionOptions already have them.

Makefile | 4 +-

  qapi-schema.json |   3 +
  qapi/audio.json  | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 228 insertions(+), 2 deletions(-)

create mode 100644 qapi/audio.json

Makefile +2 -2

index 3f97904..ac566fa 100644
@@ -257,8 +257,8 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
  		"  GEN   $@")
 
  qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
-               $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
-               $(SRC_PATH)/qapi/event.json
+               $(SRC_PATH)/qapi/audio.json  $(SRC_PATH)/qapi/block.json \
+               $(SRC_PATH)/qapi/block-core.json $(SRC_PATH)/qapi/event.json
 
  qapi-types.c qapi-types.h :\
  $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)

qapi-schema.json +3 -0

index 106008c..e751ea3 100644
@@ -5,6 +5,9 @@
  # QAPI common definitions
  { 'include': 'qapi/common.json' }
 
+# QAPI audio definitions
+{ 'include': 'qapi/audio.json' }
+
  # QAPI block definitions
  { 'include': 'qapi/block.json' }
 

qapi/audio.json new file +17 -0

new file mode 100644
index 0000000..2851689
@@ -0,0 +1,223 @@
+# -*- mode: python -*-
+#
+# Copyright (C) 2015 Zoltán Kővágó <[email protected]>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+##
+# @AudiodevNoOptions
+#
+# The none, coreaudio, sdl and spice audio backend have no options.
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevNoOptions',
+  'data': { } }
+

qapi/audio.json line 23

Quoted 6 lines

qapi/audio.json new file +6 -0

+##
+# @AudiodevAlsaPerDirectionOptions
+#
+# Options of the alsa backend that are used for both playback and recording.
+#
+# @dev: #optional the name of the alsa device to use

Who picks the default, QEMU or ALSA?

qapi/audio.json line 25

Quoted 3 lines

qapi/audio.json new file +2 -0

+#
+# @try-poll: #optional attempt to use poll mode

What happens when the attempt fails?

Quoted 16 lines

qapi/audio.json new file +16 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }
+
+##
+# @AudiodevAlsaOptions
+#
+# Options of the alsa audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream

qapi/audio.json line 51

Quoted 10 lines

qapi/audio.json new file +10 -0

+#
+# @threshold: #optional set the threshold (in frames) when playback starts
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaOptions',
+  'data': {
+    'in':         'AudiodevAlsaPerDirectionOptions',
+    'out':        'AudiodevAlsaPerDirectionOptions',
+    '*threshold': 'int' } }

Do we have an established term for a "direction"?

If not: the use with 'in' and 'out' tempts me to name the type AudiodevAlsaIOOptions.

Just rambling, use what you think is best.

qapi/audio.json line 58

Quoted 8 lines

qapi/audio.json new file +7 -0

+
+##
+# @AudiodevDsoundOptions
+#
+# Options of the dsound audio backend.
+#
+# @latency: #optional add extra latency to playback (in microseconds)

We already use seconds, milliseconds and nanoseconds. You make the zoo complete.

Quoted 7 lines

qapi/audio.json new file +7 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevDsoundOptions',
+  'data': {
+    '*latency': 'int' } }
+

qapi/audio.json line 71

Quoted 6 lines

qapi/audio.json new file +6 -0

+##
+# @AudiodevOssPerDirectionOptions
+#
+# Options of the oss backend that are used for both playback and recording.
+#
+# @dev: #optional path of the oss device

Who picks the default, QEMU or OSS?

For ALSA, you documented @dev to be "the name", here it's "path". Intentional difference?

qapi/audio.json line 80

Quoted 9 lines

qapi/audio.json new file +9 -0

+#
+# @try-poll: #optional attempt to use poll mode
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }

Same as for ALSA. Keeping them separate is fine with me.

qapi/audio.json line 91

Quoted 12 lines

qapi/audio.json new file +11 -0

+
+##
+# @AudiodevOssOptions
+#
+# Options of the oss audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @mmap: #optional try using memory mapped access

What happens when the attempt fails?

Should this be called try-mmap?

qapi/audio.json line 96

Quoted 5 lines

qapi/audio.json new file +5 -0

+#
+# @exclusive: #optional open device in exclusive mode (vmix won't work)
+#
+# @dsp-policy: #optional set the timing policy of the device, -1 to use fragment
+#              mode (option ignored on some platforms)

What are the possible values besides -1?

Quoted 16 lines

qapi/audio.json new file +15 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssOptions',
+  'data': {
+    'in':          'AudiodevOssPerDirectionOptions',
+    'out':         'AudiodevOssPerDirectionOptions',
+    '*mmap':       'bool',
+    '*exclusive':  'bool',
+    '*dsp-policy': 'int' } }
+
+##
+# @AudiodevPaOptions
+#
+# Options of the pa (PulseAudio) audio backend.

qapi/audio.json line 117

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# @server: #optional PulseAudio server address
+#
+# @sink: #optional sink device name
+#
+# @source: #optional source device name

Who picks the defaults, QEMU or PA?

Quoted 15 lines

qapi/audio.json new file +15 -0

+#
+# 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

qapi/audio.json line 138

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevWavOptions',
+  'data': {
+    '*path': 'str' } }

Who picks the default?

Quoted 37 lines

qapi/audio.json new file +37 -0

+
+
+##
+# @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

qapi/audio.json line 181

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# @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?

qapi/audio.json line 183

Quoted 2 lines

qapi/audio.json new file +2 -0

+#
+# @buffer: #optional the buffer size (in microseconds)

@buffer suggests this is a buffer, not a buffer length given as time span. @buffer-len?

Quoted 36 lines

qapi/audio.json new file +35 -0

+#
+# @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' } }
+
+##
+# @Audiodev
+#
+# Captures the configuration of an audio backend.
+#
+# @id: identifier of the backend
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @timer-period: #optional timer period (in microseconds, 0: use lowest
+#                possible)
+#
+# @opts: audio backend specific options
+#
+# Since: 2.4
+##
+{ 'struct': 'Audiodev',
+  'data': {

qapi/audio.json line 223

Quoted 5 lines

qapi/audio.json new file +5 -0

+    '*id':           'str',
+    'in':            'AudiodevPerDirectionOptions',
+    'out':           'AudiodevPerDirectionOptions',
+    '*timer-period': 'int',
+    'opts':          'AudiodevBackendOptions' } }

Have you considered making this a flat union, similar ro BlockdevOptions?

Don't get deceived by the number of my questions, this is solid work.

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

Quoted 112 lines

Copying Eric for additional QAPI schema expertise.

My questions inline, pretty sure they show my ignorance.

Quoted 106 lines · Zoltán

This patch adds structures into qapi to replace the existing configuration structures used by audio backends currently. This qapi will be the base of the -audiodev command line parameter (that replaces the old environment variables based config).

This is not a 1:1 translation of the old options, I've tried to make them much more consistent (e.g. almost every backend had an option to specify buffer size, but the name was different for every backend, and some backends required usecs, while some other required frames, samples or bytes). Also tried to reduce the number of abbreviations used by the config keys.

Some of the more important changes:
* use in and out instead of ADC and DAC, as the former is more user friendly imho
* moved buffer settings into the global setting area (so it's the same for all backends that support it. Backends that can't change buffer size will simply ignore them). Also using usecs, as it's probably more user friendly than samples or bytes.
* try-poll is now an alsa and oss backend specific option (as all other backends currently ignore it)

Signed-off-by Kővágó, Zoltán <[email protected]>

---

Changes from v1 patch:
* every time-related field now take usecs (and removed -usecs, -millis suffixes)
* fixed inconsisten optional marking, language issues

Changes from v2 RFC patch:
* in, out are no longer optional
* try-poll: moved to alsa and oss (as no other backend used them)
* voices: added (env variables had this option)
* dsound: removed primary buffer related fields

Changes from v1 RFC patch:
* fixed style issues
* moved definitions into a separate file
* documented undocumented options (hopefully)
* removed plive option. It was useless even years ago so it can probably safely go away: https://lists.nongnu.org/archive/html/qemu-devel/2012-03/msg02427.html
* removed verbose, debug options. Backends should use trace events instead.
* removed *_retries options from dsound. It's a kludge.
* moved buffer_usecs and buffer_count to the global config options. Some driver might ignore it (as they do not expose API to change them).
* wav backend: removed frequecy, format, channels as AudiodevPerDirectionOptions already have them.

Makefile | 4 +-

  qapi-schema.json |   3 +
  qapi/audio.json  | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 228 insertions(+), 2 deletions(-)

create mode 100644 qapi/audio.json

Makefile +2 -2

index 3f97904..ac566fa 100644
@@ -257,8 +257,8 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
  		"  GEN   $@")
 
  qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
-               $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
-               $(SRC_PATH)/qapi/event.json
+               $(SRC_PATH)/qapi/audio.json  $(SRC_PATH)/qapi/block.json \
+               $(SRC_PATH)/qapi/block-core.json $(SRC_PATH)/qapi/event.json
 
  qapi-types.c qapi-types.h :\
  $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)

qapi-schema.json +3 -0

index 106008c..e751ea3 100644
@@ -5,6 +5,9 @@
  # QAPI common definitions
  { 'include': 'qapi/common.json' }
 
+# QAPI audio definitions
+{ 'include': 'qapi/audio.json' }
+
  # QAPI block definitions
  { 'include': 'qapi/block.json' }
 

qapi/audio.json new file +17 -0

new file mode 100644
index 0000000..2851689
@@ -0,0 +1,223 @@
+# -*- mode: python -*-
+#
+# Copyright (C) 2015 Zoltán Kővágó <[email protected]>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+##
+# @AudiodevNoOptions
+#
+# The none, coreaudio, sdl and spice audio backend have no options.
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevNoOptions',
+  'data': { } }
+

Quoted 8 lines

qapi/audio.json line 23

Quoted 6 lines

qapi/audio.json new file +6 -0

+##
+# @AudiodevAlsaPerDirectionOptions
+#
+# Options of the alsa backend that are used for both playback and recording.
+#
+# @dev: #optional the name of the alsa device to use

Who picks the default, QEMU or ALSA?

It defaults to "default", which tells alsa to use the default device...

Quoted 5 lines

qapi/audio.json line 25

Quoted 3 lines

qapi/audio.json new file +2 -0

+#
+# @try-poll: #optional attempt to use poll mode

What happens when the attempt fails?

It falls back to non polling (timer based) mode.

Quoted 29 lines

Quoted 16 lines

qapi/audio.json new file +16 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }
+
+##
+# @AudiodevAlsaOptions
+#
+# Options of the alsa audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream

qapi/audio.json line 51

Quoted 10 lines

qapi/audio.json new file +10 -0

+#
+# @threshold: #optional set the threshold (in frames) when playback starts
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaOptions',
+  'data': {
+    'in':         'AudiodevAlsaPerDirectionOptions',
+    'out':        'AudiodevAlsaPerDirectionOptions',
+    '*threshold': 'int' } }

Do we have an established term for a "direction"?

Quoted 4 lines

If not: the use with 'in' and 'out' tempts me to name the type AudiodevAlsaIOOptions.

Just rambling, use what you think is best.

I don't know, so far nobody rambled about my naming...

Quoted 11 lines

qapi/audio.json line 58

Quoted 8 lines

qapi/audio.json new file +7 -0

+
+##
+# @AudiodevDsoundOptions
+#
+# Options of the dsound audio backend.
+#
+# @latency: #optional add extra latency to playback (in microseconds)

We already use seconds, milliseconds and nanoseconds. You make the zoo complete.

Hmm. The audio backend previously used microseconds, milliseconds, Hertz, frames, samples and bytes as time measurement, now at least everything use microseconds. Milliseconds precision is not enough, while nanosecond precision doesn't really make sense imho.

Quoted 7 lines

Quoted 7 lines

qapi/audio.json new file +7 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevDsoundOptions',
+  'data': {
+    '*latency': 'int' } }
+

Quoted 8 lines

qapi/audio.json line 71

Quoted 6 lines

qapi/audio.json new file +6 -0

+##
+# @AudiodevOssPerDirectionOptions
+#
+# Options of the oss backend that are used for both playback and recording.
+#
+# @dev: #optional path of the oss device

Who picks the default, QEMU or OSS?

It defaults to "/dev/dsp".

Quoted 2 lines

For ALSA, you documented @dev to be "the name", here it's "path". Intentional difference?

Yes. For oss you have to provide a filesystem path of the audio device (like /dev/dsp), for alsa you provide an alsa specific name, like "default", "hw:1,0", "hdmi:CARD=NVidia,DEV=1", whatever.

Quoted 11 lines

qapi/audio.json line 80

Quoted 9 lines

qapi/audio.json new file +9 -0

+#
+# @try-poll: #optional attempt to use poll mode
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }

Same as for ALSA. Keeping them separate is fine with me.

This is the same as alsa's try-poll. They're separate because only these two backend use this setting.

Quoted 14 lines

qapi/audio.json line 91

Quoted 12 lines

qapi/audio.json new file +11 -0

+
+##
+# @AudiodevOssOptions
+#
+# Options of the oss audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @mmap: #optional try using memory mapped access

What happens when the attempt fails?

It should fall back to non-mmapped access (should because when I checked, it was buggy, at least on linux with alsa emulated oss on pulseaudio alsa device...)

Quoted 1 line

Should this be called try-mmap?

Agree.

Quoted 8 lines

Quoted 6 lines

qapi/audio.json new file +4 -0

+#
+# @exclusive: #optional open device in exclusive mode (vmix won't work)
+#
+# @dsp-policy: #optional set the timing policy of the device, -1

to use fragment
+# mode (option ignored on some platforms)

What are the possible values besides -1?

It should be a number between 0 and 10 (according to this page:
http://manuals.opensound.com/developer/SNDCTL_DSP_POLICY.html)

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +15 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssOptions',
+  'data': {
+    'in':          'AudiodevOssPerDirectionOptions',
+    'out':         'AudiodevOssPerDirectionOptions',
+    '*mmap':       'bool',
+    '*exclusive':  'bool',
+    '*dsp-policy': 'int' } }
+
+##
+# @AudiodevPaOptions
+#
+# Options of the pa (PulseAudio) audio backend.

Quoted 8 lines

qapi/audio.json line 117

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# @server: #optional PulseAudio server address
+#
+# @sink: #optional sink device name
+#
+# @source: #optional source device name

Who picks the defaults, QEMU or PA?

PA

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +15 -0

+#
+# 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

Quoted 8 lines

qapi/audio.json line 138

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevWavOptions',
+  'data': {
+    '*path': 'str' } }

Who picks the default?

It defaults to "qemu.wav"

Quoted 38 lines

Quoted 38 lines

qapi/audio.json new file +35 -0

+
+
+##
+# @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.

qapi/audio.json new file +2 -0

+#
+# @fixed-settings: #optional use fixed settings for host DAC/ADC

Quoted 8 lines

qapi/audio.json line 181

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# @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.

Quoted 5 lines

qapi/audio.json line 183

Quoted 2 lines

qapi/audio.json new file +2 -0

+#
+# @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.)

Quoted 36 lines

Quoted 36 lines

qapi/audio.json new file +35 -0

+#
+# @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' } }
+
+##
+# @Audiodev
+#
+# Captures the configuration of an audio backend.
+#
+# @id: identifier of the backend
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @timer-period: #optional timer period (in microseconds, 0: use lowest
+#                possible)
+#
+# @opts: audio backend specific options
+#
+# Since: 2.4
+##
+{ 'struct': 'Audiodev',
+  'data': {

Quoted 8 lines

qapi/audio.json line 223

Quoted 5 lines

qapi/audio.json new file +5 -0

+    '*id':           'str',
+    'in':            'AudiodevPerDirectionOptions',
+    'out':           'AudiodevPerDirectionOptions',
+    '*timer-period': 'int',
+    'opts':          'AudiodevBackendOptions' } }

Have you considered making this a flat union, similar ro BlockdevOptions?

Not really. If you qapi masters out there think it's better, then I will convert it.

Quoted 2 lines

Don't get deceived by the number of my questions, this is solid work.

Quoted 122 lines · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

Quoted 112 lines

Copying Eric for additional QAPI schema expertise.

My questions inline, pretty sure they show my ignorance.

Quoted 106 lines · Zoltán

This patch adds structures into qapi to replace the existing configuration structures used by audio backends currently. This qapi will be the base of the -audiodev command line parameter (that replaces the old environment variables based config).

This is not a 1:1 translation of the old options, I've tried to make them much more consistent (e.g. almost every backend had an option to specify buffer size, but the name was different for every backend, and some backends required usecs, while some other required frames, samples or bytes). Also tried to reduce the number of abbreviations used by the config keys.

Some of the more important changes:
* use in and out instead of ADC and DAC, as the former is more user friendly imho
* moved buffer settings into the global setting area (so it's the same for all backends that support it. Backends that can't change buffer size will simply ignore them). Also using usecs, as it's probably more user friendly than samples or bytes.
* try-poll is now an alsa and oss backend specific option (as all other backends currently ignore it)

Signed-off-by Kővágó, Zoltán <[email protected]>

---

Changes from v1 patch:
* every time-related field now take usecs (and removed -usecs, -millis suffixes)
* fixed inconsisten optional marking, language issues

Changes from v2 RFC patch:
* in, out are no longer optional
* try-poll: moved to alsa and oss (as no other backend used them)
* voices: added (env variables had this option)
* dsound: removed primary buffer related fields

Changes from v1 RFC patch:
* fixed style issues
* moved definitions into a separate file
* documented undocumented options (hopefully)
* removed plive option. It was useless even years ago so it can probably safely

    go away: https://lists.nongnu.org/archive/html/qemu-devel/2012-03/msg02427.html

* removed verbose, debug options. Backends should use trace events instead.
* removed *_retries options from dsound. It's a kludge.
* moved buffer_usecs and buffer_count to the global config options. Some driver might ignore it (as they do not expose API to change them).
* wav backend: removed frequecy, format, channels as AudiodevPerDirectionOptions already have them.

Makefile | 4 +-

   qapi-schema.json |   3 +
   qapi/audio.json  | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
   3 files changed, 228 insertions(+), 2 deletions(-)

create mode 100644 qapi/audio.json

Makefile +2 -2

index 3f97904..ac566fa 100644
@@ -257,8 +257,8 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
   		"  GEN   $@")
 
   qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \
-               $(SRC_PATH)/qapi/block.json $(SRC_PATH)/qapi/block-core.json \
-               $(SRC_PATH)/qapi/event.json
+               $(SRC_PATH)/qapi/audio.json  $(SRC_PATH)/qapi/block.json \
+               $(SRC_PATH)/qapi/block-core.json $(SRC_PATH)/qapi/event.json
 
   qapi-types.c qapi-types.h :\
   $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py)

qapi-schema.json +3 -0

index 106008c..e751ea3 100644
@@ -5,6 +5,9 @@
   # QAPI common definitions
   { 'include': 'qapi/common.json' }
 
+# QAPI audio definitions
+{ 'include': 'qapi/audio.json' }
+
   # QAPI block definitions
   { 'include': 'qapi/block.json' }
 

qapi/audio.json new file +17 -0

new file mode 100644
index 0000000..2851689
@@ -0,0 +1,223 @@
+# -*- mode: python -*-
+#
+# Copyright (C) 2015 Zoltán Kővágó <[email protected]>
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+
+##
+# @AudiodevNoOptions
+#
+# The none, coreaudio, sdl and spice audio backend have no options.
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevNoOptions',
+  'data': { } }
+

Quoted 8 lines

qapi/audio.json line 23

Quoted 6 lines

qapi/audio.json new file +6 -0

+##
+# @AudiodevAlsaPerDirectionOptions
+#
+# Options of the alsa backend that are used for both playback and recording.
+#
+# @dev: #optional the name of the alsa device to use

Who picks the default, QEMU or ALSA?

Quoted 1 line

It defaults to "default", which tells alsa to use the default device...

Then make it

# @dev: #optional the name of the alsa device to use (default 'default')

Quoted 7 lines

Quoted 5 lines

qapi/audio.json line 25

Quoted 3 lines

qapi/audio.json new file +2 -0

+#
+# @try-poll: #optional attempt to use poll mode

What happens when the attempt fails?

It falls back to non polling (timer based) mode.

Okay, assuming that's the user interface we want.

Quoted 48 lines

Quoted 29 lines

Quoted 16 lines

qapi/audio.json new file +16 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }
+
+##
+# @AudiodevAlsaOptions
+#
+# Options of the alsa audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream

qapi/audio.json line 51

Quoted 10 lines

qapi/audio.json new file +10 -0

+#
+# @threshold: #optional set the threshold (in frames) when playback starts
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevAlsaOptions',
+  'data': {
+    'in':         'AudiodevAlsaPerDirectionOptions',
+    'out':        'AudiodevAlsaPerDirectionOptions',
+    '*threshold': 'int' } }

Do we have an established term for a "direction"?

Quoted 4 lines

If not: the use with 'in' and 'out' tempts me to name the type AudiodevAlsaIOOptions.

Just rambling, use what you think is best.

I don't know, so far nobody rambled about my naming...

Quoted 11 lines

qapi/audio.json line 58

Quoted 8 lines

qapi/audio.json new file +7 -0

+
+##
+# @AudiodevDsoundOptions
+#
+# Options of the dsound audio backend.
+#
+# @latency: #optional add extra latency to playback (in microseconds)

We already use seconds, milliseconds and nanoseconds. You make the zoo complete.

Quoted 4 lines

Hmm. The audio backend previously used microseconds, milliseconds, Hertz, frames, samples and bytes as time measurement, now at least everything use microseconds. Milliseconds precision is not enough, while nanosecond precision doesn't really make sense imho.

I'm just poking fun at our inability to pick a time unit for QAPI/QMP. No objection to your use of microseconds.

Quoted 16 lines

Quoted 7 lines

Quoted 7 lines

qapi/audio.json new file +7 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevDsoundOptions',
+  'data': {
+    '*latency': 'int' } }
+

Quoted 8 lines

qapi/audio.json line 71

Quoted 6 lines

qapi/audio.json new file +6 -0

+##
+# @AudiodevOssPerDirectionOptions
+#
+# Options of the oss backend that are used for both playback and recording.
+#
+# @dev: #optional path of the oss device

Who picks the default, QEMU or OSS?

Quoted 8 lines

It defaults to "/dev/dsp".

Quoted 2 lines

For ALSA, you documented @dev to be "the name", here it's "path". Intentional difference?

Yes. For oss you have to provide a filesystem path of the audio device (like /dev/dsp), for alsa you provide an alsa specific name, like "default", "hw:1,0", "hdmi:CARD=NVidia,DEV=1", whatever.

I don't like calling a filename a path, even though we already do in many places. Let's do:

# @dev: #optional file name of the oss device (default '/dev/dsp')

Quoted 29 lines

Quoted 11 lines

qapi/audio.json line 80

Quoted 9 lines

qapi/audio.json new file +9 -0

+#
+# @try-poll: #optional attempt to use poll mode
+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssPerDirectionOptions',
+  'data': {
+    '*dev':      'str',
+    '*try-poll': 'bool' } }

Same as for ALSA. Keeping them separate is fine with me.

This is the same as alsa's try-poll. They're separate because only these two backend use this setting.

Quoted 14 lines

qapi/audio.json line 91

Quoted 12 lines

qapi/audio.json new file +11 -0

+
+##
+# @AudiodevOssOptions
+#
+# Options of the oss audio backend.
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @mmap: #optional try using memory mapped access

What happens when the attempt fails?

Quoted 3 lines

It should fall back to non-mmapped access (should because when I checked, it was buggy, at least on linux with alsa emulated oss on pulseaudio alsa device...)

Okay, assuming that's the user interface we want.

Quoted 12 lines

Quoted 1 line

Should this be called try-mmap?

Agree.

Quoted 8 lines

Quoted 6 lines

qapi/audio.json new file +4 -0

+#
+# @exclusive: #optional open device in exclusive mode (vmix won't work)
+#
+# @dsp-policy: #optional set the timing policy of the device, -1

to use fragment
+# mode (option ignored on some platforms)

What are the possible values besides -1?

Quoted 2 lines

It should be a number between 0 and 10 (according to this page:
http://manuals.opensound.com/developer/SNDCTL_DSP_POLICY.html)

Please cover that in your comment.

Quoted 24 lines

Quoted 23 lines

Quoted 11 lines

qapi/audio.json new file +11 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevOssOptions',
+  'data': {
+    'in':          'AudiodevOssPerDirectionOptions',
+    'out':         'AudiodevOssPerDirectionOptions',
+    '*mmap':       'bool',
+    '*exclusive':  'bool',
+    '*dsp-policy': 'int' } }
+

qapi/audio.json line 117

Quoted 10 lines

qapi/audio.json new file +10 -0

+##
+# @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?

Quoted 1 line

PA

Is there a way to explicitly ask for the PA default? Something like source=default?

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +15 -0

+#
+# 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

Quoted 8 lines

qapi/audio.json line 138

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevWavOptions',
+  'data': {
+    '*path': 'str' } }

Who picks the default?

Quoted 1 line

It defaults to "qemu.wav"

Make it

# @path: #optional path of the wav file to record (default 'qemu.wav')

Quoted 47 lines

Quoted 46 lines

Quoted 36 lines

qapi/audio.json new file +35 -0

+
+
+##
+# @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.

qapi/audio.json line 181

Quoted 8 lines

qapi/audio.json new file +8 -0

+#
+# @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?

Quoted 1 line

No.

If @fixed-settings, are the other three all required? If not, what are their defaults?

Quoted 54 lines

Quoted 5 lines

qapi/audio.json line 183

Quoted 2 lines

qapi/audio.json new file +2 -0

+#
+# @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.)

Quoted 36 lines

Quoted 36 lines

qapi/audio.json new file +35 -0

+#
+# @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' } }
+
+##
+# @Audiodev
+#
+# Captures the configuration of an audio backend.
+#
+# @id: identifier of the backend
+#
+# @in: options of the capture stream
+#
+# @out: options of the playback stream
+#
+# @timer-period: #optional timer period (in microseconds, 0: use lowest
+#                possible)
+#
+# @opts: audio backend specific options
+#
+# Since: 2.4
+##
+{ 'struct': 'Audiodev',
+  'data': {

Quoted 8 lines

qapi/audio.json line 223

Quoted 5 lines

qapi/audio.json new file +5 -0

+    '*id':           'str',
+    'in':            'AudiodevPerDirectionOptions',
+    'out':           'AudiodevPerDirectionOptions',
+    '*timer-period': 'int',
+    'opts':          'AudiodevBackendOptions' } }

Have you considered making this a flat union, similar ro BlockdevOptions?

Quoted 2 lines

Not really. If you qapi masters out there think it's better, then I will convert it.

Related: discussion about flattening in review of PATCH 2.

Quoted 2 lines

Quoted 2 lines

Don't get deceived by the number of my questions, this is solid work.

2015-06-17 13:48 keltezéssel, Markus Armbruster írta:

Quoted 8 lines

Quoted 6 lines · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

Quoted 5 lines

Copying Eric for additional QAPI schema expertise.

My questions inline, pretty sure they show my ignorance.

"Kővágó, Zoltán" <[email protected]> writes:

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Quoted 4 lines

qapi/audio.json new file +4 -0

+##
+# @AudiodevPaOptions
+#
+# Options of the pa (PulseAudio) audio backend.

Quoted 8 lines

qapi/audio.json line 117

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# @server: #optional PulseAudio server address
+#
+# @sink: #optional sink device name
+#
+# @source: #optional source device name

Who picks the defaults, QEMU or PA?

PA

Quoted 2 lines

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.)

Quoted 77 lines

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +15 -0

+#
+# 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

Quoted 8 lines

qapi/audio.json line 138

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevWavOptions',
+  'data': {
+    '*path': 'str' } }

Who picks the default?

Quoted 1 line

It defaults to "qemu.wav"

Make it

# @path: #optional path of the wav file to record (default 'qemu.wav')

Quoted 47 lines

Quoted 46 lines

Quoted 36 lines

qapi/audio.json new file +35 -0

+
+
+##
+# @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.

qapi/audio.json line 181

Quoted 8 lines

qapi/audio.json new file +8 -0

+#
+# @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?

Quoted 4 lines

Quoted 1 line

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.

I
guess I should also document it...

Quoted 24 lines

Quoted 24 lines

Quoted 5 lines

qapi/audio.json line 183

Quoted 2 lines

qapi/audio.json new file +2 -0

+#
+# @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.)

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +14 -0

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

Quoted 9 lines · Kővágó Zoltán

2015-06-17 13:48 keltezéssel, Markus Armbruster írta:

Quoted 8 lines

Quoted 6 lines · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

Quoted 5 lines

Copying Eric for additional QAPI schema expertise.

My questions inline, pretty sure they show my ignorance.

"Kővágó, Zoltán" <[email protected]> writes:

[...]

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

Quoted 4 lines

Quoted 4 lines

qapi/audio.json new file +4 -0

+##
+# @AudiodevPaOptions
+#
+# Options of the pa (PulseAudio) audio backend.

Quoted 8 lines

qapi/audio.json line 117

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# @server: #optional PulseAudio server address
+#
+# @sink: #optional sink device name
+#
+# @source: #optional source device name

Who picks the defaults, QEMU or PA?

PA

Quoted 7 lines

Quoted 2 lines

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 :)

Quoted 77 lines

Quoted 77 lines

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +15 -0

+#
+# 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

Quoted 8 lines

qapi/audio.json line 138

Quoted 6 lines

qapi/audio.json new file +6 -0

+#
+# Since: 2.4
+##
+{ 'struct': 'AudiodevWavOptions',
+  'data': {
+    '*path': 'str' } }

Who picks the default?

Quoted 1 line

It defaults to "qemu.wav"

Make it

# @path: #optional path of the wav file to record (default 'qemu.wav')

Quoted 47 lines

Quoted 46 lines

Quoted 36 lines

qapi/audio.json new file +35 -0

+
+
+##
+# @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.

qapi/audio.json line 181

Quoted 8 lines

qapi/audio.json new file +8 -0

+#
+# @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?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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?

Quoted 3 lines

I
guess I should also document it...

Yes, please.

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 5 lines

qapi/audio.json line 183

Quoted 2 lines

qapi/audio.json new file +2 -0

+#
+# @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.)

Quoted 15 lines

Quoted 15 lines

qapi/audio.json new file +14 -0

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

[...]

2015-06-17 15:37 keltezéssel, Markus Armbruster írta:

Quoted 6 lines

Quoted 4 lines · Kővágó Zoltán

2015-06-17 13:48 keltezéssel, Markus Armbruster írta:

Quoted 3 lines

Quoted 1 line · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

Quoted 52 lines

Quoted 45 lines

Quoted 45 lines

Quoted 45 lines

Quoted 44 lines

Quoted 34 lines

(quoted diff) +33 -0

+##
+# @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.

Review

Quoted 8 lines

(quoted diff) +8 -0

+#
+# @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?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 2 lines

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.

Quoted 7 lines · Kővágó Zoltán

2015-06-17 15:37 keltezéssel, Markus Armbruster írta:

Quoted 6 lines

Quoted 4 lines · Kővágó Zoltán

2015-06-17 13:48 keltezéssel, Markus Armbruster írta:

Quoted 3 lines

Quoted 1 line · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

[...]

Quoted 66 lines

Quoted 52 lines

Quoted 45 lines

Quoted 45 lines

Quoted 45 lines

Quoted 44 lines

Quoted 34 lines

(quoted diff) +33 -0

+##
+# @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.

Review

Quoted 8 lines

(quoted diff) +8 -0

+#
+# @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?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 2 lines

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

Quoted 8 lines

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.

So user typically specifies either fixed-settings=off, or any combination of the other three (including none of them). Correct?

We could reject the non-sensical combination of fixed-settings=off plus any of the other three instead of silently ignoring their values. Matter of taste, your choice.

Whatever you do, make sure to document how these four work together.

Thank you for educating me so patiently.

2015-06-17 18:06 keltezéssel, Markus Armbruster írta:

Quoted 88 lines

Quoted 7 lines · Kővágó Zoltán

2015-06-17 15:37 keltezéssel, Markus Armbruster írta:

Quoted 6 lines

Quoted 4 lines · Kővágó Zoltán

2015-06-17 13:48 keltezéssel, Markus Armbruster írta:

Quoted 3 lines

Quoted 1 line · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

[...]

Quoted 66 lines

Quoted 52 lines

Quoted 45 lines

Quoted 45 lines

Quoted 45 lines

Quoted 44 lines

Quoted 34 lines

(quoted diff) +33 -0

+##
+# @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.

Review

Quoted 8 lines

(quoted diff) +8 -0

+#
+# @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?

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 8 lines

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.

Quoted 2 lines

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

Quoted 8 lines

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.

So user typically specifies either fixed-settings=off, or any combination of the other three (including none of them). Correct?

Quoted 7 lines

We could reject the non-sensical combination of fixed-settings=off plus any of the other three instead of silently ignoring their values. Matter of taste, your choice.

Whatever you do, make sure to document how these four work together.

Thank you for educating me so patiently.

The audio backend currently works like that you can pass any non-sensical values to it, like negative frequency, or 'kdp' count of channels, it will silently fallback to some default value, or just fail, but qemu will continue to run. We can make the new config more strict (and we should, I think), so if you have any idea where should we be more strict (without creating a backward compatibility headache), don't hesitate to point it out.

Quoted 12 lines · Kővágó Zoltán

2015-06-17 09:46 keltezéssel, Markus Armbruster írta:

Quoted 11 lines

Copying Eric for additional QAPI schema expertise.

Quoted 7 lines

qapi/audio.json new file +7 -0

+{ 'struct': 'Audiodev',
+  'data': {
+    '*id':           'str',
+    'in':            'AudiodevPerDirectionOptions',
+    'out':           'AudiodevPerDirectionOptions',
+    '*timer-period': 'int',
+    'opts':          'AudiodevBackendOptions' } }

Quoted 5 lines

Quoted 2 lines

Have you considered making this a flat union, similar ro BlockdevOptions?

Not really. If you qapi masters out there think it's better, then I will convert it.

Related: discussion about flattening in review of PATCH 2.

The current OptsVisitor flattens the whole structure, if there are same named fields under different paths (like in' and out' in Audiodev'), the current visitor can't cope with them (for example setting frequency=44100' will set the in's frequency to 44100 and leave out's frequency unspecified).

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example).
2) Optionally user can specify the path in the hierarchy. Names are separated by a dot (e.g. in.frequency', foo.bar.something', etc). The user need not specify the whole path, only the last few components (i.e. bar.something' is equivalent to foo.bar.something' if only foo' has a bar' field). This way
1) is just a special case of this when only the last component is specified.
3) In case of an ambiguity (e.g frequency=44100,in.frequency=8000') the longest matching (the most specific) path wins (so in this example, in's frequency would become 8000, because in.frequency' is more specific that frequency', and out's frequency would become 44100, because only frequency' matches it).

Copying László because his fingerprints are on OptsVisitor.

Quoted 5 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named fields under different paths (like in' and out' in Audiodev'), the current visitor can't cope with them (for example setting frequency=44100' will set the in's frequency to 44100 and leave out's frequency unspecified).

Quoted 13 lines

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example).
2) Optionally user can specify the path in the hierarchy. Names are separated by a dot (e.g. in.frequency', foo.bar.something', etc). The user need not specify the whole path, only the last few components (i.e. bar.something' is equivalent to foo.bar.something' if only foo' has a bar' field). This way
1) is just a special case of this when only the last component is specified.
3) In case of an ambiguity (e.g frequency=44100,in.frequency=8000') the longest matching (the most specific) path wins (so in this example, in's frequency would become 8000, because in.frequency' is more specific that frequency', and out's frequency would become 44100, because only frequency' matches it).

Can you explain why the complexity is needed, i.e. why we can't just require full paths always?

Quoted 23 lines

Copying László because his fingerprints are on OptsVisitor.

Quoted 5 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named fields under different paths (like in' and out' in Audiodev'), the current visitor can't cope with them (for example setting frequency=44100' will set the in's frequency to 44100 and leave out's frequency unspecified).

Quoted 13 lines

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example).
2) Optionally user can specify the path in the hierarchy. Names are separated by a dot (e.g. in.frequency', foo.bar.something', etc). The user need not specify the whole path, only the last few components (i.e. bar.something' is equivalent to foo.bar.something' if only foo' has a bar' field). This way
1) is just a special case of this when only the last component is specified.
3) In case of an ambiguity (e.g frequency=44100,in.frequency=8000') the longest matching (the most specific) path wins (so in this example, in's frequency would become 8000, because in.frequency' is more specific that frequency', and out's frequency would become 44100, because only frequency' matches it).

Quoted 2 lines

Can you explain why the complexity is needed, i.e. why we can't just require full paths always?

Keeping the short names is required for -netdev backward compatibility.

Restricting to short or full (i.e. something= or foo.bar.something=, but disallow bar.something=) should not be a problem. I'm not sure this simplifies things much though. We have to build the full path anyway, and I think bar.something= is just a convenient thing we get almost for free ...

2015-06-17 10:41 keltezéssel, Gerd Hoffmann írta:

Quoted 38 lines · Markus Armbruster

Quoted 34 lines

Copying László because his fingerprints are on OptsVisitor.

Quoted 21 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named
fields under different paths (like in' and out' in `Audiodev'), the current
visitor can't cope with them (for example setting frequency=44100' will set the
in's frequency to 44100 and leave out's frequency unspecified).

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example).
2) Optionally user can specify the path in the hierarchy. Names are separated by
a dot (e.g. in.frequency', foo.bar.something', etc). The user need not specify the whole path, only the last few components
(i.e. `bar.something' is
equivalent to foo.bar.something' if only foo' has a `bar'
field). This way
1) is just a special case of this when only the last component
is specified.

Quoted 8 lines

3) In case of an ambiguity (e.g frequency=44100,in.frequency=8000') the longest matching (the most specific) path wins (so in this example,
in's frequency
would become 8000, because `in.frequency' is more specific
that `frequency',
and out's frequency would become 44100, because only
frequency' matches it).

Quoted 2 lines

Can you explain why the complexity is needed, i.e. why we can't just require full paths always?

Quoted 7 lines

Keeping the short names is required for -netdev backward compatibility.

Restricting to short or full (i.e. something= or foo.bar.something=, but disallow bar.something=) should not be a problem. I'm not sure this simplifies things much though. We have to build the full path anyway, and I think bar.something= is just a convenient thing we get almost for free ...

With the current implementation you can specify (see my previous patch) in.try-poll=off in case of alsa. If we would need full paths, it would look like opts.data.in.try-poll=off, which is probably not something we want.

Quoted 10 lines

Quoted 2 lines

Can you explain why the complexity is needed, i.e. why we can't just require full paths always?

Keeping the short names is required for -netdev backward compatibility.

Restricting to short or full (i.e. something= or foo.bar.something=, but disallow bar.something=) should not be a problem. I'm not sure this simplifies things much though. We have to build the full path anyway, and I think bar.something= is just a convenient thing we get almost for free ...

With the current implementation you can specify (see my previous patch) in.try-poll=off in case of alsa. If we would need full paths, it would look like opts.data.in.try-poll=off, which is probably not something we want.

Quoted 12 lines · Markus Armbruster

Copying László because his fingerprints are on OptsVisitor.

Quoted 8 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named
fields under different paths (like in' and out' in `Audiodev'), the current
visitor can't cope with them (for example setting frequency=44100' will set the
in's frequency to 44100 and leave out's frequency unspecified).

Quoted 4 lines

Quoted 4 lines

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example).

Quoted 9 lines

Quoted 9 lines

2) Optionally user can specify the path in the hierarchy. Names are separated by
a dot (e.g. in.frequency', foo.bar.something', etc). The user need not specify the whole path, only the last few components
(i.e. `bar.something' is
equivalent to foo.bar.something' if only foo' has a `bar'
field). This way
1) is just a special case of this when only the last component
is specified.

Quoted 8 lines

Quoted 8 lines

3) In case of an ambiguity (e.g frequency=44100,in.frequency=8000') the longest matching (the most specific) path wins (so in this example,
in's frequency
would become 8000, because `in.frequency' is more specific that
frequency',
and out's frequency would become 44100, because only
frequency' matches it).

Quoted 2 lines

Can you explain why the complexity is needed, i.e. why we can't just require full paths always?

Keeping the short names is required for -netdev backward compatibility.

Restricting to short or full (i.e. something= or foo.bar.something=, but disallow bar.something=) should not be a problem. I'm not sure this simplifies things much though. We have to build the full path anyway, and I think bar.something= is just a convenient thing we get almost for free ...

Copying Kevin because similar issues exist in the block layer.

Quoted 13 lines · Gerd Hoffmann

Quoted 12 lines · Markus Armbruster

Copying László because his fingerprints are on OptsVisitor.

Quoted 8 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named
fields under different paths (like in' and out' in `Audiodev'), the current
visitor can't cope with them (for example setting frequency=44100' will set the
in's frequency to 44100 and leave out's frequency unspecified).

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's frequency to 44100 in the above example).

What if they have different types?

What if one of them can't take the value?

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

2) Optionally user can specify the path in the hierarchy. Names are separated by
a dot (e.g. in.frequency', foo.bar.something', etc). The user need not specify the whole path, only the last few components
(i.e. `bar.something' is
equivalent to foo.bar.something' if only foo' has a `bar'
field). This way
1) is just a special case of this when only the last component
is specified.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

3) In case of an ambiguity (e.g frequency=44100,in.frequency=8000') the longest
matching (the most specific) path wins (so in this example,
in's frequency
would become 8000, because `in.frequency' is more specific that
frequency',
and out's frequency would become 44100, because only
frequency' matches it).

The current rule for multiple assignments is "last one wins". E.g. in

-drive if=none,file=tmp.img,file=tmp.qcow2

file=tmp.qcow2 wins.

If I understand correctly, this patch amends the rule to "last most specific one wins". Correct?

Quoted 4 lines

Quoted 2 lines

Can you explain why the complexity is needed, i.e. why we can't just require full paths always?

Keeping the short names is required for -netdev backward compatibility.

I suspect mostly because NetLegacy and Netdev aren't flat unions. Could be self-inflicted pain.

What about flattening them instead? Assuming that's possible; I'd have to try.

Quoted 5 lines

Restricting to short or full (i.e. something= or foo.bar.something=, but disallow bar.something=) should not be a problem. I'm not sure this simplifies things much though. We have to build the full path anyway, and I think bar.something= is just a convenient thing we get almost for free ...

We've been bitten by convenience features before. Adding them tends to be cheap, but maintaining compatibility can become a terrible headache.

2015-06-17 13:18 keltezéssel, Markus Armbruster írta:

Quoted 23 lines

Copying Kevin because similar issues exist in the block layer.

Quoted 18 lines · Gerd Hoffmann

Quoted 17 lines · Markus Armbruster

Copying László because his fingerprints are on OptsVisitor.

Quoted 13 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named
fields under different paths (like in' and out' in `Audiodev'), the current
visitor can't cope with them (for example setting frequency=44100' will set the
in's frequency to 44100 and leave out's frequency unspecified).

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's
frequency to
44100 in the above example).

Quoted 3 lines

What if they have different types?

What if one of them can't take the value?

Currently it will error out, requiring the user to be more explicit. Probably not the best solution, but I can't really think of a better solution. (If we would ignore invalid values that would be very confusing imho.)

Quoted 24 lines · Kővágó Zoltán

2015-06-17 13:18 keltezéssel, Markus Armbruster írta:

Quoted 23 lines

Copying Kevin because similar issues exist in the block layer.

Quoted 18 lines · Gerd Hoffmann

Quoted 17 lines · Markus Armbruster

Copying László because his fingerprints are on OptsVisitor.

Quoted 13 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named
fields under different paths (like in' and out' in `Audiodev'), the current
visitor can't cope with them (for example setting frequency=44100' will set the
in's frequency to 44100 and leave out's frequency unspecified).

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's
frequency to
44100 in the above example).

Quoted 8 lines

Quoted 3 lines

What if they have different types?

What if one of them can't take the value?

Currently it will error out, requiring the user to be more explicit. Probably not the best solution, but I can't really think of a better solution. (If we would ignore invalid values that would be very confusing imho.)

Yes, we clearly don't want foo=0 to set a.foo and b.foo, but foo=x set only a.foo, because the former can take any string, but the latter only a number.

Can we require the LHS to be unambiguous?

2015-06-17 15:41 keltezéssel, Markus Armbruster írta:

Quoted 35 lines

Quoted 32 lines · Kővágó Zoltán

2015-06-17 13:18 keltezéssel, Markus Armbruster írta:

Quoted 23 lines

Copying Kevin because similar issues exist in the block layer.

Quoted 13 lines · Gerd Hoffmann

Quoted 12 lines · Markus Armbruster

Copying László because his fingerprints are on OptsVisitor.

Quoted 8 lines · Zoltán

The current OptsVisitor flattens the whole structure, if there are same named
fields under different paths (like in' and out' in `Audiodev'), the current
visitor can't cope with them (for example setting frequency=44100' will set the
in's frequency to 44100 and leave out's frequency unspecified).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

This patch fixes it, by the following changes:
1) Specifying just the field name will apply to all fields that has the specified name (this means it would set both in's and out's
frequency to
44100 in the above example).

Quoted 3 lines

What if they have different types?

What if one of them can't take the value?

Currently it will error out, requiring the user to be more explicit. Probably not the best solution, but I can't really think of a better solution. (If we would ignore invalid values that would be very confusing imho.)

Quoted 5 lines

Yes, we clearly don't want foo=0 to set a.foo and b.foo, but foo=x set only a.foo, because the former can take any string, but the latter only a number.

Can we require the LHS to be unambiguous?

Originally I designed it that way because it allows you to specify frequency=44100 and set both in.frequency and out.frequency.

But this
could also be the convenience feature that we not really need. I don't see any other downside of making it unambigous.

This allows to print options in a format that the user would actually write it on the command line (foo=bar,baz=asd,etc=def), without prepending a spurious comma at the beginning of the list.

Only block.c depended on the old behavior, but it was also updated.

Signed-off-by Kővágó, Zoltán <[email protected]>

---
 block.c            | 2 +-
 util/qemu-option.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

block.c +1 -1

index dd4f58d..c5d456c 100644
@@ -3823,7 +3823,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
     }
 
     if (!quiet) {
-        printf("Formatting '%s', fmt=%s", filename, fmt);
+        printf("Formatting '%s', fmt=%s ", filename, fmt);
         qemu_opts_print(opts, " ");
         puts("");
     }

util/qemu-option.c +3 -1

index 840f5f7..b347d92 100644
@@ -728,14 +728,16 @@ void qemu_opts_del(QemuOpts *opts)
     g_free(opts);
 }
 
-void qemu_opts_print(QemuOpts *opts, const char *sep)
+void qemu_opts_print(QemuOpts *opts, const char *d_sep)
 {
     QemuOpt *opt;
     QemuOptDesc *desc = opts->list->desc;
+    const char *sep = "";
 
     if (desc[0].name == NULL) {
         QTAILQ_FOREACH(opt, &opts->head, next) {
             printf("%s%s=\"%s\"", sep, opt->name, opt->str);
+            sep = d_sep;
         }
         return;
     }

This allows to print options in a format that the user would actually write it on the command line (foo=bar,baz=asd,etc=def), without prepending a spurious comma at the beginning of the list.

Only block.c depended on the old behavior, but it was also updated.

Signed-off-by Kővágó, Zoltán <[email protected]>

Simple visitor that recursively allocates structures with only optional variables. Unions are initialized to the first type specified. Other non optional types are not supported.

Quoted 3 lines · Zoltán

Simple visitor that recursively allocates structures with only optional variables. Unions are initialized to the first type specified. Other non optional types are not supported.

Sounds dubious :)

Can you explain why it's useful? I guess later patches provide an example.

2015-06-17 09:56 keltezéssel, Markus Armbruster írta:

Quoted 6 lines

Quoted 3 lines · Zoltán

Simple visitor that recursively allocates structures with only optional variables. Unions are initialized to the first type specified. Other non optional types are not supported.

Quoted 4 lines

Sounds dubious :)

Can you explain why it's useful? I guess later patches provide an example.

Oh, crap, ignore this commit. Just realized that in the refactorings I did I removed all references to this visitor... so it's not needed.