1497573 - Remove DesktopCapture::Stop

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

We've added a Stop() method to DesktopCapturer and each of its subclasses to handle cleaning up state when capture ends. Rather than changing the webrtc.org code, we should find a way to accomplish the same thing in our own code.

Comment 1

[Dan Minor [:dminor]]

As far as I can tell, the reason to add Stop was to make it possible to call Start() multiple times on the same capturer. This is disallowed by debug assertions in the upstream code, e.g. [1]. I think the fix would be to lazily create the capturer instance when Start() is called and then free it when StopCapture() is called [2].

Before I do that, it kind of looks like when we call Stop(), we call Deallocate() right afterwards anyway [3], so I'm wondering if it is worth making the change to lazily create the capturer. Andreas, what do you think?

[1] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_x11.cc#183
[2] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc#645
[3] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/dom/media/MediaManager.cpp#4336

Comment 2

[Andreas Pehrson [:pehrsons]]

I think doing the lazy change makes sense if we keep it in our own code. I'll just note that there's already some lazy handling in DesktopCaptureImpl that we should also lift out to our own code.

Looking at whether this would be a wasted effort, I think we need to look at how the capturer is handled on the parent side rather than in the child, since it's a shared resource in the parent, [4]. It is true that we release the capturer in the parent after a call to Deallocate() in the child [5], but Stop() and Deallocate() in the child result in separate IPC calls to StopCapture() and ReleaseCaptureDevice() in the parent. That is then highly susceptible to racing, both in one child with multiple requests, and across children.

So to answer the question. No, we can't get away without handling this.
Maybe the manual test case on bug 1307254 [6] can be of help in testing and verifying the changes.


Since it took a small effort to track down the bug that added Stop() I've linked this as a dependency.


[4] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/dom/media/systemservices/CamerasParent.cpp#1001
[5] https://searchfox.org/mozilla-central/rev/d35199ef15ffa57d190886e20d5df6471faf0870/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#236
[6] https://jsfiddle.net/jib1/9g47pp22/

Comment 3

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c38b0d2117aff9e23003994ef206d10ad94d22

I also did manual testing using the fiddle that Andreas found.

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1497573 - Remove DesktopCapturer::Stop; r=ng!

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1497573 - Create desktop_capturer_cursor_composer_ lazily; r=ng!

The webrtc.org capturer implementations do not allow Start to be called more
than once. Previously we worked around this by adding a Stop method that was
called from StopCapture. With this change, we instead free the capturer in
StopCapture and create or re-create it as needed from StartCapture or
FocusOnSelectedSource.

Depends on D14066

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d9c607f2619
Remove DesktopCapturer::Stop; r=ng
https://hg.mozilla.org/integration/autoland/rev/c63b8de37d6a
Create desktop_capturer_cursor_composer_ lazily; r=ng

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0d9c607f2619
https://hg.mozilla.org/mozilla-central/rev/c63b8de37d6a