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

[Markus Armbruster]

"Kővágó Zoltán" <address@hidden> writes:

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

Understood.

>> 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.

Let's make it mandatory for now.  We can always change mandatory to
optional, but not the other way round.

>>>> 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".

Works for me.

>>> 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.

If we didn't have to do backwards compatibility, we'd make audiodev
mandatory, wouldn't we?

Here's how I'd add back-compat to that baseline.  audiodev becomes
optional, but omitting it is deprecated.  If you do, you implicitly use
the legacy backend that takes its configuration from the environment.
The legacy backend gets created when a frontend is using it.

If you think you got a better way to do it, the person to convince is
Gerd.

I'll try to find time to experiment with flattening your QAPI schema,
perhaps we can make things a bit simpler.

I'm glad you're taking care of turning our bizarro audio UI into
something that actually fits in with the rest.  Much appreciated!