From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42908) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5DLZ-0004sP-VO for qemu-devel@nongnu.org; Wed, 17 Jun 2015 09:25:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5DLT-0004s8-Ls for qemu-devel@nongnu.org; Wed, 17 Jun 2015 09:25:41 -0400 Received: from mail-wg0-x22a.google.com ([2a00:1450:400c:c00::22a]:36578) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5DLT-0004pg-Ci for qemu-devel@nongnu.org; Wed, 17 Jun 2015 09:25:35 -0400 Received: by wgzl5 with SMTP id l5so37048394wgz.3 for ; Wed, 17 Jun 2015 06:25:34 -0700 (PDT) From: "=?UTF-8?B?S8WRdsOhZ8OzIFpvbHTDoW4=?=" Message-ID: <55817550.1050606@gmail.com> Date: Wed, 17 Jun 2015 15:25:36 +0200 MIME-Version: 1.0 References: <34d6db2f41b9101490b3f3549a9f1c5a85be03c8.1434458391.git.DirtY.iCE.hu@gmail.com> <874mm6g3b8.fsf@blackfin.pond.sub.org> <5581579B.2020402@gmail.com> <87wpz24j0c.fsf@blackfin.pond.sub.org> In-Reply-To: <87wpz24j0c.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 6/6] audio: -audiodev command line option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann 2015-06-17 14:27 keltezéssel, Markus Armbruster írta: > "Kővágó Zoltán" writes: > >> 2015-06-17 10:13 keltezéssel, Markus Armbruster írta: >>> "Kővágó, Zoltán" writes: >>> >>>> This patch adds an -audiodev command line option, and deprecates the QEMU_* >>>> environment variables for audio backend configuration. It's syntax >>>> is similar to >>>> existing options (-netdev, -device, etc): >>>> -audiodev driver_name,property=value,... >>> >>> Sounds really good. >>> >>> Please wrap your commit message lines a bit earlier, around column 70. >>> >>>> Audio drivers now get an Audiodev * as config paramters, instead of >>>> the global >>>> audio_option structs. There is some code in audio/audio_legacy.c >>>> that converts >>>> the old environment variables to audiodev options (this way >>>> backends do not have >>>> to worry about legacy options, also print out them with -audio-help, to ease >>>> migrating to -audiodev). >>> >>> The parenthesis isn't as clear as the rest of your message, probably >>> because it deals with two separate things. Suggest to move out the bit >>> about help into its own paragraph. >>> >>>> Although now it's possible to specify multiple -audiodev options on command >>>> line, multiple audio backends are not supported yet. >>> >>> What happens when I specify multiple -audiodev? >> >> You get an error and qemu terminates. > > Unlikely to create backward compatibility trouble. Works for me. > >>> How should the command line look like when multiple audio backends are >>> supported? >> >> There's an id property of audiodev, so you can identify them: >> -audiodev alsa,id=foo,... -audiodev pa,id=bar,... >> and audio devices should get an extra parameter, like audiodev or >> something like that: >> -device usb-audio,audiodev=foo -device usb-audio,audiodev=bar >> And you have two cards, one connected to the alsa device and the other >> connected to pulseaudio. > > Good, because it's consistent with how we connect other kinds of > frontends/backends. > > Multiple audio frontends (device models) can connect to the same audio > backend, can't they? Yes. (It's mandatory, since currently all audio frontends connect to a single backend.) > Is backend property "id" mandatory? Hmm, judging from PATCH 1 it isn't. > It generally is for other kinds of backends, e.g. -netdev, -chardev and > (in the future) -blockdev. It's optional with -drive only because > -drive is crazy. It's optional right now (because I thought specifying an id when you have a single backend is pretty pointless), but it's probably better then if I change it to mandatory. > >>> Do we have a clear backward-compatible path from here to there? >> >> Currently if you specify an -audiodev option, the environment >> variables are completely ignored, and it will create an audio backend >> using the specified options. If you do not provide an -audiodev, it >> will initialize the audio subsystem using the old environment >> variables when you add the first sound card (so no -audiodev and no >> sound device means no audio subsystem, just like the old times). > > Should we document this? Where? Maybe adding a phrase to -audiodev documentation like "If you specify this option, the deprecated environment variables will be ignored". >> About multiple backends: if the user does not specify the id of the >> backend when creating the sound card, just use the first -audiodev >> specified on the command line (or the legacy config, if there's no >> -audiodev). This way we stay backward-compatible (there won't be >> multiple -audiodevs in legacy configs). > > Old way (before your patch): there is only one audio backend, and it's > configuration comes from a bunch of environment variables. > > New way (we're not there yet): you can have multiple audio backends, and > you connect frontends to backends using backend ID as usual, i.e. id=ID > on the backend, audiodev=ID on the frontend. > > Required backward compatibility: the old way still works, i.e. when > there's no new-way backend, and no frontend has an audiodev=..., then we > create a backend configured from the environment. > > Anything beyond required backward compatibility should be carefully > judged on its UI merits. > > If I understand your description correctly, the plan is to default a > frontend's audiodev to the first backend, and if there is none, create > one configured from the environment. That's definitely beyond required. > Is it a good user interface? The "create one configured from the environment" is needed for backward compatibility. Specifying an audiodev=... on frontend makes no sense without an -audiodev, so that's not allowed. The only question is what happens with an -audiodev, but no audiodev= on the frontend. We can't make audiodev= required because that would break compatibility. So we either a) make audiodev= required but only when there's an -audiodev, or b) try to find a sensible default when audiodev= is not specified. Imho b is better, because having a parameter that's sometimes required, sometimes forbidden is a bit confusing.