From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36747) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0Emf-0004jY-1y for qemu-devel@nongnu.org; Wed, 03 Jun 2015 15:57:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0Emb-0007eI-Gt for qemu-devel@nongnu.org; Wed, 03 Jun 2015 15:57:04 -0400 Received: from mail-wg0-x231.google.com ([2a00:1450:400c:c00::231]:35832) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0Emb-0007du-7O for qemu-devel@nongnu.org; Wed, 03 Jun 2015 15:57:01 -0400 Received: by wgme6 with SMTP id e6so17947091wgm.2 for ; Wed, 03 Jun 2015 12:57:00 -0700 (PDT) From: "=?UTF-8?B?S8WRdsOhZ8OzIFpvbHTDoW4=?=" Message-ID: <556F5C16.5030308@gmail.com> Date: Wed, 03 Jun 2015 21:57:10 +0200 MIME-Version: 1.0 References: <65207bc462fbee8168ff5e54e3fe857d5cfabf84.1433349894.git.DirtY.iCE.hu@gmail.com> <556F52C6.6000705@redhat.com> In-Reply-To: <556F52C6.6000705@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [RFC PATCH] qapi for audio backends List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Gerd Hoffmann 2015-06-03 21:17 keltezéssel, Eric Blake írta: > On 06/03/2015 10:48 AM, Kővágó, Zoltán wrote: >> This is a proposal to add structures into qapi-schema.json to replace the >> existing configuration structures used by audio backends currently. I'm going to >> use it to implement a new way to specify audio backend options (an -audiodev >> command line option, instead of environment variables. This will also allow us >> to use multiple audio backends in one qemu instance), so the structure used here >> will be the basis of the command line syntax. >> >> This is currently more or less a direct translation of the current audio backend >> options. I've changed some names, trying to accomplish a more consistent naming >> scheme. I wouldn't be surprised if there were options that doesn't work if you >> set their value to anything other than the default (for example, log to monitor >> can crash qemu, QEMU_DSOUND_RESTOURE_RETRIES has a typo, so probably nobody used >> it, etc). I've also tried to reduce copy-paste, when the same set of options can >> be given to output and input (QEMU_*_DAC_* and QEMU_*_ADC_* options), also using >> in and out instead of ADC and DAC, as in the world of SPDIF and HDMI it's >> completely possible that your computer has nothing to do with analog converters. >> Plus a non technician user probably has no idea what ADC and DAC stands for. >> >> Any comment is appreciated. >> >> Signed-off-by: Kővágó, Zoltán >> --- >> qapi-schema.json | 330 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 330 insertions(+) >> >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 0662a9b..ff67d5a 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -3769,3 +3769,333 @@ >> # Since: 2.1 >> ## >> { 'command': 'rtc-reset-reinjection' } >> + >> +## >> +# @AudiodevNoneOptions >> +# >> +# The dummy audio backend has no options. >> +# >> +# Since: XXX > > It's okay to tentatively put 2.4 here, if you are aiming for 2.4. If > you think it will be a big enough project to miss the current release > window, put 2.5. > >> +## >> +{ 'struct': 'AudiodevNoneOptions', >> + 'data': { } } >> + >> +## >> +# @AudiodevAlsaPerDirectionOptions >> +# >> +# Options of the alsa backend that are used for both playback and recording. >> +# >> +# @dev: #optional the name of the alsa device to use. >> +# >> +# @period_size_usec: #optional the period size in microseconds. Must not be >> +# specified with @period_size_frames. >> +# >> +# @period_size_frames: #optional the period size in frames. Must not be >> +# specified with @period_size_usec. >> +# >> +# @buffer_size_usec: #optional the buffer size in microseconds. Must not be >> +# specified with @buffer_size_frames. >> +# >> +# @buffer_size_frames: #optional the buffer size in frames. Must not be >> +# specified with @buffer_size_usec. > > Can we name these with s/_/-/? We've documented that QMP prefers dash > unless there is compelling reason or consistency to worry about, and I > don't see the compelling reason here. There's no particular reason other than when I looked through qapi-schema.json I've mostly seen underscores. Will fix this. > >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevAlsaPerDirectionOptions', >> + 'data': { >> + '*dev': 'str', >> + '*period_size_usec': 'int', >> + '*period_size_frames': 'int', >> + '*buffer_size_usec': 'int', >> + '*buffer_size_frames': 'int' } } >> + >> +## >> +# @AudiodevAlsaOptions >> +# >> +# Options of the alsa audio backend. >> +# >> +# @in: #optional options of the capture stream. >> +# >> +# @out: #optional options of the playback stream. >> +# >> +# @threshold: #optional > > Document this. This (and some other option) currently only has the documentation "(undocumented)", but I will try to figure out what they do... > >> +# >> +# @verbose: #optional behave in a more verbose way >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevAlsaOptions', >> + 'data': { >> + '*in': 'AudiodevAlsaPerDirectionOptions', >> + '*out': 'AudiodevAlsaPerDirectionOptions', >> + '*threshold': 'int', >> + '*verbose': 'bool' } } >> + >> +## >> +# @AudiodevCoreaudioOptions >> +# >> +# Options of the coreaudio backend. >> +# >> +# @buffer_size: #optional size of the buffer in frames >> +# >> +# @buffer_count: #optional number of buffers > > Again, dashes would be nicer, if there is no compelling reason otherwise > (I'll quit repeating it). > >> +# >> +# Since: XXX > > (and I'll quit pointing out XXX in Since lines) > >> +## >> +{ 'struct': 'AudiodevCoreaudioOptions', >> + 'data': { >> + '*buffer_size': 'int', >> + '*buffer_count': 'int' } } >> + >> +## >> +# @AudiodevDsoundPerDirectionOptions >> +# >> +# Options of the dsound backend that are used for both playback and recording. >> +# >> +# @bufsize: #optional > > Document this > >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevDsoundPerDirectionOptions', >> + 'data' : { >> + '*bufsize': 'int' } } >> + >> +## >> +# @AudiodevDsoundOptions >> +# >> +# Options of the dsound audio backend. >> +# >> +# @in: #optional options of the capture stream. >> +# >> +# @out: #optional options of the playback stream. >> +# >> +# @lock_retries: #optional number of times to attempt locking the buffer >> +# >> +# @restore_retries: #optional number of times to attempt restoring the buffer >> +# >> +# @getstatus_retries: #optional number of times to attempt getting status of the > > Borders on being a long line (yes, it's exactly 80, but I tend to stick > to < 80) > >> +# buffer >> +# >> +# @set_primary: #optional set the parameters of primary buffer >> +# >> +# @latency_millis: #optional >> +# >> +# @primary_freq: #optional primary buffer frequency >> +# >> +# @primary_channels: #optional primary buffer number of channels >> +# >> +# @primary_format: #optional primary buffer format >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevDsoundOptions', >> + 'data': { >> + '*in': 'AudiodevDsoundPerDirectionOptions', >> + '*out': 'AudiodevDsoundPerDirectionOptions', >> + '*lock_retries': 'int', >> + '*restore_retries': 'int', >> + '*getstatus_retries': 'int', >> + '*set_primary': 'bool', >> + '*latency_millis': 'int', >> + '*primary_freq': 'int', >> + '*primary_channels': 'int', >> + '*primary_format': 'AudioFormat' } } >> + >> +## >> +# @AudiodevOssPerDirectionOptions >> +# >> +# Options of the oss backend that are used for both playback and recording. >> +# >> +# @dev: #optional path of the oss device >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevOssPerDirectionOptions', >> + 'data': { >> + '*dev': 'str' } } >> + >> +## >> +# @AudiodevOssOptions >> +# >> +# Options of the oss audio backend. >> +# >> +# @in: #optional options of the capture stream. >> +# >> +# @out: #optional options of the playback stream. >> +# >> +# @fragsize: #optional fragment size in bytes >> +# >> +# @frags: #optional number of fragments >> +# >> +# @mmap: #optional try using memory mapped access >> +# >> +# @exclusive: #optional open device in exclusive mode (vmix wont work) >> +# >> +# @dsp_policy: #optional set the timing policy of the device, -1 to use fragment >> +# mode (option ignored on some platforms) >> +# >> +# @debug: #optional turn on some debugging messages >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevOssOptions', >> + 'data': { >> + '*in': 'AudiodevOssPerDirectionOptions', >> + '*out': 'AudiodevOssPerDirectionOptions', >> + '*fragsize': 'int', >> + '*frags': 'int', >> + '*mmap': 'bool', >> + '*exclusive': 'bool', >> + '*dsp_policy': 'int', >> + '*debug': 'bool' } } >> + >> +## >> +# @AudiodevPaOptions >> +# >> +# Options of the pa audio backend. >> +# >> +# @samples: #optional buffer size in samples >> +# >> +# @server: #optional PulseAudio server address > > Worth mentioning that 'pa' == PulseAudio earlier in the docs? > >> +# >> +# @sink: #optional sink device name >> +# >> +# @source: #optional source device name >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevPaOptions', >> + 'data': { >> + '*samples': 'int', >> + '*server': 'str', >> + '*sink': 'str', >> + '*source': 'str' } } >> + >> +## >> +# @AudiodevSdlOptions >> +# >> +# Options of the sdl audio backend. (Note that most options are only changeable >> +# through SDL's environment variables). >> +# >> +# @samples: #optional size of SDL buffer in samples >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevSdlOptions', >> + 'data': { >> + '*samples': 'int' } } >> + >> +## >> +# @AudiodevSpiceOptions >> +# >> +# The spice audio backend has no options. >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevSpiceOptions', >> + 'data': { } } >> + >> +## >> +# @AudiodevWavOptions >> +# >> +# Options of the wav audio backend. >> +# >> +# @frequency: #optional frequency of the wav file >> +# >> +# @format: #optional sample format of the wav file >> +# >> +# @channels: #optional number of channels of the wav file >> +# >> +# @path: #optional path of the wav file to record. >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevWavOptions', >> + 'data': { >> + '*frequency': 'int', >> + '*format': 'AudioFormat', >> + '*channels': 'int', >> + '*path': 'str' } } > > Inconsistent indentation. > >> + >> + >> +## >> +# @AudiodevBackendOptions >> +# >> +# A discriminated record of audio backends. >> +# >> +# Since: XXX >> +## >> +{ 'union': 'AudiodevBackendOptions', >> + 'data': { >> + 'none': 'AudiodevNoneOptions', >> + 'alsa': 'AudiodevAlsaOptions', >> + 'coreaudio': 'AudiodevCoreaudioOptions', >> + 'dsound': 'AudiodevDsoundOptions', >> + 'oss': 'AudiodevOssOptions', >> + 'pa': 'AudiodevPaOptions', >> + 'sdl': 'AudiodevSdlOptions', >> + 'spice': 'AudiodevSpiceOptions', >> + 'wav': 'AudiodevWavOptions' } } > > AudiodevNoneOptions and AudiodevSpiceOptions are identical; you could > consolidate them into one struct. I'd also (someday) like to get to the > point of using anonymous structures in a union, particularly when the > variant adds no additional fields, so that you could do: > > 'data': { > 'none': {}, > 'alsa': 'AudiodevAlsaOptions', ... > > but don't know if that is worth doing any sooner than Markus can land > his introspection patches. > >> + >> +## >> +# @AudioFormat >> +# >> +# An enumeration of possible audio formats. >> +# >> +# Since: XXX >> +## >> +{ 'enum': 'AudioFormat', >> + 'data': [ 'u8', 's8', 'u16', 's16', 'u32', 's32' ] } >> + >> +## >> +# @AudiodevPerDirectionOptions >> +# >> +# General audio backend options that are used for both playback and recording. >> +# >> +# @fixed_settings: #optional use fixed settings for host DAC/ADC >> +# >> +# @frequency: #optional frequency to use when using fixed settings >> +# >> +# @channels: #optional number of channels when using fixed settings >> +# >> +# @format: #optional sample fortmat to use when using fixed settings >> +# >> +# @try_poll: #optional attempt to use poll mode >> +# >> +# Since: XXX >> +## >> +{ 'struct': 'AudiodevPerDirectionOptions', >> + 'data': { >> + '*fixed_settings': 'bool', >> + '*frequency': 'int', >> + '*channels': 'int', >> + '*format': 'AudioFormat', >> + '*try_poll': 'bool' } } >> + >> +## >> +# @Audiodev >> +# >> +# Captures the configuration of an audio backend. >> +# >> +# @id: identifier of the backend. > > Inconsistent on whether you end with '.' (but the whole file is already > that inconsistent, so not too much of a worry) > >> +# >> +# @in: #optional options of the capture stream. >> +# >> +# @out: #optional options of the playback stream. >> +# >> +# @timer_period: #optional timer period in HZ (0 - use lowest possible) >> +# >> +# @plive: #optional >> +# >> +# @opts: audio backend specific options >> +# >> +# Since XXX >> +## >> +{ 'struct': 'Audiodev', >> + 'data': { >> + 'id': 'str', >> + '*in': 'AudiodevPerDirectionOptions', >> + '*out': 'AudiodevPerDirectionOptions', >> + '*timer_period': 'int', >> + '*plive': 'bool', >> + 'opts': 'AudiodevBackendOptions' } } >> > > Overall looks fairly reasonable. Is it enough structures to be worth > splitting into a new qapi/audio.json file and merely touch > qapi-schema.json to add an include? > Maybe. Given that there are a few quite short includes already, it should be ok...