After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 763799 - alsasrc: should not always assume that 8 channels implies 7.1 setup
alsasrc: should not always assume that 8 channels implies 7.1 setup
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on: 763985
Blocks:
 
 
Reported: 2016-03-17 10:09 UTC by Guillaume Desmottes
Modified: 2016-04-12 18:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
audioringbuffer: don't attempt to reorder position-less channels (2.13 KB, patch)
2016-03-21 15:44 UTC, Guillaume Desmottes
committed Details | Review
alsa: properly convert position-less channels from ALSA (1.78 KB, patch)
2016-03-21 15:45 UTC, Guillaume Desmottes
committed Details | Review

Description Guillaume Desmottes 2016-03-17 10:09:52 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.
Comment 1 Guillaume Desmottes 2016-03-18 16:25:07 UTC
(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?
Comment 2 Guillaume Desmottes 2016-03-21 11:53:12 UTC
As part of this work I opened bug#763985 adding some debug output.
Comment 3 Guillaume Desmottes 2016-03-21 15:44:52 UTC
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.
Comment 4 Guillaume Desmottes 2016-03-21 15:45:05 UTC
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.
Comment 5 Nicolas Dufresne (ndufresne) 2016-04-11 14:14:00 UTC
Review of attachment 324450 [details] [review]:

Sound reasonable to me.
Comment 6 Nicolas Dufresne (ndufresne) 2016-04-11 14:14:08 UTC
Review of attachment 324450 [details] [review]:

Sound reasonable to me.
Comment 7 Nicolas Dufresne (ndufresne) 2016-04-11 14:15:01 UTC
Review of attachment 324451 [details] [review]:

Good.
Comment 8 Guillaume Desmottes 2016-04-12 07:04:13 UTC
Thanks for the review.

Those patches depend on bug#763985 so we should merge this one first.
Comment 9 Nicolas Dufresne (ndufresne) 2016-04-12 18:50:00 UTC
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
Comment 10 Nicolas Dufresne (ndufresne) 2016-04-12 18:50:58 UTC
Thanks.