[desktopCapture Mac]reorder execution order in start/release processing
This cl is to move the RegisterRefreshAndMoveHandlers to be done on
capture thread, and reverse some execution order of releasing processing,
also remove a lock since the handler is on capturing thread too.
As we doubt the existing sequence may be the cause of a crash due to
race conditions at end of capture.
Bug: chromium:851883
Change-Id: I2254a69815144415424a77b4c82f150cfc369585
Reviewed-on: https://webrtc-review.googlesource.com/83822
Commit-Queue: Brave Yao <braveyao@webrtc.org>
Reviewed-by: Zijie He <zijiehe@chromium.org>
Cr-Commit-Position: refs/heads/master@{#23647}diff --git a/modules/desktop_capture/mac/desktop_frame_provider.h b/modules/desktop_capture/mac/desktop_frame_provider.h
index b2abca8..7e17271 100644
--- a/modules/desktop_capture/mac/desktop_frame_provider.h
+++ b/modules/desktop_capture/mac/desktop_frame_provider.h
@@ -18,7 +18,7 @@
#include <memory>
#include "modules/desktop_capture/shared_desktop_frame.h"
-#include "rtc_base/synchronization/rw_lock_wrapper.h"
+#include "rtc_base/thread_checker.h"
#include "sdk/objc/Framework/Classes/Common/scoped_cftyperef.h"
namespace webrtc {
@@ -44,11 +44,9 @@
void Release();
private:
+ rtc::ThreadChecker thread_checker_;
const bool allow_iosurface_;
- // A lock protecting |io_surfaces_| across threads.
- const std::unique_ptr<RWLockWrapper> io_surfaces_lock_;
-
// Most recent IOSurface that contains a capture of matching display.
std::map<CGDirectDisplayID, std::unique_ptr<SharedDesktopFrame>> io_surfaces_;
diff --git a/modules/desktop_capture/mac/desktop_frame_provider.mm b/modules/desktop_capture/mac/desktop_frame_provider.mm
index 8417227..4234315 100644
--- a/modules/desktop_capture/mac/desktop_frame_provider.mm
+++ b/modules/desktop_capture/mac/desktop_frame_provider.mm
@@ -18,24 +18,25 @@
namespace webrtc {
DesktopFrameProvider::DesktopFrameProvider(bool allow_iosurface)
- : allow_iosurface_(allow_iosurface), io_surfaces_lock_(RWLockWrapper::CreateRWLock()) {}
+ : allow_iosurface_(allow_iosurface) {
+ thread_checker_.DetachFromThread();
+}
DesktopFrameProvider::~DesktopFrameProvider() {
- // Might be called from a thread which is not the one running the CGDisplayStream
- // handler. Indeed chromium's content destroys it from a dedicated thread.
+ RTC_DCHECK(thread_checker_.CalledOnValidThread());
+
Release();
}
std::unique_ptr<DesktopFrame> DesktopFrameProvider::TakeLatestFrameForDisplay(
CGDirectDisplayID display_id) {
+ RTC_DCHECK(thread_checker_.CalledOnValidThread());
+
if (!allow_iosurface_) {
// Regenerate a snapshot.
return DesktopFrameCGImage::CreateForDisplay(display_id);
}
- // Might be called from a thread which is not the one running the CGDisplayStream
- // handler. Indeed chromium's content uses a dedicates thread.
- WriteLockScoped scoped_io_surfaces_lock(*io_surfaces_lock_);
if (io_surfaces_[display_id]) {
return io_surfaces_[display_id]->Share();
}
@@ -45,6 +46,8 @@
void DesktopFrameProvider::InvalidateIOSurface(CGDirectDisplayID display_id,
rtc::ScopedCFTypeRef<IOSurfaceRef> io_surface) {
+ RTC_DCHECK(thread_checker_.CalledOnValidThread());
+
if (!allow_iosurface_) {
return;
}
@@ -52,19 +55,18 @@
std::unique_ptr<DesktopFrameIOSurface> desktop_frame_iosurface =
DesktopFrameIOSurface::Wrap(io_surface);
- // Call from the thread which runs the CGDisplayStream handler.
- WriteLockScoped scoped_io_surfaces_lock(*io_surfaces_lock_);
io_surfaces_[display_id] = desktop_frame_iosurface ?
SharedDesktopFrame::Wrap(std::move(desktop_frame_iosurface)) :
nullptr;
}
void DesktopFrameProvider::Release() {
+ RTC_DCHECK(thread_checker_.CalledOnValidThread());
+
if (!allow_iosurface_) {
return;
}
- WriteLockScoped scoped_io_surfaces_lock(*io_surfaces_lock_);
io_surfaces_.clear();
}
diff --git a/modules/desktop_capture/mac/screen_capturer_mac.mm b/modules/desktop_capture/mac/screen_capturer_mac.mm
index ebe77c8..ae6c47c 100644
--- a/modules/desktop_capture/mac/screen_capturer_mac.mm
+++ b/modules/desktop_capture/mac/screen_capturer_mac.mm
@@ -234,11 +234,7 @@
desktop_config_monitor_->Lock();
desktop_config_ = desktop_config_monitor_->desktop_configuration();
desktop_config_monitor_->Unlock();
- if (!RegisterRefreshAndMoveHandlers()) {
- RTC_LOG(LS_ERROR) << "Failed to register refresh and move handlers.";
- return false;
- }
- ScreenConfigurationChanged();
+
return true;
}
@@ -256,6 +252,13 @@
"webrtc", "ScreenCapturermac::Start", "target display id ", current_display_);
callback_ = callback;
+ // Start and operate CGDisplayStream handler all from capture thread.
+ if (!RegisterRefreshAndMoveHandlers()) {
+ RTC_LOG(LS_ERROR) << "Failed to register refresh and move handlers.";
+ callback_->OnCaptureResult(Result::ERROR_PERMANENT, nullptr);
+ return;
+ }
+ ScreenConfigurationChanged();
}
void ScreenCapturerMac::CaptureFrame() {
@@ -273,7 +276,11 @@
// structures. Occasionally, the refresh and move handlers are lost when
// the screen mode changes, so re-register them here.
UnregisterRefreshAndMoveHandlers();
- RegisterRefreshAndMoveHandlers();
+ if (!RegisterRefreshAndMoveHandlers()) {
+ RTC_LOG(LS_ERROR) << "Failed to register refresh and move handlers.";
+ callback_->OnCaptureResult(Result::ERROR_PERMANENT, nullptr);
+ return;
+ }
ScreenConfigurationChanged();
}
@@ -554,10 +561,9 @@
}
void ScreenCapturerMac::UnregisterRefreshAndMoveHandlers() {
+ display_stream_manager_->UnregisterActiveStreams();
// Release obsolete io surfaces.
desktop_frame_provider_.Release();
-
- display_stream_manager_->UnregisterActiveStreams();
}
void ScreenCapturerMac::ScreenRefresh(CGDirectDisplayID display_id,
