Allow OnCaptureResult to be called asynchronously

As part of this change, DesktopCaptureDevice will explicitly stop
capturing frames if it encounters a permanent error. This should be a
no-op since OnError implementations are supposed to destroy the
DesktopCapturer.

A test is added to confirm that the throttling logic still works
properly when the callbacks run asynchronously, and the Lacros
desktop capturer is modified to run OnCaptureResult asynchronously.

This CL is important because it sets the stage for further improvements
to the Lacros capturer. It will need to use mojo's QueryVersion method
to check for newer methods on ScreenManager that will enable more
optimal screen capture using shared memory. QueryVersion runs
asynchronously, which is why this CL is needed.

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

content/browser/media/capture/desktop_capture_device.cc

@@ -124,6 +124,7 @@ class DesktopCaptureDevice::Core : public webrtc::DesktopCapturer::Callback {
 
  private:
   // webrtc::DesktopCapturer::Callback interface.
+  // A side-effect of this method is to schedule the next frame.
   void OnCaptureResult(
     webrtc::DesktopCapturer::Result result,
     std::unique_ptr<webrtc::DesktopFrame> frame) override;
@@ -132,11 +133,12 @@ class DesktopCaptureDevice::Core : public webrtc::DesktopCapturer::Callback {
   // to capture a frame.
   void OnCaptureTimer();
 
-  // Captures a frame and schedules timer for the next one.
-  void CaptureFrameAndScheduleNext();
+  // Captures a frame. Upon completion, schedules the next frame.
+  void CaptureFrame();
 
-  // Captures a single frame.
-  void DoCapture();
+  // Schedules a timer for the next call to |CaptureFrame|. This method assumes
+  // that |CaptureFrame| has already been called at least once before.
+  void ScheduleNextCaptureFrame();
 
   void RequestWakeLock();
 
@@ -158,6 +160,9 @@ class DesktopCaptureDevice::Core : public webrtc::DesktopCapturer::Callback {
   // Inverse of the requested frame rate.
   base::TimeDelta requested_frame_duration_;
 
+  // Records time of last call to CaptureFrame.
+  base::TimeTicks capture_start_time_;
+
   // Size of frame most recently captured from the source.
   webrtc::DesktopSize previous_frame_size_;
 
@@ -258,7 +263,7 @@ void DesktopCaptureDevice::Core::AllocateAndStart(
   // Assume it will be always started successfully for now.
   client_->OnStarted();
 
-  CaptureFrameAndScheduleNext();
+  CaptureFrame();
 }
 
 void DesktopCaptureDevice::Core::SetNotificationWindowId(
@@ -310,7 +315,10 @@ void DesktopCaptureDevice::Core::OnCaptureResult(
       client_->OnError(media::VideoCaptureError::
                            kDesktopCaptureDeviceWebrtcDesktopCapturerHasFailed,
                        FROM_HERE, "The desktop capturer has failed.");
+      return;
     }
+    // Continue capturing frames in the temporary error case.
+    ScheduleNextCaptureFrame();
     return;
   }
   DCHECK(frame);
@@ -437,6 +445,8 @@ void DesktopCaptureDevice::Core::OnCaptureResult(
           requested_frame_rate_, media::PIXEL_FORMAT_ARGB),
       frame_color_space, 0 /* clockwise_rotation */, false /* flip_y */, now,
       now - first_ref_time_);
+
+  ScheduleNextCaptureFrame();
 }
 
 void DesktopCaptureDevice::Core::OnCaptureTimer() {
@@ -445,14 +455,24 @@ void DesktopCaptureDevice::Core::OnCaptureTimer() {
   if (!client_)
     return;
 
-  CaptureFrameAndScheduleNext();
+  CaptureFrame();
 }
 
-void DesktopCaptureDevice::Core::CaptureFrameAndScheduleNext() {
+void DesktopCaptureDevice::Core::CaptureFrame() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::TimeTicks started_time = NowTicks();
-  DoCapture();
-  base::TimeDelta last_capture_duration = NowTicks() - started_time;
+  DCHECK(!capture_in_progress_);
+
+  capture_start_time_ = NowTicks();
+  capture_in_progress_ = true;
+
+  desktop_capturer_->CaptureFrame();
+}
+
+void DesktopCaptureDevice::Core::ScheduleNextCaptureFrame() {
+  // Make sure CaptureFrame() was called at least once before.
+  DCHECK(!capture_start_time_.is_null());
+
+  base::TimeDelta last_capture_duration = NowTicks() - capture_start_time_;
 
   // Limit frame-rate to reduce CPU consumption.
   base::TimeDelta capture_period =
@@ -464,18 +484,6 @@ void DesktopCaptureDevice::Core::CaptureFrameAndScheduleNext() {
                         &Core::OnCaptureTimer);
 }
 
-void DesktopCaptureDevice::Core::DoCapture() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(!capture_in_progress_);
-
-  capture_in_progress_ = true;
-  desktop_capturer_->CaptureFrame();
-
-  // Currently only synchronous implementations of DesktopCapturer are
-  // supported.
-  DCHECK(!capture_in_progress_);
-}
-
 void DesktopCaptureDevice::Core::RequestWakeLock() {
   mojo::Remote<device::mojom::WakeLockProvider> wake_lock_provider;
   auto receiver = wake_lock_provider.BindNewPipeAndPassReceiver();

content/browser/media/capture/desktop_capture_device_unittest.cc

@@ -130,12 +130,8 @@ class UnpackedDesktopFrame : public webrtc::DesktopFrame {
 // TODO(sergeyu): Move this to a separate file where it can be reused.
 class FakeScreenCapturer : public webrtc::DesktopCapturer {
  public:
-  FakeScreenCapturer()
-      : callback_(nullptr),
-        frame_index_(0),
-        generate_inverted_frames_(false),
-        generate_cropped_frames_(false) {}
-  ~FakeScreenCapturer() override {}
+  FakeScreenCapturer() = default;
+  ~FakeScreenCapturer() override = default;
 
   void set_generate_inverted_frames(bool generate_inverted_frames) {
     generate_inverted_frames_ = generate_inverted_frames;
@@ -145,6 +141,10 @@ class FakeScreenCapturer : public webrtc::DesktopCapturer {
     generate_cropped_frames_ = generate_cropped_frames;
   }
 
+  void set_run_callback_asynchronously(bool run_callback_asynchronously) {
+    run_callback_asynchronously_ = run_callback_asynchronously;
+  }
+
   // VideoFrameCapturer interface.
   void Start(Callback* callback) override { callback_ = callback; }
 
@@ -164,19 +164,35 @@ class FakeScreenCapturer : public webrtc::DesktopCapturer {
     } else if (generate_cropped_frames_) {
       frame.reset(new UnpackedDesktopFrame(std::move(frame)));
     }
+
+    if (run_callback_asynchronously_) {
+      base::ThreadTaskRunnerHandle::Get()->PostTask(
+          FROM_HERE, base::BindOnce(&FakeScreenCapturer::RunCallback,
+                                    weak_factory_.GetWeakPtr(),
+                                    webrtc::DesktopCapturer::Result::SUCCESS,
+                                    std::move(frame)));
+    } else {
       callback_->OnCaptureResult(webrtc::DesktopCapturer::Result::SUCCESS,
                                  std::move(frame));
     }
+  }
 
   bool GetSourceList(SourceList* screens) override { return false; }
 
   bool SelectSource(SourceId id) override { return false; }
 
  private:
-  Callback* callback_;
-  int frame_index_;
-  bool generate_inverted_frames_;
-  bool generate_cropped_frames_;
+  void RunCallback(webrtc::DesktopCapturer::Result result,
+                   std::unique_ptr<webrtc::DesktopFrame> frame) {
+    callback_->OnCaptureResult(result, std::move(frame));
+  }
+
+  Callback* callback_ = nullptr;
+  int frame_index_ = 0;
+  bool generate_inverted_frames_ = false;
+  bool generate_cropped_frames_ = false;
+  bool run_callback_asynchronously_ = false;
+  base::WeakPtrFactory<FakeScreenCapturer> weak_factory_{this};
 };
 
 // Helper used to check that only two specific frame sizes are delivered to the
@@ -545,8 +561,10 @@ class DesktopCaptureDeviceThrottledTest : public DesktopCaptureDeviceTest {
   // Capture frames at kFrameRate for a duration of total_capture_duration and
   // return the throttled frame rate.
   double CaptureFrames() {
-    CreateScreenCaptureDevice(
-        std::unique_ptr<webrtc::DesktopCapturer>(new FakeScreenCapturer()));
+    auto capturer = std::make_unique<FakeScreenCapturer>();
+    capturer->set_run_callback_asynchronously(run_callback_asynchronously_);
+
+    CreateScreenCaptureDevice(std::move(capturer));
 
     FormatChecker format_checker(
         gfx::Size(kTestFrameWidth3, kTestFrameHeight3),
@@ -631,6 +649,8 @@ class DesktopCaptureDeviceThrottledTest : public DesktopCaptureDeviceTest {
 
     return nb_frames / kVirtualTestDurationSeconds.InSecondsF();
   }
+
+  bool run_callback_asynchronously_ = false;
 };
 
 // The test verifies that the capture pipeline is throttled as defined with
@@ -649,6 +669,24 @@ TEST_F(DesktopCaptureDeviceThrottledTest, ThrottledOn) {
   EXPECT_LE(actual_framerate, expected_framerate + 0.1);
 }
 
+// Same tests as above but runs callbacks asynchronously to verify that that
+// doesn't disrupt the throttling machinery.
+TEST_F(DesktopCaptureDeviceThrottledTest, ThrottledOn_Async) {
+  run_callback_asynchronously_ = true;
+
+  const double actual_framerate = CaptureFrames();
+
+  // By default when capturing a frame it is expected to do the actual device
+  // capture for at most half of a capture period. This is to ensure that the
+  // cpu is idle for at least 50% of the time, otherwise it will be throttled
+  // to reach this idle duration.
+  const int expected_framerate = kFrameRate / 2;
+
+  // The test succeeds if the actual framerate is near the expected_framerate.
+  EXPECT_GE(actual_framerate, expected_framerate);
+  EXPECT_LE(actual_framerate, expected_framerate + 0.1);
+}
+
 // The test verifies that the capture pipeline is not throttled when passing
 // --webrtc-max-cpu-consumption-percentage=100.
 TEST_F(DesktopCaptureDeviceThrottledTest, ThrottledOff) {

content/browser/media/capture/desktop_capturer_lacros.cc

@@ -78,23 +78,14 @@ void DesktopCapturerLacros::CaptureFrame() {
   EnsureScreenManager();
 
   if (capture_type_ == kScreen) {
-    crosapi::Bitmap snapshot;
-    {
-      // lacros-chrome is allowed to make sync calls to ash-chrome.
-      mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
-      screen_manager_->TakeScreenSnapshot(&snapshot);
-    }
-    DidTakeSnapshot(/*success=*/true, snapshot);
+    screen_manager_->TakeScreenSnapshot(
+        base::BindOnce(&DesktopCapturerLacros::DidTakeSnapshot,
+                       weak_factory_.GetWeakPtr(), /*success*/ true));
   } else {
-    bool success;
-    crosapi::Bitmap snapshot;
-    {
-      // lacros-chrome is allowed to make sync calls to ash-chrome.
-      mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync_call;
-      screen_manager_->TakeWindowSnapshot(selected_source_, &success,
-                                          &snapshot);
-    }
-    DidTakeSnapshot(success, snapshot);
+    screen_manager_->TakeWindowSnapshot(
+        selected_source_,
+        base::BindOnce(&DesktopCapturerLacros::DidTakeSnapshot,
+                       weak_factory_.GetWeakPtr()));
   }
 }
 
@@ -122,7 +113,7 @@ void DesktopCapturerLacros::EnsureScreenManager() {
 }
 
 void DesktopCapturerLacros::DidTakeSnapshot(bool success,
-                                            const crosapi::Bitmap& snapshot) {
+                                            crosapi::Bitmap snapshot) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   if (!success) {

content/browser/media/capture/desktop_capturer_lacros.h

@@ -5,6 +5,7 @@
 #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/remote.h"
@@ -52,7 +53,7 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
 
   // Callback for when ash-chrome returns a snapshot of the screen or window as
   // a bitmap.
-  void DidTakeSnapshot(bool success, const crosapi::Bitmap& snapshot);
+  void DidTakeSnapshot(bool success, crosapi::Bitmap snapshot);
 
   SEQUENCE_CHECKER(sequence_checker_);
 
@@ -77,6 +78,8 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
 
   // The remote connection to the screen manager.
   mojo::Remote<crosapi::mojom::ScreenManager> screen_manager_;
+
+  base::WeakPtrFactory<DesktopCapturerLacros> weak_factory_{this};
 };
 
 }  // namespace content