Closed Bug 1497573 Opened 6 years ago Closed 6 years ago

Remove DesktopCapture::Stop

Categories

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

63 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: dminor, Assigned: dminor)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Assignee: nobody → 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
Flags: needinfo?(apehrson)
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/
Blocks: 1307254
Flags: needinfo?(apehrson)
Status: NEW → ASSIGNED
Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0c38b0d2117aff9e23003994ef206d10ad94d22 I also did manual testing using the fiddle that Andreas found.
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
Pushed by dminor@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
No longer depends on: 1646904
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: