"Kővágó Zoltán" <dirty.ice...@gmail.com> writes: > 2015-06-17 09:46 keltezéssel, Markus Armbruster írta: >> Copying Eric for additional QAPI schema expertise. >> >> My questions inline, pretty sure they show my ignorance. >> >> "Kővágó, Zoltán" <dirty.ice...@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...@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...@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? > > 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') >> >>> +# >>> +# @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. >>> +# >>> +# 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. > > I don't know, so far nobody rambled about my naming... > >> >>> + >>> +## >>> +# @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. I'm just poking fun at our inability to pick a time unit for QAPI/QMP. No objection to your use of microseconds. >>> +# >>> +# 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? > > It defaults to "/dev/dsp". > >> 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') >>> +# >>> +# @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. >> >>> + >>> +## >>> +# @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...) Okay, assuming that's the user interface we want. >> Should this be called try-mmap? > Agree. > >>> +# >>> +# @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) Please cover that in your comment. >>> +# >>> +# 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? > > PA Is there a way to explicitly ask for the PA default? Something like source=default? >>> +# >>> +# Since: 2.4 >>> +## >>> +{ 'struct': 'AudiodevPaOptions', >>> + 'data': { >>> + '*server': 'str', >>> + '*sink': 'str', >>> + '*source': 'str' } } >>> + >>> +## >>> +# @AudiodevWavOptions >>> +# >>> +# Options of the wav audio backend. >>> +# >>> +# @path: #optional path of the wav file to record >>> +# >>> +# Since: 2.4 >>> +## >>> +{ 'struct': 'AudiodevWavOptions', >>> + 'data': { >>> + '*path': 'str' } } >> >> Who picks the default? > > It defaults to "qemu.wav" Make it # @path: #optional path of the wav file to record (default 'qemu.wav') >>> + >>> + >>> +## >>> +# @AudiodevBackendOptions >>> +# >>> +# A discriminated record of audio backends. >>> +# >>> +# Since: 2.4 >>> +## >>> +{ 'union': 'AudiodevBackendOptions', >>> + 'data': { >>> + 'none': 'AudiodevNoOptions', >>> + 'alsa': 'AudiodevAlsaOptions', >>> + 'coreaudio': 'AudiodevNoOptions', >>> + 'dsound': 'AudiodevDsoundOptions', >>> + 'oss': 'AudiodevOssOptions', >>> + 'pa': 'AudiodevPaOptions', >>> + 'sdl': 'AudiodevNoOptions', >>> + 'spice': 'AudiodevNoOptions', >>> + 'wav': 'AudiodevWavOptions' } } >>> + >>> +## >>> +# @AudioFormat >>> +# >>> +# An enumeration of possible audio formats. >>> +# >>> +# Since: 2.4 >>> +## >>> +{ 'enum': 'AudioFormat', >>> + 'data': [ 'u8', 's8', 'u16', 's16', 'u32', 's32' ] } >>> + >>> +## >>> +# @AudiodevPerDirectionOptions >>> +# >>> +# General audio backend options that are used for both playback >>> and recording. >>> +# >>> +# @fixed-settings: #optional use fixed settings for host DAC/ADC >>> +# >>> +# @frequency: #optional frequency to use when using fixed settings >>> +# >>> +# @channels: #optional number of channels when using fixed settings >>> +# >>> +# @format: #optional sample format to use when using fixed settings >> >> Are these guys used when @fixed-settings is off? > > No. If @fixed-settings, are the other three all required? If not, what are their defaults? >>> +# >>> +# @buffer: #optional the buffer size (in microseconds) >> >> @buffer suggests this is a buffer, not a buffer length given as time >> span. @buffer-len? > > Ok. (It used to be called buffer-usecs before I changed everything to > microseconds.) > >> >>> +# >>> +# @buffer-count: #optional number of buffers >>> +# >>> +# Since: 2.4 >>> +## >>> +{ 'struct': 'AudiodevPerDirectionOptions', >>> + 'data': { >>> + '*fixed-settings': 'bool', >>> + '*frequency': 'int', >>> + '*channels': 'int', >>> + '*voices': 'int', >>> + '*format': 'AudioFormat', >>> + '*buffer': 'int', >>> + '*buffer-count': 'int' } } >>> + >>> +## >>> +# @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? > > 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. >> Don't get deceived by the number of my questions, this is solid work. >>