1436780 - Apparent device identifier mismatch between MediaEngineWebRTC/CamerasChild and CamerasParent

Status: NEW

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

Description

[Andreas Pehrson [:pehrsons]]

There's a fundamental mismatch between the child processes' and the parent process' way of handling devices.

A MediaEngineRemoteVideoSource stores an integer ID in the child that's assigned by MediaEngineWebRTC, during enumeration [1]. This ID is in fact just the index of this device in the range [0..numDevices] [2].

This is fine if the list of devices is static, but it is not. A user may plug in a new device or unplug a connected device at any time. Even between call asking CamerasParent for the number of devices and any of the followup calls to CamerasParent to get capabilities for a particular device.

In CamerasParent we have a couple of maps mapping from this identifier to various things, like a uniqueID string, or the currently chosen capability [3].


Now imagine you have a camera A connected to your computer when you participate in a video meeting. This will in the child create a MediaEngineRemoteVideoSource with mID 0 per [1] and [2].

At some later point the user then disconnects camera A and connects another camera B. The next child coming along to participate in a video meeting will also create a MediaEngineRemoteVideoSource with mID 0. That's fine for the child, but in CamerasParent id 0 already exists with old, obsolete state for camera A.

Modify this timeline as you wish to explode this into a multitude of interesting outcomes.

At the core the fact that each child is an authority over which id to assign a device and communicate to the parent is a fundamental flaw.

What's worse is that the protocol PCameras [4] supports this behavior and must have been written on an assumption that devices reported to a child never go away, i.e., the list of devices can only grow and disconnected devices are kept forever. In any case that's no longer true.


[1] https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/media/webrtc/MediaEngineWebRTC.cpp#252
[2] https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/media/webrtc/MediaEngineWebRTC.cpp#191
[3] https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/media/systemservices/CamerasParent.cpp#42-43
[4] https://searchfox.org/mozilla-central/source/dom/media/systemservices/PCameras.ipdl

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Sorry to confuse you.
I should have refactored the code or add more comments to prevent confusion.
Actually, there is no need to init mCaptureIndex in MediaEngineRemoteVideoSource ctor anymore. Since long time ago (before my multi-e10s patches), mCaptureIndex has been assigned from VideoEngine (in chrome process) through CamerasChild::AllocateCaptureDevice [2]. VideoEngine assigns a universal id [3] for each CreateVideoCapture call, which should let us avoid the situation you mentioned. jib knows more detail and intention about this change.

[1] https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#37

[2] https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#200-203

[3] https://searchfox.org/mozilla-central/source/dom/media/systemservices/VideoEngine.cpp#226-230

Comment 2

[Andreas Pehrson [:pehrsons]]

Aha, thanks! That is very confusing indeed.

I'll keep the bug for cleaning this up.

P3 based on the info in comment 1.