[RFC,v2] qapi for audio backends

Changes from v1:
* fixed style issues
* moved definitions into a separate file
* documented undocumented options (hopefully)
* removed plive option. It was useless even years ago so it can probably safely
  go away: https://lists.nongnu.org/archive/html/qemu-devel/2012-03/msg02427.html
* removed verbose, debug options. Backends should use trace events instead.
* removed *_retries options from dsound. It's a kludge.
* moved buffer_usecs and buffer_count to the global config options. Some driver
  might ignore it (as they do not expose API to change them).
* wav backend: removed frequecy, format, channels as AudiodevPerDirectionOptions
  already have them.

Not sure about this one, so it's not yet in this patch:
* remove poll_mode: another obscure setting, and it's only matter of time until
  the code bitrots enough to break it.

This is a proposal to add structures into qapi 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 <DirtY.iCE.hu@gmail.com>
---
 qapi-schema.json |   3 +
 qapi/audio.json  | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 228 insertions(+)
 create mode 100644 qapi/audio.json

Hi,

> Not sure about this one, so it's not yet in this patch:
> * remove poll_mode: another obscure setting, and it's only matter of time until
>   the code bitrots enough to break it.

I'd tend to drop this too, but it's probably good to check what exactly
it is doing and to test whenever it actually works.

> Any comment is appreciated.

Looks good to me as draft to start working with.  I expect we'll find
some details which need tweeking when implementing this.

cheers,
  Gerd

Hi,

2015-06-04 17:30 keltezéssel, Gerd Hoffmann írta:
>> Not sure about this one, so it's not yet in this patch:
>> * remove poll_mode: another obscure setting, and it's only matter of time until
>>    the code bitrots enough to break it.
>
> I'd tend to drop this too, but it's probably good to check what exactly
> it is doing and to test whenever it actually works.

In my quick test it actually worked (but that was with pulseaudio's alsa 
emulation). Plus currently only alsa an oss seem to care about this 
option, so even if we keep it, we should probably move it into alsa's 
and oss's backend options.

>> Any comment is appreciated.
>
> Looks good to me as draft to start working with.  I expect we'll find
> some details which need tweeking when implementing this.
>

Yeah, I've already hit a problem. The opts_visitor doesn't really handle 
nested structs (it just flattens it into a single, non hierarchic 
namespace), which is a problem because of the input and output options. 
First I need to make them required (the in and out in Audiodev) if I 
want the current visitor to visit them at all, but it's still not enough.

Doing something like -audiodev frequency=8000 sets the input frequency 
to 8000 and leaves output frequency undefined. I think I should add an 
additional syntax: -audiodev in.frequency=8000,out.frequency=16000 (of 
course it should support deeper nesting like foo.bar.baz.asd=13). The 
question is what should happen if the user specifies frequency=8000. I 
see two alternatives:

0. set every frequency field to 8000 (i.e. the same as 
in.frequency=8000,out.frequency=8000 in this example)
1. bail out with a parameter ambiguous error

In the case of audiodev, the 0. approach seems more straightforward (if 
the user sets frequency, he wants to set both input and output 
frequency), but in more generic scenarios, the 1. is maybe better.

Thanks,
Zoltan

Hi,

> Plus currently only alsa an oss seem to care about this 
> option, so even if we keep it, we should probably move it into alsa's 
> and oss's backend options.

Makes sense.

> > Looks good to me as draft to start working with.  I expect we'll find
> > some details which need tweeking when implementing this.
> 
> Yeah, I've already hit a problem. The opts_visitor doesn't really handle 
> nested structs (it just flattens it into a single, non hierarchic 
> namespace), which is a problem because of the input and output options. 
> First I need to make them required (the in and out in Audiodev) if I 
> want the current visitor to visit them at all, but it's still not enough.

I think we should improve the visitor instead of making in & out
mandatory just because the current implementation (which simply
implements the stuff needed for Netdev) can't handle it.

> Doing something like -audiodev frequency=8000 sets the input frequency 
> to 8000 and leaves output frequency undefined. I think I should add an 
> additional syntax: -audiodev in.frequency=8000,out.frequency=16000 (of 
> course it should support deeper nesting like foo.bar.baz.asd=13). The 
> question is what should happen if the user specifies frequency=8000. I 
> see two alternatives:
> 
> 0. set every frequency field to 8000 (i.e. the same as 
> in.frequency=8000,out.frequency=8000 in this example)
> 1. bail out with a parameter ambiguous error
> 
> In the case of audiodev, the 0. approach seems more straightforward (if 
> the user sets frequency, he wants to set both input and output 
> frequency),

Agree.

>  but in more generic scenarios, the 1. is maybe better.

There is always the option to be more specific (in.frequency=...) if
setting all parameters named 'frequency' doesn't do what you want.  Also
I wouldn't worry too much about possible cases which don't exist right
now.  I'd suggest to go for (0).

cheers,
  Gerd

Hi,

2015-06-05 12:57 keltezéssel, Gerd Hoffmann írta:
>> Yeah, I've already hit a problem. The opts_visitor doesn't really handle
>> nested structs (it just flattens it into a single, non hierarchic
>> namespace), which is a problem because of the input and output options.
>> First I need to make them required (the in and out in Audiodev) if I
>> want the current visitor to visit them at all, but it's still not enough.
>
> I think we should improve the visitor instead of making in & out
> mandatory just because the current implementation (which simply
> implements the stuff needed for Netdev) can't handle it.

It's not that simple I think. The visit_optional only receives a field 
name, but no info about the type of the field, but it has to decide if 
it wants the field using only this info. So sort of hacking an if 
(strcmp(name, "in") == 0 || strcmp(name, "out") == 0) ... into the 
option visitor code, the only way is probably to add a type parameter to 
visit_optional (struct/union/uint32/whatever) and in the opts visitor if 
type is struct or union, force visiting it. Is it ok?

> There is always the option to be more specific (in.frequency=...) if
> setting all parameters named 'frequency' doesn't do what you want.  Also
> I wouldn't worry too much about possible cases which don't exist right
> now.  I'd suggest to go for (0).

Alright.

Thanks,
Zoltan

On Fr, 2015-06-05 at 15:54 +0200, Kővágó Zoltán wrote:
> Hi,
> 
> 2015-06-05 12:57 keltezéssel, Gerd Hoffmann írta:
> >> Yeah, I've already hit a problem. The opts_visitor doesn't really handle
> >> nested structs (it just flattens it into a single, non hierarchic
> >> namespace), which is a problem because of the input and output options.
> >> First I need to make them required (the in and out in Audiodev) if I
> >> want the current visitor to visit them at all, but it's still not enough.
> >
> > I think we should improve the visitor instead of making in & out
> > mandatory just because the current implementation (which simply
> > implements the stuff needed for Netdev) can't handle it.
> 
> It's not that simple I think. The visit_optional only receives a field 
> name, but no info about the type of the field, but it has to decide if 
> it wants the field using only this info. So sort of hacking an if 
> (strcmp(name, "in") == 0 || strcmp(name, "out") == 0) ... into the 
> option visitor code, the only way is probably to add a type parameter to 
> visit_optional (struct/union/uint32/whatever) and in the opts visitor if 
> type is struct or union, force visiting it. Is it ok?

Thinking about this a bit more, I suspect it becomes tricky when it
comes to allocation.  We want allocate *in and *out only in case there
are actually parameters for it on the command line.

But I think we have to pass something down to the struct visitor (given
how visitors are designed).  So allocate, pass down, then free again in
case nothing was filled?  How do we figure nothing was filled, in the
generic visit_optional function?

Of course we can avoid this complexity by simply allocating *in and *out
unconditionally, but then it is pointless to make it optional in the
first place.  So maybe it is better to go with your original idea to
just make them mandatory (and all elements inside optional).

cheers,
  Gerd