| Message ID | 8f650662fd6cc50baaede260581aeb560eed44fb.1568927990.git.DirtY.iCE.hu@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Audio: Mixeng-free 5.1/7.1 audio support | expand |
"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']? > + > @end table > > @item -audiodev sdl,id=@var{id}[,@var{prop}[=@var{value}][,...]]
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}][,...]]
"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.
Hi, > > 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. Do we have well-known mapping names for the common use cases? So maybe just support these? cheers, Gerd
On 2019-09-25 16:13, Gerd Hoffmann wrote: > Hi, > >>> 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. > > Do we have well-known mapping names for the common use cases? > So maybe just support these? > > cheers, > Gerd > It's surprisingly hard to figure out what are these "well-known names", I could only find them in the source: https://github.com/michaelwu/pulseaudio/blob/16e3dccfa88455ebd329de27a98a3d979a8bdc8e/src/pulse/channelmap.c#L538 It doesn't provide much over the "auto detect channel maps from the number of channels" pulseaudio already has (the only ambiguous case is surround-41 vs surround-50). IIRC I originally added this feature because USB audio and pulseaudio didn't agree about the order of channels, for example in 5.1 pa defaults to left,right,rear-left,rear-right,front-center,lfe while USB audio expects left,right,center,lfe,rear-left,rear-right order. You can't specify this with the well-known names. Regards, Zoltan
Hi, > vs surround-50). IIRC I originally added this feature because USB audio and > pulseaudio didn't agree about the order of channels, for example in 5.1 pa > defaults to left,right,rear-left,rear-right,front-center,lfe while USB audio > expects left,right,center,lfe,rear-left,rear-right order. You can't specify > this with the well-known names. Shouldn't backend and frontend be able to negotiate a channel map without user invention? Initially, with only usb-audio supporting 5.1/7.1 we can simply declare the usb-audio channel map as the one which is used. When other devices are added some day (hda-surround?) we might need a ctl function so the frontend can set the channel map (similar to the way volume is handled today). cheers, Gerd
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: + +@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}][,...]]
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(-)