"Zoltán Kővágó" <dirty.ice...@gmail.com> writes:
> On 2019-09-23 15:12, Markus Armbruster wrote:
>> "Kővágó, Zoltán" <dirty.ice...@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...@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.
