From: Markus Armbruster <armbru@redhat.com>
To: "Kővágó, Zoltán" <dirty.ice.hu@gmail.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>,
qemu-devel@nongnu.org, Michael Roth <mdroth@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH v2 1/6] qapi: qapi for audio backends
Date: Wed, 17 Jun 2015 09:46:26 +0200 [thread overview]
Message-ID: <87si9qg4kt.fsf@blackfin.pond.sub.org> (raw)
In-Reply-To: <d662ea2787c63d291afd01da86c59f662ebee769.1434458391.git.DirtY.iCE.hu@gmail.com> ("Kővágó, Zoltán"'s message of "Tue, 16 Jun 2015 14:49:04 +0200")
Copying Eric for additional QAPI schema expertise.
My questions inline, pretty sure they show my ignorance.
"Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes:
> 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 <DirtY.iCE.hu@gmail.com>
>
> ---
>
> 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
>
> diff --git a/Makefile b/Makefile
> index 3f97904..ac566fa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 106008c..e751ea3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -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' }
>
> diff --git a/qapi/audio.json b/qapi/audio.json
> new file mode 100644
> index 0000000..2851689
> --- /dev/null
> +++ b/qapi/audio.json
> @@ -0,0 +1,223 @@
> +# -*- mode: python -*-
> +#
> +# Copyright (C) 2015 Zoltán Kővágó <DirtY.iCE.hu@gmail.com>
> +#
> +# 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': { } }
> +
> +##
> +# @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?
> +#
> +# @try-poll: #optional attempt to use poll mode
What happens when the attempt fails?
> +#
> +# 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
> +#
> +# @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.
> +
> +##
> +# @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.
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevDsoundOptions',
> + 'data': {
> + '*latency': 'int' } }
> +
> +##
> +# @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?
> +#
> +# @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.
> +
> +##
> +# @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?
> +#
> +# @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?
> +#
> +# 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.
> +#
> +# @server: #optional PulseAudio server address
> +#
> +# @sink: #optional sink device name
> +#
> +# @source: #optional source device name
Who picks the defaults, QEMU or PA?
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevPaOptions',
> + 'data': {
> + '*server': 'str',
> + '*sink': 'str',
> + '*source': 'str' } }
> +
> +##
> +# @AudiodevWavOptions
> +#
> +# Options of the wav audio backend.
> +#
> +# @path: #optional path of the wav file to record
> +#
> +# Since: 2.4
> +##
> +{ 'struct': 'AudiodevWavOptions',
> + 'data': {
> + '*path': 'str' } }
Who picks the default?
> +
> +
> +##
> +# @AudiodevBackendOptions
> +#
> +# A discriminated record of audio backends.
> +#
> +# Since: 2.4
> +##
> +{ 'union': 'AudiodevBackendOptions',
> + 'data': {
> + 'none': 'AudiodevNoOptions',
> + 'alsa': 'AudiodevAlsaOptions',
> + 'coreaudio': 'AudiodevNoOptions',
> + 'dsound': 'AudiodevDsoundOptions',
> + 'oss': 'AudiodevOssOptions',
> + 'pa': 'AudiodevPaOptions',
> + 'sdl': 'AudiodevNoOptions',
> + 'spice': 'AudiodevNoOptions',
> + 'wav': 'AudiodevWavOptions' } }
> +
> +##
> +# @AudioFormat
> +#
> +# An enumeration of possible audio formats.
> +#
> +# Since: 2.4
> +##
> +{ 'enum': 'AudioFormat',
> + 'data': [ 'u8', 's8', 'u16', 's16', 'u32', 's32' ] }
> +
> +##
> +# @AudiodevPerDirectionOptions
> +#
> +# General audio backend options that are used for both playback and recording.
> +#
> +# @fixed-settings: #optional use fixed settings for host DAC/ADC
> +#
> +# @frequency: #optional frequency to use when using fixed settings
> +#
> +# @channels: #optional number of channels when using fixed settings
> +#
> +# @format: #optional sample format to use when using fixed settings
Are these guys used when @fixed-settings is off?
> +#
> +# @buffer: #optional the buffer size (in microseconds)
@buffer suggests this is a buffer, not a buffer length given as time
span. @buffer-len?
> +#
> +# @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': {
> + '*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.
next prev parent reply other threads:[~2015-06-17 7:46 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-16 12:49 [Qemu-devel] [PATCH v2 0/6] -audiodev option Kővágó, Zoltán
2015-06-16 12:49 ` [Qemu-devel] [PATCH v2 1/6] qapi: qapi for audio backends Kővágó, Zoltán
2015-06-17 7:46 ` Markus Armbruster [this message]
2015-06-17 10:54 ` Kővágó Zoltán
2015-06-17 11:48 ` Markus Armbruster
2015-06-17 12:07 ` Kővágó Zoltán
2015-06-17 13:37 ` Markus Armbruster
2015-06-17 13:53 ` Kővágó Zoltán
2015-06-17 16:06 ` Markus Armbruster
2015-06-18 0:21 ` Kővágó Zoltán
2015-06-18 8:51 ` Markus Armbruster
2015-06-17 15:50 ` Eric Blake
2015-06-16 12:49 ` [Qemu-devel] [PATCH v2 2/6] qapi: support nested structs in OptsVisitor Kővágó, Zoltán
2015-06-17 7:50 ` Markus Armbruster
2015-06-17 8:41 ` Gerd Hoffmann
2015-06-17 11:01 ` Kővágó Zoltán
2015-06-17 11:50 ` Markus Armbruster
2015-06-17 15:47 ` Eric Blake
2015-06-17 11:18 ` Markus Armbruster
2015-06-17 12:11 ` Kővágó Zoltán
2015-06-17 13:41 ` Markus Armbruster
2015-06-17 14:02 ` Kővágó Zoltán
2015-06-17 16:10 ` Markus Armbruster
2015-06-16 12:49 ` [Qemu-devel] [PATCH v2 3/6] opts: do not print separator before first item in qemu_opts_print Kővágó, Zoltán
2015-06-17 7:53 ` Markus Armbruster
2015-06-17 9:02 ` Kevin Wolf
2015-06-16 12:49 ` [Qemu-devel] [PATCH v2 4/6] qapi: AllocVisitor Kővágó, Zoltán
2015-06-17 7:56 ` Markus Armbruster
2015-06-17 12:01 ` Kővágó Zoltán
2015-06-17 13:42 ` Markus Armbruster
2015-06-16 12:49 ` [Qemu-devel] [PATCH v2 5/6] audio: use qapi AudioFormat instead of audfmt_e Kővágó, Zoltán
2015-06-17 8:01 ` Markus Armbruster
2015-06-17 11:05 ` Kővágó Zoltán
2015-06-17 11:51 ` Markus Armbruster
2015-06-17 16:01 ` Eric Blake
2015-06-16 12:49 ` [Qemu-devel] [PATCH v2 6/6] audio: -audiodev command line option Kővágó, Zoltán
2015-06-17 8:13 ` Markus Armbruster
2015-06-17 11:18 ` Kővágó Zoltán
2015-06-17 12:27 ` Markus Armbruster
2015-06-17 13:25 ` Kővágó Zoltán
2015-06-17 16:13 ` Markus Armbruster
2015-06-18 6:54 ` Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87si9qg4kt.fsf@blackfin.pond.sub.org \
--to=armbru@redhat.com \
--cc=dirty.ice.hu@gmail.com \
--cc=kraxel@redhat.com \
--cc=mdroth@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.