GNOME Bugzilla – Bug 763799
alsasrc: should not always assume that 8 channels implies 7.1 setup
Last modified: 2016-04-12 18:50:58 UTC
We are using a professional audio card to record from 8 channels. It's a very specific use cases so they are really 8 independent channels recorded altogether. We deinterleave and process them separately. Problem is, alsasrc handles them as a 7.1 setup and does some reordering when capturing which is not what we want. Here is what happens: When detecting which channels configuration is supported by the sound card, gstalsa.c:caps_add_channel_configuration() assumes that having 8 channels means 7.1 audio and so set it as channel-mask on the caps. This behavior is hard coded. Later when gst_audio_ring_buffer_set_channel_positions() is called it checks if the channel positions of the stream (which has been derived from the channel-mask using gst_audio_channel_positions_from_mask()) matches the channel positions of ALSA (the alsa_positions array defined in gstalsa.c). As the stream claimed to be 7.1 and as the ALSA channel ordering is different from the GST ordering, a reordering map is generated using gst_audio_get_channel_reorder_map() and samples are reordered by alsasrc. This breaks things with our hardware as the 8 channels are really independents and so don't have any specific position. A simple hack disabling re-ordering in gst_audio_ring_buffer_set_channel_positions() workarounds the problem for us but we'd like to fix this properly upstream. I think the proper way to fix this would be to read the channel positions from ALSA (cf https://www.kernel.org/doc/Documentation/sound/alsa/Channel-Mapping-API.txt ) but I'm not sure of the right way to use this info. caps_add_channel_configuration() could use it to set the proper channel-mask. In this case, the driver would say that all channels are independents (8 SNDRV_CHMAP_MONO) and gst would turn this to channel-mask=0. Then the audioringbuffer could check in gst_audio_ring_buffer_set_channel_positions() if the stream position is composed only of GST_AUDIO_CHANNEL_POSITION_NONE and don't do any reodering in this case. GST_AUDIO_CHANNEL_POSITION_NONE is defined is the doc as "used for position-less channels, e.g. from a sound card that records 1024 channels; mutually exclusive with any other channel position" so that seems to be what we want here. But I'm wondering if gst_audio_ring_buffer_set_channel_positions() shouldn't also use the channel positions returned by alsa instead of the hardcoded alsa_position array. In this case gst_audio_ring_buffer_set_channel_positions() shouldn't be modified as both array will then be equals (8 GST_AUDIO_CHANNEL_POSITION_NONE). Of course we'll have to keep the existing code path for old drivers not implementing ALSA's channel mappings API. I didn't check in details but I think the same problem may arise with alsasink as the code paths are pretty symetric.
(In reply to Guillaume Desmottes from comment #0) > But I'm wondering if gst_audio_ring_buffer_set_channel_positions() shouldn't > also use the channel positions returned by alsa instead of the hardcoded > alsa_position array. In this case > gst_audio_ring_buffer_set_channel_positions() shouldn't be modified as both > array will then be equals (8 GST_AUDIO_CHANNEL_POSITION_NONE). > asink as the code paths are pretty symetric. Actually, alsasrc/alsasink already query Alsa for the channel map (using snd_pcm_get_chmap()) and passes this as channel position to the ringbuffer. See https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/ext/alsa/gstalsasink.c#n911 So this work is already done. But as long as gstalsa.c:caps_add_channel_configuration() keeps assuming that 8 channels implies 7.1 gst will reorder things. Shouldn't caps_add_channel_configuration() use the channel map from ALSA as well?
As part of this work I opened bug#763985 adding some debug output.
Created attachment 324450 [details] [review] audioringbuffer: don't attempt to reorder position-less channels As said in its doc GST_AUDIO_CHANNEL_POSITION_NONE is meant to be used for "position-less channels, e.g. from a sound card that records 1024 channels; mutually exclusive with any other channel position". But at the moment using such positions would raise a 'g_return_if_reached' warning as gst_audio_get_channel_reorder_map() would reject it. Fix this by preventing any attempt to reorder in such case as that's not what we want anyway.
Created attachment 324451 [details] [review] alsa: properly convert position-less channels from ALSA The only way for ALSA to expose a position-less multi channels is to return an array full of SND_CHMAP_MONO. Converting this to a GST_AUDIO_CHANNEL_POSITION_MONO array would be invalid as GST_AUDIO_CHANNEL_POSITION_MONO is meant to be used only with one channel. Fix this by using GST_AUDIO_CHANNEL_POSITION_NONE which is meant to be used for position-less channels.
Review of attachment 324450 [details] [review]: Sound reasonable to me.
Review of attachment 324451 [details] [review]: Good.
Thanks for the review. Those patches depend on bug#763985 so we should merge this one first.
Attachment 324450 [details] pushed as 7c5dfd7 - audioringbuffer: don't attempt to reorder position-less channels Attachment 324451 [details] pushed as 6e7fa66 - alsa: properly convert position-less channels from ALSA
Thanks.