Lazily initialize ScreenManager connection

This enables us to replace usage of mojo::SharedRemote and the thread
pool usage with an ordinary mojo::Remote. This should be a performance
improvement as it eliminates a hop through the thread pool.

Adds usage of sequence checker to assert thread safety expectations.

Change-Id: I93a681a2a6543fed417dcffbd0b43da121caddf5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439559Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Darin Fisher <darin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812414}

content/browser/media/capture/desktop_capturer_lacros.cc

@@ -16,26 +16,17 @@ DesktopCapturerLacros::DesktopCapturerLacros(
     CaptureType capture_type,
     const webrtc::DesktopCaptureOptions& options)
     : capture_type_(capture_type), options_(options) {
-  mojo::PendingRemote<crosapi::mojom::ScreenManager> pending_screen_manager;
-  mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver =
-      pending_screen_manager.InitWithNewPipeAndPassReceiver();
-
-  // The lacros chrome service exists at all times except during early start-up
-  // and late shut-down. This class should never be used in those two times.
-  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
-  DCHECK(lacros_chrome_service);
-  lacros_chrome_service->BindScreenManagerReceiver(std::move(pending_receiver));
-
-  // We create a SharedRemote that binds the underlying Remote onto a
-  // dedicated sequence.
-  screen_manager_ = mojo::SharedRemote<crosapi::mojom::ScreenManager>(
-      std::move(pending_screen_manager),
-      base::ThreadPool::CreateSequencedTaskRunner({}));
+  // Allow this class to be constructed on any sequence.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
-DesktopCapturerLacros::~DesktopCapturerLacros() = default;
+DesktopCapturerLacros::~DesktopCapturerLacros() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
 
 bool DesktopCapturerLacros::GetSourceList(SourceList* sources) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (capture_type_ == kScreen) {
     // TODO(https://crbug.com/1094460): Implement this source list
     // appropriately.
@@ -45,6 +36,8 @@ bool DesktopCapturerLacros::GetSourceList(SourceList* sources) {
     return true;
   }
 
+  EnsureScreenManager();
+
   std::vector<crosapi::mojom::WindowDetailsPtr> windows;
   {
     mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
@@ -61,6 +54,11 @@ bool DesktopCapturerLacros::GetSourceList(SourceList* sources) {
 }
 
 bool DesktopCapturerLacros::SelectSource(SourceId id) {
+#if DCHECK_IS_ON()
+  // OK to modify on any thread prior to calling Start.
+  DCHECK(!callback_ || sequence_checker_.CalledOnValidSequence());
+#endif
+
   selected_source_ = id;
   return true;
 }
@@ -70,10 +68,15 @@ bool DesktopCapturerLacros::FocusOnSelectedSource() {
 }
 
 void DesktopCapturerLacros::Start(Callback* callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   callback_ = callback;
 }
 
 void DesktopCapturerLacros::CaptureFrame() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  EnsureScreenManager();
+
   if (capture_type_ == kScreen) {
     crosapi::Bitmap snapshot;
     {
@@ -104,8 +107,24 @@ void DesktopCapturerLacros::SetSharedMemoryFactory(
 
 void DesktopCapturerLacros::SetExcludedWindow(webrtc::WindowId window) {}
 
+void DesktopCapturerLacros::EnsureScreenManager() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (screen_manager_)
+    return;
+
+  // The lacros chrome service exists at all times except during early start-up
+  // and late shut-down. This class should never be used in those two times.
+  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
+  DCHECK(lacros_chrome_service);
+  lacros_chrome_service->BindScreenManagerReceiver(
+      screen_manager_.BindNewPipeAndPassReceiver());
+}
+
 void DesktopCapturerLacros::DidTakeSnapshot(bool success,
                                             const crosapi::Bitmap& snapshot) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!success) {
     callback_->OnCaptureResult(Result::ERROR_PERMANENT,
                                std::unique_ptr<webrtc::DesktopFrame>());

content/browser/media/capture/desktop_capturer_lacros.h

@@ -5,9 +5,9 @@
 #ifndef CONTENT_BROWSER_MEDIA_CAPTURE_DESKTOP_CAPTURER_LACROS_H_
 #define CONTENT_BROWSER_MEDIA_CAPTURE_DESKTOP_CAPTURER_LACROS_H_
 
-#include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
 #include "chromeos/crosapi/mojom/screen_manager.mojom.h"
-#include "mojo/public/cpp/bindings/shared_remote.h"
+#include "mojo/public/cpp/bindings/remote.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capture_options.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_capturer.h"
 
@@ -18,11 +18,13 @@ struct Bitmap;
 namespace content {
 
 // This class is responsible for communicating with ash-chrome to get snapshots
-// of the desktop. This class is used on several different threads with no clear
-// signaling. This is a contextual requirement of the current implementation of
-// the media capture code. We do our best to:
-//   * Minimize state stored in this class.
-//   * Ensure that stored state is accessed safely.
+// of the desktop.
+//
+// NOTE: Instances of this class may be allocated and configured on one affine
+// sequence and then transferred to another affine sequence (e.g., a worker
+// thread) where |Start| gets called. Subsequent methods are allowed to do
+// blocking I/O or other expensive operations. The instance, when no longer
+// needed, is deleted on the same affine sequence on which |Start| was called.
 class DesktopCapturerLacros : public webrtc::DesktopCapturer {
  public:
   enum CaptureType { kScreen, kWindow };
@@ -44,10 +46,16 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
   void SetExcludedWindow(webrtc::WindowId window) override;
 
  private:
+  // This method is used to lazily initialize screen_manager_ on the same
+  // affine sequence on which |Start| is called.
+  void EnsureScreenManager();
+
   // Callback for when ash-chrome returns a snapshot of the screen or window as
   // a bitmap.
   void DidTakeSnapshot(bool success, const crosapi::Bitmap& snapshot);
 
+  SEQUENCE_CHECKER(sequence_checker_);
+
   // Whether this object is capturing screens or windows.
   const CaptureType capture_type_;
 
@@ -67,10 +75,8 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
   // Thus, we do not worry about thread safety when invoking callback_.
   Callback* callback_ = nullptr;
 
-  // This remote is thread safe. Callbacks are invoked on the calling sequence.
-  mojo::SharedRemote<crosapi::mojom::ScreenManager> screen_manager_;
-
-  base::WeakPtrFactory<DesktopCapturerLacros> weak_factory_{this};
+  // The remote connection to the screen manager.
+  mojo::Remote<crosapi::mojom::ScreenManager> screen_manager_;
 };
 
 }  // namespace content