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.
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
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]:
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