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

[Kővágó Zoltán]

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

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