Re: [Qemu-devel] [PATCH v2 6/6] audio: -audiodev command line option

["Kővágó Zoltán" <dirty.ice.hu@gmail.com>]

2015-06-17 14:27 keltezéssel, Markus Armbruster írta:
> "Kővágó Zoltán" <dirty.ice.hu@gmail.com> writes:
>
>> 2015-06-17 10:13 keltezéssel, Markus Armbruster írta:
>>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> 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.