GNOME Bugzilla – Bug 792897
wasapi: Implement multichannel support and a device provider
Last modified: 2018-01-31 09:44:51 UTC
Despite the rewrite in commit 145085109, multichannel support was missing in both the WASAPI source and sink elements. It should hopefully work with these patches, but I have not tested anything beyond mono and stereo because I do not have any multichannel devices. The second patch implements a probe-only device provider for the source and sink elements. This has been tested and works quite well. Note that WASAPI does not handle device state changes for clients transparently, and these elements do not implement device hotplug yet. Hence, device monitor support is also missing right now.
Created attachment 367418 [details] [review] wasapi: Implement support for >2 channels We need to parse the WAVEFORMATEXTENSIBLE structure, figure out what positions the channels have (if they are positional), and reorder them as necessary.
Created attachment 367419 [details] [review] wasapi: Implement a device provider for probing Currently only does probing and does not handle messages from endpoints/devices. In the future we want to do proper monitoring which is well-supported in WASAPI.
Review of attachment 367418 [details] [review]: Mostly looks good ::: sys/wasapi/gstwasapisink.c @@ +182,3 @@ } + g_clear_pointer (&self->positions, g_free); You could also make that a static 64-element array fwiw ::: sys/wasapi/gstwasapiutil.c @@ +212,3 @@ +static IMMDeviceEnumerator* +gst_wasapi_util_get_device_enumerator (GstElement * element) This and the related changes look like something that should've been in the next commit? @@ +423,3 @@ + GST_INFO ("wasapi: got %i channels from wasapi, assuming non-positional", + nChannels); + mask = GST_AUDIO_CHANNEL_POSITION_NONE; mask = NONE makes no sense. All positions NONE would be a mask of 0 @@ +430,3 @@ + * i.e. the speaker positions and the enum values are the same till + * 0x100 = SPEAKER_BACK_CENTER = GST_AUDIO_CHANNEL_POSITION_REAR_CENTER + * Hence if those are the only bits set, we can just use the mask directly. */ Considering the code further below, it might be easiest to just define a mapping table between WASAPI and GStreamer, and use that for all cases. Now you special-case <= 9 channels in a weird way @@ +444,3 @@ + "mask (%x), assuming non-positional channels, please report a bug", + nChannels, (guint) dwChannelMask); + mask = GST_AUDIO_CHANNEL_POSITION_NONE; And here
Review of attachment 367419 [details] [review]: Looks good to me ::: sys/wasapi/gstwasapidevice.c @@ +57,3 @@ +gst_wasapi_device_provider_finalize (GObject * object) +{ + CoUninitialize (); I hope these functions are refcounted :)
(In reply to Sebastian Dröge (slomo) from comment #3) > Review of attachment 367418 [details] [review] [review]: > > ::: sys/wasapi/gstwasapiutil.c > @@ +212,3 @@ > > +static IMMDeviceEnumerator* > +gst_wasapi_util_get_device_enumerator (GstElement * element) > > This and the related changes look like something that should've been in the > next commit? > Oops, yes. > @@ +423,3 @@ > + GST_INFO ("wasapi: got %i channels from wasapi, assuming > non-positional", > + nChannels); > + mask = GST_AUDIO_CHANNEL_POSITION_NONE; > > mask = NONE makes no sense. All positions NONE would be a mask of 0 > Yes, not sure what I was thinking there... > @@ +430,3 @@ > + * i.e. the speaker positions and the enum values are the same till > + * 0x100 = SPEAKER_BACK_CENTER = GST_AUDIO_CHANNEL_POSITION_REAR_CENTER > + * Hence if those are the only bits set, we can just use the mask > directly. */ > > Considering the code further below, it might be easiest to just define a > mapping table between WASAPI and GStreamer, and use that for all cases. Now > you special-case <= 9 channels in a weird way > I couldn't immediately think of a clean way to also populate the positions array at the same time without having a bunch of ugly macros. Do you have suggestions?
(In reply to Sebastian Dröge (slomo) from comment #4) > Review of attachment 367419 [details] [review] [review]: > > Looks good to me > > ::: sys/wasapi/gstwasapidevice.c > @@ +57,3 @@ > +gst_wasapi_device_provider_finalize (GObject * object) > +{ > + CoUninitialize (); > > I hope these functions are refcounted :) These functions initialize thread-local data, and the uninitialize has the same behaviour as being ref-counted (must uninit the same number of times you've init)
(In reply to Nirbheek Chauhan from comment #5) > > Considering the code further below, it might be easiest to just define a > > mapping table between WASAPI and GStreamer, and use that for all cases. Now > > you special-case <= 9 channels in a weird way > > > > I couldn't immediately think of a clean way to also populate the positions > array at the same time without having a bunch of ugly macros. Do you have > suggestions? You just do a table struct { guint64 wasapi_enum; GstAudioChannelPosition gst_enum; } positions[] = { { WASAPI_FRONT_LEFT, GST_FRONT_LEFT }, ... }; And then iterate over that to find the correct matches and build your channel positions array, channel mask, etc.
Created attachment 367663 [details] [review] wasapi: Implement support for >2 channels
Created attachment 367664 [details] [review] wasapi: Implement a device provider for probing
Created attachment 367665 [details] [review] wasapi: Correctly set ringbuffer segsize/segtotal This patch fixes a bug where we wouldn't use the same period (latency_time) as WASAPI, which is problematic while reading and writing because the API does not allow us to read or write a subset of bytes. Till we can use IAudioClient3 to actually set/get the device period, we now ignore the latency-time/buffer-time settings on the elements.
Attachment 367663 [details] pushed as d6d3106 - wasapi: Implement support for >2 channels Attachment 367664 [details] pushed as ec6a10e - wasapi: Implement a device provider for probing Attachment 367665 [details] pushed as 538ccb6 - wasapi: Correctly set ringbuffer segsize/segtotal