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

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

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

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

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