Re: [PATCH v4 21/24] paaudio: channel-map option

[Markus Armbruster <armbru@redhat.com>]

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

> On 2019-09-23 15:12, Markus Armbruster wrote:
>> "Kővágó, Zoltán" <dirty.ice.hu@gmail.com> writes:
>>
>>> Add an option to change the channel map used by pulseaudio.  If not
>>> specified, falls back to an OSS compatible channel map.
>>>
>>> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
>>> ---
>>>   audio/paaudio.c | 18 ++++++++++++++----
>>>   qapi/audio.json |  7 +++++--
>>>   qemu-options.hx |  9 +++++++++
>>>   3 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/audio/paaudio.c b/audio/paaudio.c
>>> index d195b1caa8..20402b0718 100644
>>> --- a/audio/paaudio.c
>>> +++ b/audio/paaudio.c
>>> @@ -338,17 +338,27 @@ static pa_stream *qpa_simple_new (
>>>           pa_stream_direction_t dir,
>>>           const char *dev,
>>>           const pa_sample_spec *ss,
>>> -        const pa_channel_map *map,
>>> +        const char *map,
>>>           const pa_buffer_attr *attr,
>>>           int *rerror)
>>>   {
>>>       int r;
>>>       pa_stream *stream;
>>>       pa_stream_flags_t flags;
>>> +    pa_channel_map pa_map;
>>>         pa_threaded_mainloop_lock(c->mainloop);
>>>   -    stream = pa_stream_new(c->context, name, ss, map);
>>> +    if (map && !pa_channel_map_parse(&pa_map, map)) {
>>> +        dolog("Invalid channel map specified: '%s'\n", map);
>>> +        map = NULL;
>>> +    }
>>> +    if (!map) {
>>> +        pa_channel_map_init_extend(&pa_map, ss->channels,
>>> +                                   PA_CHANNEL_MAP_OSS);
>>> +    }
>>> +
>>> +    stream = pa_stream_new(c->context, name, ss, &pa_map);
>>>       if (!stream) {
>>>           goto fail;
>>>       }
>>> @@ -421,7 +431,7 @@ static int qpa_init_out(HWVoiceOut *hw, struct audsettings *as,
>>>           PA_STREAM_PLAYBACK,
>>>           ppdo->has_name ? ppdo->name : NULL,
>>>           &ss,
>>> -        NULL,                   /* channel map */
>>> +        ppdo->has_channel_map ? ppdo->channel_map : NULL,
>>>           &ba,                    /* buffering attributes */
>>>           &error
>>>           );
>>> @@ -470,7 +480,7 @@ static int qpa_init_in(HWVoiceIn *hw, struct audsettings *as, void *drv_opaque)
>>>           PA_STREAM_RECORD,
>>>           ppdo->has_name ? ppdo->name : NULL,
>>>           &ss,
>>> -        NULL,                   /* channel map */
>>> +        ppdo->has_channel_map ? ppdo->channel_map : NULL,
>>>           &ba,                    /* buffering attributes */
>>>           &error
>>>           );
>>> diff --git a/qapi/audio.json b/qapi/audio.json
>>> index 0535eff794..07003808cb 100644
>>> --- a/qapi/audio.json
>>> +++ b/qapi/audio.json
>>> @@ -214,13 +214,16 @@
>>>   # @latency: latency you want PulseAudio to achieve in microseconds
>>>   #           (default 15000)
>>>   #
>>> +# @channel-map: channel map to use (default: OSS compatible map, since: 4.2)
>>> +#
>>>   # Since: 4.0
>>>   ##
>>>   { 'struct': 'AudiodevPaPerDirectionOptions',
>>>     'base': 'AudiodevPerDirectionOptions',
>>>     'data': {
>>> -    '*name': 'str',
>>> -    '*latency': 'uint32' } }
>>> +    '*name':        'str',
>>> +    '*latency':     'uint32',
>>> +    '*channel-map': 'str' } }
>>>     ##
>>>   # @AudiodevPaOptions:
>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>> index 395427422a..f3bc342f98 100644
>>> --- a/qemu-options.hx
>>> +++ b/qemu-options.hx
>>> @@ -471,6 +471,7 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
>>>       "-audiodev pa,id=id[,prop[=value][,...]]\n"
>>>       "                server= PulseAudio server address\n"
>>>       "                in|out.name= source/sink device name\n"
>>> +    "                in|out.channel-map= channel map to use\n"
>>>   #endif
>>>   #ifdef CONFIG_AUDIO_SDL
>>>       "-audiodev sdl,id=id[,prop[=value][,...]]\n"
>>> @@ -636,6 +637,14 @@ Sets the PulseAudio @var{server} to connect to.
>>>   @item in|out.name=@var{sink}
>>>   Use the specified source/sink for recording/playback.
>>>   +@item in|out.channel-map=@var{map}
>>> +Use the specified channel map.  The default is an OSS compatible
>>> +channel map.  Do not forget to escape commas inside the map:
>>
>> Awkward.
>>
>>> +
>>> +@example
>>> +-audiodev pa,id=example,sink.channel-map=front-left,,front-right
>>> +@end example
>>
>> Makes me realize new AudiodevPaPerDirectionOptions member @channel-map
>> is a list encoded in a string.  QAPI heavily frowns upon encoding stuff
>> in strings.  Any reason why you can't (or don't want to) make it
>> ['str']?
>
> Hmm, I don't think it's used too frequently on structs parsed by qapi
> opts visitor. What would be the command line format in that case?
> Something like this?
>
> -audiodev
> pa,id=example,sink.channel-map=front-left,sink.channel-map=front-right

Let's talk concepts before details.  I'll get back to this below.

> I think it's simply a string because while conceptually it's a string,
> we don't try to interpret it, we just pass the string to
> pa_channel_map_parse.  Of course we could take a list and instead
> either rebuild the string or reimplement half of pa_channel_map_parse
> by manually calling pa_channel_position_from_string.

Precedence: BlockdevOptionsRbd member @auth-client-required.  Under the
hood, this is Ceph configuration option auth-client-required.
Semantically a list of well-known values.  rados_conf_set() takes it
encoded in a string.  We could have plumbed that straight through QAPI
by making @auth-client-required a 'str'.  Instead, we made it
['RbdAuthMode'].  rbd.c has to encode the list in a string for
rados_conf_set().

Why did we bother?  Besides the dogmatic reason "do not encode stuff in
strings in QAPI", there's a pragmatic one: introspection.
@auth-client-required's type ['RbdAuthMode'] tells exactly what the
acceptable values are.  If Ceph ever grows another mode, QEMU support
for it will be visible in introspection: enum RbdAuthMode will have
another value.

Baking the acceptable modes into QEMU means new modes need a QEMU update
to work.  New modes that just work without a QEMU update feel unlikely.
If we're wrong, and a QEMU update is impractical, you can still work
around the limitation with a Ceph configuration file.

Which of these considerations other than "do not encode in strings"
apply to PA I can't say.

Let's turn the question around: is there any reason to break the "do not
encode in strings" rule other than "it saves a few lines of code"?

> Oh now that I looked again at the pulseaudio docs, channel-map doesn't
> have to be a list, it can be also a "well-known mapping name".

Unambiguous because the well-known mapping names are not valid channel
position list members.

Semantially, this is a disjoint union of well-known mapping names and
channel position lists.

The tools QAPI provides for disjoint unions are union and alternate
types.  In this case, an alternate of 'str' and ['str'] would do.
Well-known mapping names use the 'str' branch, channel position lists
the ['str'] branch.

>>
>>> +
>>>   @end table
>>>     @item -audiodev
>>> sdl,id=@var{id}[,@var{prop}[=@var{value}][,...]]

I promised you to get back to command line syntax.

The opts visitor uses repeated keys for lists, just like you guessed.
It supports only a subset of the QAPI types, and has weird magic for
integer lists.

-audiodev actually uses the QObject input visitor, which is more general
and somewhat less magical.  Its CLI syntax for lists is even wordier,
though.  I'd expect something like

    -audiodev pa,id=example,sink.channel-map.0=front-left,sink.channel-map.1=front-right

Aside: CLI QAPIfication will have to find a way to accomodate the opts
visitor's syntax and quirks, or break compatibility.