1482150 - Expand CubebDeviceEnumerator to enumerate output devices

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P2)

Description

[Alex Chronopoulos [:achronop]]

CubebDeviceEnumerator [0] is the new way to enumerate input devices. An important improvement is that it does not do unnecessary enumerations, it saves a local device list until it has been notified that the list has changed.

It would be great to expand CubebDeviceEnumerator to enumerate output devices too. We already hold an instance of the enumerator in MediaEngineWebRTC [1] so we could use it to get output devices too.

However, it has some limitation currently. The enumerator makes use of the static cubeb context [2], which means we can have only one instance of the enumerator per process. This is because every new instantiation will replace the device collection callback pointer, inside cubeb, and the logic of reusing the local device list will work only for the last one.

A second issue, with the output devices in place, is that the saved list will be invalidated for either input or output device collection changes. This will not be very accurate, it's not a big problem though.

With this bug I would like to create a logic that the CubebDeviceEnumerator will own its own cubeb context instance instead of reusing the static one from CubebUtils. We could even have two cubeb context instances, one for input and one for output, to avoid the problem above. Cubeb context is cheap enough and I do not see any problem using extra instances. However, it would be important to respect the "media.cubeb.force_null_context" pref for every new instance, in order to have accurate results on testing.

[0] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/dom/media/webrtc/MediaEngineWebRTC.h#127
[1] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/dom/media/webrtc/MediaEngineWebRTC.h#482
[2] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/dom/media/CubebUtils.cpp#125
[3] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/dom/media/CubebUtils.cpp#45

Comment 1

[Paul Adenot (:padenot)]

It would probably be better to have a more central CubebDeviceEnumerator:
- Putting it in its own file, making it a singleton.
- Allow multiple consumer to subscribe to device list notification callback in Gecko.
- Using it from MediaEngineWebRTC and also other locations.
- Possibly adding a feature where we invalidate either the input or output device list or both.

Comment 2

[Alex Chronopoulos [:achronop]]

If we make it a singleton we need to make it thread safe. We can avoid that if we keep it as a "simple" object which creates internally it's own cubeb instances. Do you prefer that?

Comment 3

[Paul Adenot (:padenot)]

It's already thread safe isn't it? There is a lock for all methods and member access.

Comment 4

[Alex Chronopoulos [:achronop]]

Yes it is, my point is that we could avoid that.

Comment 5

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1482150 - Move CubebDeviceEnumerator to its own source and header files. r?padenot

WIP patch

Comment 6

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1482150 - Make CubebDeviceEnumerator singleton. r?padenot

WIP patch

Comment 7

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1482150 - Implement enumeration of output devices. r?padenot

WIP patch. Affected by https://github.com/djg/audioipc-2/issues/50

Comment 8

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=203043380&revision=263ec12593942ef1a49e89a58bf44888d7ab78a1

Comment 9

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1482150 - Separate notification callbacks for input and output.

WIP patch

Comment 10

[Alex Chronopoulos [:achronop]]

In Linux _and_ when cubeb remote is on, we are affected by https://github.com/djg/audioipc-2/issues/50. Bug 1500468 creates a temporary solution and allow enumerator to enumerate every time. When audioipc issue is solved enumerator will be optimized to enumerate only after a device collection change.

Comment 11

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1482150 - Create a unit test to verify all corner cases of the helper method. r?padenot

Quering the stored devices list for an ID was not always correct. The stored devices list is expected to be empty before an enumeration takes place or after a collection change notification. In those cases, the method will fail to find an existing ID. The method has been updated to accept the device list as an argument. The device list can be created from the input and/or output device enumeration. Currently, the method is not being used.

Comment 12

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31f71194dbb4f8e7f0899b300377d291c13d7402

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ade887b7ba31
Move CubebDeviceEnumerator to its own source and header files. r=padenot
https://hg.mozilla.org/integration/autoland/rev/18925888f5d7
Make CubebDeviceEnumerator singleton. r=padenot
https://hg.mozilla.org/integration/autoland/rev/20a4fa9f6214
Implement enumeration of output devices. r=padenot
https://hg.mozilla.org/integration/autoland/rev/662bbd0961e5
Create a unit test to verify all corner cases of the helper method. r=padenot

Comment 14

[Cosmin Sabou [:CosminS]]

Backed out for causing windows build bustages on dom/media/CubebUtils.cpp

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=busted&fromchange=662bbd0961e58ad88e840c079f0a01fedb1ee035&tochange=a8b6a0a000c049313ed103512bf975a3a81693fa&selectedJob=210375013

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=210375039&repo=autoland&lineNumber=41807

Backout link: https://hg.mozilla.org/integration/autoland/rev/a8b6a0a000c049313ed103512bf975a3a81693fa

Comment 15

[Alex Chronopoulos [:achronop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8376343e4f9a6925e130c057817dbb42b681e0d

Comment 16

[Alex Chronopoulos [:achronop]]

The problem was that Windows MinGW 32/64 builds have WebRTc disabled due to Bug 1393901. A green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3980b0b16e0d81e61beef708dae7e618335ff91c

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22d08a6ea7cc
Move CubebDeviceEnumerator to its own source and header files. r=padenot
https://hg.mozilla.org/integration/autoland/rev/8a075dabb0c5
Make CubebDeviceEnumerator singleton. r=padenot
https://hg.mozilla.org/integration/autoland/rev/79ffde86ea1f
Implement enumeration of output devices. r=padenot
https://hg.mozilla.org/integration/autoland/rev/15bffbe82ec6
Create a unit test to verify all corner cases of the helper method. r=padenot

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/22d08a6ea7cc
https://hg.mozilla.org/mozilla-central/rev/8a075dabb0c5
https://hg.mozilla.org/mozilla-central/rev/79ffde86ea1f
https://hg.mozilla.org/mozilla-central/rev/15bffbe82ec6

Comment 19

[Alex Chronopoulos [:achronop]]

Clearing NI