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

[Zoltán Kővágó]

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

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}][,...]]