Expand CubebDeviceEnumerator to enumerate output devices

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
Rank:
15
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: achronop, Assigned: achronop)

Tracking

(Depends on 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
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
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.

Updated

8 months ago
Rank: 15
Priority: -- → P2
(Assignee)

Comment 2

7 months ago
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?
It's already thread safe isn't it? There is a lock for all methods and member access.
(Assignee)

Comment 4

7 months ago
Yes it is, my point is that we could avoid that.
(Assignee)

Updated

7 months ago
Assignee: nobody → achronop
(Assignee)

Updated

6 months ago
Depends on: 1498242
Attachment #9015821 - Attachment description: Bug 1482150 - Add cubeb instances for input and output. → Bug 1482150 - Separate notification callbacks for input and output.
(Assignee)

Updated

6 months ago
Depends on: 1500468
(Assignee)

Comment 10

6 months ago
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.
Attachment #9015821 - Attachment is obsolete: true
Attachment #9014022 - Attachment description: Bug 1482150 - Move enumerator to a separate file. → Bug 1482150 - Move CubebDeviceEnumerator to its own source and header files. r?padenot
Attachment #9014023 - Attachment description: Bug 1482150 - Make enumerator singleton. → Bug 1482150 - Make CubebDeviceEnumerator singleton. r?padenot
Attachment #9014024 - Attachment description: Bug 1482150 - Add output device enumeration. → Bug 1482150 - Implement enumeration of output devices. r?padenot
(Assignee)

Comment 11

6 months ago
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.
(Assignee)

Updated

6 months ago
See Also: → 1500909
Attachment #9019014 - Attachment description: Bug 1482150 - Change helper method to avoid corner cases when device list is empty. r?padenot → Bug 1482150 - Create a unit test to verify all corner cases of the helper method. r?padenot

Comment 13

6 months ago
Pushed by achronopoulos@mozilla.com:
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
(Assignee)

Comment 16

5 months ago
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

5 months ago
Pushed by achronopoulos@mozilla.com:
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
(Assignee)

Comment 19

5 months ago
Clearing NI
Flags: needinfo?(achronop)
(Assignee)

Updated

5 months ago
See Also: → 1499871
You need to log in before you can comment on or make changes to this bug.