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>
---
 qapi/audio.json |  5 ++++-
 audio/paaudio.c | 18 ++++++++++++++----
 qemu-options.hx |  9 +++++++++
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/qapi/audio.json b/qapi/audio.json
index 7bcea6240f..86078039dc 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -107,11 +107,14 @@
 #
 # @name: name of the sink/source to use
 #
+# @channel-map: channel map to use (default: OSS compatible map)
+#
 # Since: 4.0
 ##
 { 'struct': 'AudiodevPaPerDirectionOptions',
   'data': {
-    '*name': 'str' } }
+    '*name':        'str',
+    '*channel-map': 'str' } }
 
 ##
 # @AudiodevPaOptions:
diff --git a/audio/paaudio.c b/audio/paaudio.c
index 0d67c03b4c..b0e311584a 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -337,17 +337,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;
     }
@@ -424,7 +434,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
         );
@@ -472,7 +482,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,
         NULL,                   /* buffering attributes */
         &error
         );
diff --git a/qemu-options.hx b/qemu-options.hx
index 53fc5ef453..c4835f77c3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -468,6 +468,7 @@ DEF("audiodev", HAS_ARG, QEMU_OPTION_audiodev,
     "-audiodev pa,id=id[,prop[=value][,...]]\n"
     "                server= PulseAudio server address\n"
     "                sink|source.name= sink/source device name\n"
+    "                sink|source.channel-map= channel map to use\n"
 #endif
 #ifdef CONFIG_SDL
     "-audiodev sdl,id=id[,prop[=value][,...]]\n"
@@ -621,6 +622,14 @@ Sets the PulseAudio @var{server} to connect to.
 @item sink|source.name=@var{sink}
 Use the specified sink/source for playback/recording.
 
+@item sink|source.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:
+
+@example
+-audiodev pa,id=example,sink.channel-map=front-left,,front-right
+@end example
+
 @end table
 
 @item -audiodev sdl,id=@var{id}[,@var{prop}[=@var{value}][,...]]
-- 
2.20.1

On Thu, Jan 17, 2019 at 12:37:20AM +0100, Kővágó, Zoltán wrote:
> 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>
> ---
>  qapi/audio.json |  5 ++++-
>  audio/paaudio.c | 18 ++++++++++++++----
>  qemu-options.hx |  9 +++++++++
>  3 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/audio.json b/qapi/audio.json
> index 7bcea6240f..86078039dc 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -107,11 +107,14 @@
>  #
>  # @name: name of the sink/source to use
>  #
> +# @channel-map: channel map to use (default: OSS compatible map)
> +#
>  # Since: 4.0
>  ##
>  { 'struct': 'AudiodevPaPerDirectionOptions',
>    'data': {
> -    '*name': 'str' } }
> +    '*name':        'str',
> +    '*channel-map': 'str' } }

Ah, I see.  Thats why patch #1 creates a AudiodevPaPerDirectionOptions
struct with just one field ...

cheers,
  Gerd

On 2019-01-17 11:03, Gerd Hoffmann wrote:
> On Thu, Jan 17, 2019 at 12:37:20AM +0100, Kővágó, Zoltán wrote:
>> 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>
>> ---
>>  qapi/audio.json |  5 ++++-
>>  audio/paaudio.c | 18 ++++++++++++++----
>>  qemu-options.hx |  9 +++++++++
>>  3 files changed, 27 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/audio.json b/qapi/audio.json
>> index 7bcea6240f..86078039dc 100644
>> --- a/qapi/audio.json
>> +++ b/qapi/audio.json
>> @@ -107,11 +107,14 @@
>>  #
>>  # @name: name of the sink/source to use
>>  #
>> +# @channel-map: channel map to use (default: OSS compatible map)
>> +#
>>  # Since: 4.0
>>  ##
>>  { 'struct': 'AudiodevPaPerDirectionOptions',
>>    'data': {
>> -    '*name': 'str' } }
>> +    '*name':        'str',
>> +    '*channel-map': 'str' } }
> 
> Ah, I see.  Thats why patch #1 creates a AudiodevPaPerDirectionOptions
> struct with just one field ...

Yes, I was bitten by it too during a refactor, probably I should write a
few words about it in the commit message.

However, this is the exact reason I'd recommend nested structs instead
of randomly flattening it when we can.  This way, if we later have to
add an extra option, we don't end up in a problematic situation, since
we can't easily change things like 'dev-in' to a structure without
breaking backward compatibility.

Regards,
Zoltan

On 1/23/19 2:13 PM, Zoltán Kővágó wrote:

>>>  { 'struct': 'AudiodevPaPerDirectionOptions',
>>>    'data': {
>>> -    '*name': 'str' } }
>>> +    '*name':        'str',
>>> +    '*channel-map': 'str' } }
>>
>> Ah, I see.  Thats why patch #1 creates a AudiodevPaPerDirectionOptions
>> struct with just one field ...
> 
> Yes, I was bitten by it too during a refactor, probably I should write a
> few words about it in the commit message.

Yes, a good commit message tries to anticipate the questions the
reviewer will ask. The "what" is easy to see (read the patch), but the
"why" is the one that determines whether the patch is worth including.

> 
> However, this is the exact reason I'd recommend nested structs instead
> of randomly flattening it when we can.  This way, if we later have to
> add an extra option, we don't end up in a problematic situation, since
> we can't easily change things like 'dev-in' to a structure without
> breaking backward compatibility.

We can support QAPI Alternates, accepting either a string or a struct
for a given member name like 'dev-in'. Admittedly, it's a bit harder to
write C code interfacing with a QAPI alternate type, but at least it is
possible and allows for back-compatibility.  Then again, getting the
struct right in the first place is even nicer than having to make
back-compat tweaks.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org