[Reland] capture_mode: Recording service lifetime and APIs

This is a reland of https://crrev.com/c/2536030, which was reverted
by mistake. The actual failure was caused by another CL
(https://crrev.com/c/2537939) which was also reverted.
No changes were made in this CL from the original.

TBR=jamescook@chromium.org, dcheng@chromium.org

Original CL description:
This is a follow-up CL to handle requests made on the recording
service CL (https://crrev.com/c/2404145):
- The Record*() APIs of the service should provide the needed client,
  video capturer, and audio stream factory, instead of exposing APIs
  in the client so that anyone can bind to those capturers.
- The service is launched to handle video recording, and terminated
  once recording ends, and all the chunks have been sent.
- A GPU crash is handled by ending the recording.

BUG=1126586, 1147989, 1143411
TEST=Manually, existing tests.

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536030Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#828574}
Change-Id: I747e6f74005b304e3954dc77615574ffe7a27cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2546857Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828943}

ash/capture_mode/capture_mode_controller.h

@@ -90,11 +90,6 @@ class ASH_EXPORT CaptureModeController
   void OpenFeedbackDialog();
 
   // recording::mojom::RecordingServiceClient:
-  void BindVideoCapturer(
-      mojo::PendingReceiver<viz::mojom::FrameSinkVideoCapturer> receiver)
-      override;
-  void BindAudioStreamFactory(
-      mojo::PendingReceiver<audio::mojom::StreamFactory> receiver) override;
   void OnMuxerOutput(const std::string& chunk) override;
   void OnRecordingEnded(bool success) override;
 
@@ -105,16 +100,6 @@ class ASH_EXPORT CaptureModeController
  private:
   friend class CaptureModeTestApi;
 
-  // Launches the mojo service that handles audio and video recording.
-  void LaunchRecordingService();
-
-  // Called back when the mojo pipe to the recording service gets disconnected.
-  void OnRecordingServiceDisconnected();
-
-  // Called to terminate |is_recording_in_progress_|, the stop-recording shelf
-  // pod button, and the |video_recording_watcher_| when recording ends.
-  void TerminateRecordingUiElements();
-
   // Returns the capture parameters for the capture operation that is about to
   // be performed (i.e. the window to be captured, and the capture bounds). If
   // nothing is to be captured (e.g. when there's no window selected in a
@@ -129,10 +114,22 @@ class ASH_EXPORT CaptureModeController
   };
   base::Optional<CaptureParams> GetCaptureParams() const;
 
+  // Launches the mojo service that handles audio and video recording, and
+  // begins recording according to the given |capture_params|.
+  void LaunchRecordingServiceAndStartRecording(
+      const CaptureParams& capture_params);
+
+  // Called back when the mojo pipe to the recording service gets disconnected.
+  void OnRecordingServiceDisconnected();
+
   // Returns true if doing a screen capture is currently allowed, false
   // otherwise.
   bool IsCaptureAllowed(const CaptureParams& capture_params) const;
 
+  // Called to terminate |is_recording_in_progress_|, the stop-recording shelf
+  // pod button, and the |video_recording_watcher_| when recording ends.
+  void TerminateRecordingUiElements();
+
   // The below functions start the actual image/video capture. They expect that
   // the capture session is still active when called, so they can retrieve the
   // capture parameters they need. They will end the sessions themselves.

ash/capture_mode/test_capture_mode_delegate.cc

@@ -26,18 +26,32 @@ class FakeRecordingService : public recording::mojom::RecordingService {
   }
 
   // mojom::RecordingService:
-  void SetClient(mojo::PendingRemote<recording::mojom::RecordingServiceClient>
-                     client) override {
+  void RecordFullscreen(
+      mojo::PendingRemote<recording::mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      const viz::FrameSinkId& frame_sink_id,
+      const gfx::Size& fullscreen_size) override {
+    remote_client_.Bind(std::move(client));
+  }
+  void RecordWindow(
+      mojo::PendingRemote<recording::mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      const viz::FrameSinkId& frame_sink_id,
+      const gfx::Size& initial_window_size,
+      const gfx::Size& max_window_size) override {
+    remote_client_.Bind(std::move(client));
+  }
+  void RecordRegion(
+      mojo::PendingRemote<recording::mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      const viz::FrameSinkId& frame_sink_id,
+      const gfx::Size& fullscreen_size,
+      const gfx::Rect& corp_region) override {
     remote_client_.Bind(std::move(client));
   }
-  void RecordFullscreen(const viz::FrameSinkId& frame_sink_id,
-                        const gfx::Size& fullscreen_size) override {}
-  void RecordWindow(const viz::FrameSinkId& frame_sink_id,
-                    const gfx::Size& initial_window_size,
-                    const gfx::Size& max_window_size) override {}
-  void RecordRegion(const viz::FrameSinkId& frame_sink_id,
-                    const gfx::Size& fullscreen_size,
-                    const gfx::Rect& corp_region) override {}
   void StopRecording() override {
     remote_client_->OnRecordingEnded(/*success=*/true);
     remote_client_.FlushForTesting();
@@ -51,8 +65,7 @@ class FakeRecordingService : public recording::mojom::RecordingService {
 // -----------------------------------------------------------------------------
 // TestCaptureModeDelegate:
 
-TestCaptureModeDelegate::TestCaptureModeDelegate()
-    : fake_service_(std::make_unique<FakeRecordingService>()) {}
+TestCaptureModeDelegate::TestCaptureModeDelegate() = default;
 
 TestCaptureModeDelegate::~TestCaptureModeDelegate() = default;
 
@@ -91,7 +104,8 @@ void TestCaptureModeDelegate::StopObservingRestrictedContent() {}
 void TestCaptureModeDelegate::OpenFeedbackDialog() {}
 
 mojo::Remote<recording::mojom::RecordingService>
-TestCaptureModeDelegate::LaunchRecordingService() const {
+TestCaptureModeDelegate::LaunchRecordingService() {
+  fake_service_ = std::make_unique<FakeRecordingService>();
   mojo::Remote<recording::mojom::RecordingService> service;
   fake_service_->Bind(service.BindNewPipeAndPassReceiver());
   return service;

ash/capture_mode/test_capture_mode_delegate.h

@@ -37,7 +37,7 @@ class TestCaptureModeDelegate : public CaptureModeDelegate {
   void StopObservingRestrictedContent() override;
   void OpenFeedbackDialog() override;
   mojo::Remote<recording::mojom::RecordingService> LaunchRecordingService()
-      const override;
+      override;
   void BindAudioStreamFactory(
       mojo::PendingReceiver<audio::mojom::StreamFactory> receiver) override;
 

ash/public/cpp/capture_mode_delegate.h

@@ -80,7 +80,7 @@ class ASH_PUBLIC_EXPORT CaptureModeDelegate {
 
   // Launches the Recording Service into a separate utility process.
   virtual mojo::Remote<recording::mojom::RecordingService>
-  LaunchRecordingService() const = 0;
+  LaunchRecordingService() = 0;
 
   // Binds the given audio StreamFactory |receiver| to the audio service.
   virtual void BindAudioStreamFactory(

ash/services/recording/public/mojom/recording_service.mojom

@@ -15,17 +15,6 @@ import "ui/gfx/geometry/mojom/geometry.mojom";
 // The recording services consumes this interface to communicate with the client
 // during video recording and to send over the video chunks.
 interface RecordingServiceClient {
-  // Called by the service to ask the client to create a video capturer for it
-  // via the client's access to the host FrameSinkManager.
-  // TODO(https://crbug.com/1147989): This will be removed.
-  BindVideoCapturer(
-      pending_receiver<viz.mojom.FrameSinkVideoCapturer> receiver);
-
-  // Called by the service to ask the client to bind the given pending audio
-  // StreamFactory receiver to the audio service.
-  // TODO(https://crbug.com/1147989): This will be removed.
-  BindAudioStreamFactory(pending_receiver<audio.mojom.StreamFactory> receiver);
-
   // Called repeatedly by the service while video recording is in progress,
   // to provide the client with final encoded and webm-muxed video and audio
   // frames. Frames will continue to be provided until OnRecordingEnded() is
@@ -46,11 +35,11 @@ interface RecordingServiceClient {
 // of it, or an individual window. The service captures, encodes, and muxes the
 // audio and video frames, and sends the WebM muxed video chunks to the client.
 // Note that a maximum of one screen recording can be done at any time.
-// TODO(https://crbug.com/1147989): This interface will change so that Record*()
-// APIs will take a client, a video capturer, and an audio stream factory.
 interface RecordingService {
-  // Sets the single client of this service.
-  SetClient(pending_remote<RecordingServiceClient> client);
+  // All the below Record*() interfaces, take a pending remote to a client (e.g.
+  // Ash) to which it will send the muxed video chunks, and two other pending
+  // remotes bound to the video capturer on Viz on the GPU process, and to the
+  // audio stream factory on the Audio Service respectively.
 
   // Starts a fullscreen recording of a root window which has the given
   // |frame_sink_id|. The resulting video will have a resolution equal to the
@@ -58,7 +47,11 @@ interface RecordingService {
   // is different than that of the captured source, the content will letterbox
   // or pillarbox as needed.
   // |frame_sink_id| must be valid.
-  RecordFullscreen(viz.mojom.FrameSinkId frame_sink_id,
+  RecordFullscreen(
+      pending_remote<RecordingServiceClient> client,
+      pending_remote<viz.mojom.FrameSinkVideoCapturer> video_capturer,
+      pending_remote<audio.mojom.StreamFactory> audio_stream_factory,
+      viz.mojom.FrameSinkId frame_sink_id,
       gfx.mojom.Size video_size);
 
   // Starts a recording of a window which has the given |frame_sink_id|.
@@ -69,7 +62,11 @@ interface RecordingService {
   // Note that if the aspect ratio of |max_video_size| is different than that of
   // the captured source, the content will letterbox  or pillarbox as needed.
   // |frame_sink_id| must be valid.
-  RecordWindow(viz.mojom.FrameSinkId frame_sink_id,
+  RecordWindow(
+      pending_remote<RecordingServiceClient> client,
+      pending_remote<viz.mojom.FrameSinkVideoCapturer> video_capturer,
+      pending_remote<audio.mojom.StreamFactory> audio_stream_factory,
+      viz.mojom.FrameSinkId frame_sink_id,
       gfx.mojom.Size initial_video_size,
       gfx.mojom.Size max_video_size);
 
@@ -78,7 +75,11 @@ interface RecordingService {
   // given |full_capture_size| in DIPs, but the resulting video frames will be
   // cropped to the given |crop_region| in DIPs.
   // |frame_sink_id| must be valid.
-  RecordRegion(viz.mojom.FrameSinkId frame_sink_id,
+  RecordRegion(
+      pending_remote<RecordingServiceClient> client,
+      pending_remote<viz.mojom.FrameSinkVideoCapturer> video_capturer,
+      pending_remote<audio.mojom.StreamFactory> audio_stream_factory,
+      viz.mojom.FrameSinkId frame_sink_id,
       gfx.mojom.Size full_capture_size,
       gfx.mojom.Rect crop_region);
 

ash/services/recording/recording_service.cc

@@ -26,11 +26,6 @@ namespace recording {
 
 namespace {
 
-// The amount of time to wait before attempting to reconnect to a new video
-// capturer in case we get disconnected.
-constexpr base::TimeDelta kReconnectDelay =
-    base::TimeDelta::FromMilliseconds(100);
-
 // For a capture size of 320 by 240, we use a bitrate of 256 kbit/s. Based on
 // that, we calculate the bits per second per squared pixel.
 constexpr uint64_t kMinBitrateInBitsPerSecond = 256 * 1000;
@@ -82,38 +77,50 @@ RecordingService::RecordingService(
 
 RecordingService::~RecordingService() = default;
 
-void RecordingService::SetClient(
-    mojo::PendingRemote<mojom::RecordingServiceClient> client) {
-  DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
-  client_remote_.Bind(std::move(client));
-}
-
-void RecordingService::RecordFullscreen(const viz::FrameSinkId& frame_sink_id,
-                                        const gfx::Size& video_size) {
+void RecordingService::RecordFullscreen(
+    mojo::PendingRemote<mojom::RecordingServiceClient> client,
+    mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+    mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+    const viz::FrameSinkId& frame_sink_id,
+    const gfx::Size& video_size) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
 
-  StartNewRecording(VideoCaptureParams::CreateForFullscreenCapture(
-      frame_sink_id, video_size));
+  StartNewRecording(std::move(client), std::move(video_capturer),
+                    std::move(audio_stream_factory),
+                    VideoCaptureParams::CreateForFullscreenCapture(
+                        frame_sink_id, video_size));
 }
 
-void RecordingService::RecordWindow(const viz::FrameSinkId& frame_sink_id,
-                                    const gfx::Size& initial_video_size,
-                                    const gfx::Size& max_video_size) {
+void RecordingService::RecordWindow(
+    mojo::PendingRemote<mojom::RecordingServiceClient> client,
+    mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+    mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+    const viz::FrameSinkId& frame_sink_id,
+    const gfx::Size& initial_video_size,
+    const gfx::Size& max_video_size) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
 
   // TODO(crbug.com/1143930): Window recording doesn't produce any frames at the
   // moment.
-  StartNewRecording(VideoCaptureParams::CreateForWindowCapture(
-      frame_sink_id, initial_video_size, max_video_size));
+  StartNewRecording(std::move(client), std::move(video_capturer),
+                    std::move(audio_stream_factory),
+                    VideoCaptureParams::CreateForWindowCapture(
+                        frame_sink_id, initial_video_size, max_video_size));
 }
 
-void RecordingService::RecordRegion(const viz::FrameSinkId& frame_sink_id,
-                                    const gfx::Size& full_capture_size,
-                                    const gfx::Rect& crop_region) {
+void RecordingService::RecordRegion(
+    mojo::PendingRemote<mojom::RecordingServiceClient> client,
+    mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+    mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+    const viz::FrameSinkId& frame_sink_id,
+    const gfx::Size& full_capture_size,
+    const gfx::Rect& crop_region) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
 
-  StartNewRecording(VideoCaptureParams::CreateForRegionCapture(
-      frame_sink_id, full_capture_size, crop_region));
+  StartNewRecording(std::move(client), std::move(video_capturer),
+                    std::move(audio_stream_factory),
+                    VideoCaptureParams::CreateForRegionCapture(
+                        frame_sink_id, full_capture_size, crop_region));
 }
 
 void RecordingService::StopRecording() {
@@ -228,6 +235,9 @@ void RecordingService::OnCaptureError(const std::string& message) {
 void RecordingService::OnCaptureMuted(bool is_muted) {}
 
 void RecordingService::StartNewRecording(
+    mojo::PendingRemote<mojom::RecordingServiceClient> client,
+    mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+    mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
     std::unique_ptr<VideoCaptureParams> capture_params) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
 
@@ -236,6 +246,9 @@ void RecordingService::StartNewRecording(
     return;
   }
 
+  client_remote_.reset();
+  client_remote_.Bind(std::move(client));
+
   current_video_capture_params_ = std::move(capture_params);
   const auto capture_size = current_video_capture_params_->GetCaptureSize();
   media::VideoEncoder::Options video_encoder_options;
@@ -255,11 +268,7 @@ void RecordingService::StartNewRecording(
       base::BindOnce(&RecordingService::OnEncodingFailure,
                      base::Unretained(this)));
 
-  ConnectAndStartVideoCapturer();
-
-  mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory;
-  client_remote_->BindAudioStreamFactory(
-      audio_stream_factory.InitWithNewPipeAndPassReceiver());
+  ConnectAndStartVideoCapturer(std::move(video_capturer));
 
   audio_capturer_ = audio::CreateInputDevice(
       std::move(audio_stream_factory),
@@ -283,17 +292,13 @@ void RecordingService::TerminateRecording(bool success) {
                                weak_ptr_factory_.GetWeakPtr(), success));
 }
 
-void RecordingService::ConnectAndStartVideoCapturer() {
+void RecordingService::ConnectAndStartVideoCapturer(
+    mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
-
-  if (!current_video_capture_params_) {
-    // No need to reconnect and resume capturing if there's no onging recording.
-    return;
-  }
+  DCHECK(current_video_capture_params_);
 
   video_capturer_remote_.reset();
-  client_remote_->BindVideoCapturer(
-      video_capturer_remote_.BindNewPipeAndPassReceiver());
+  video_capturer_remote_.Bind(std::move(video_capturer));
   // The GPU process could crash while recording is in progress, and the video
   // capturer will be disconnected. We need to handle this event gracefully.
   video_capturer_remote_.set_disconnect_handler(base::BindOnce(
@@ -305,13 +310,15 @@ void RecordingService::ConnectAndStartVideoCapturer() {
 
 void RecordingService::OnVideoCapturerDisconnected() {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
-  // TODO(afakhry): Do we need an exponential backoff delay here? Should we
-  // really continue capturing here?
-  main_task_runner_->PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&RecordingService::ConnectAndStartVideoCapturer,
-                     weak_ptr_factory_.GetWeakPtr()),
-      kReconnectDelay);
+
+  // On a crash in the GPU, the video capturer gets disconnected, so we can't
+  // communicate with it any longer, but we can still communicate with the audio
+  // capturer. We will stop the recording and flush whatever video chunks we
+  // currently have.
+  did_failure_occur_ = true;
+  audio_capturer_->Stop();
+  audio_capturer_.reset();
+  TerminateRecording(/*success=*/false);
 }
 
 void RecordingService::OnAudioCaptured(

ash/services/recording/recording_service.h

@@ -40,16 +40,26 @@ class RecordingService : public mojom::RecordingService,
   ~RecordingService() override;
 
   // mojom::RecordingService:
-  void SetClient(
-      mojo::PendingRemote<mojom::RecordingServiceClient> client) override;
-  void RecordFullscreen(const viz::FrameSinkId& frame_sink_id,
-                        const gfx::Size& video_size) override;
-  void RecordWindow(const viz::FrameSinkId& frame_sink_id,
-                    const gfx::Size& initial_video_size,
-                    const gfx::Size& max_video_size) override;
-  void RecordRegion(const viz::FrameSinkId& frame_sink_id,
-                    const gfx::Size& full_capture_size,
-                    const gfx::Rect& crop_region) override;
+  void RecordFullscreen(
+      mojo::PendingRemote<mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      const viz::FrameSinkId& frame_sink_id,
+      const gfx::Size& video_size) override;
+  void RecordWindow(
+      mojo::PendingRemote<mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      const viz::FrameSinkId& frame_sink_id,
+      const gfx::Size& initial_video_size,
+      const gfx::Size& max_video_size) override;
+  void RecordRegion(
+      mojo::PendingRemote<mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      const viz::FrameSinkId& frame_sink_id,
+      const gfx::Size& full_capture_size,
+      const gfx::Rect& crop_region) override;
   void StopRecording() override;
 
   // viz::mojom::FrameSinkVideoConsumer:
@@ -72,16 +82,21 @@ class RecordingService : public mojom::RecordingService,
   void OnCaptureMuted(bool is_muted) override;
 
  private:
-  void StartNewRecording(std::unique_ptr<VideoCaptureParams> capture_params);
+  void StartNewRecording(
+      mojo::PendingRemote<mojom::RecordingServiceClient> client,
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer,
+      mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory,
+      std::unique_ptr<VideoCaptureParams> capture_params);
 
   // Called on the main thread on |success| from OnStopped() when all video
   // frames have been sent, or from OnEncodingFailure() with |success| set to
   // false.
   void TerminateRecording(bool success);
 
-  // Creates and binds a video capturer and start capturing video according to
-  // the current |current_video_capture_params_|.
-  void ConnectAndStartVideoCapturer();
+  // Binds the given |video_capturer| to |video_capturer_remote_| and starts
+  // video according to the current |current_video_capture_params_|.
+  void ConnectAndStartVideoCapturer(
+      mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer);
 
   // If the video capturer gets disconnected (e.g. Viz crashes) during an
   // ongoing recording, this attempts to reconnect to a new capturer and resumes

chrome/browser/ui/ash/chrome_capture_mode_delegate.cc

@@ -118,7 +118,7 @@ void ChromeCaptureModeDelegate::OpenFeedbackDialog() {
 }
 
 mojo::Remote<recording::mojom::RecordingService>
-ChromeCaptureModeDelegate::LaunchRecordingService() const {
+ChromeCaptureModeDelegate::LaunchRecordingService() {
   return content::ServiceProcessHost::Launch<
       recording::mojom::RecordingService>(
       content::ServiceProcessHost::Options()

chrome/browser/ui/ash/chrome_capture_mode_delegate.h

@@ -34,7 +34,7 @@ class ChromeCaptureModeDelegate : public ash::CaptureModeDelegate {
   void StopObservingRestrictedContent() override;
   void OpenFeedbackDialog() override;
   mojo::Remote<recording::mojom::RecordingService> LaunchRecordingService()
-      const override;
+      override;
   void BindAudioStreamFactory(
       mojo::PendingReceiver<audio::mojom::StreamFactory> receiver) override;
 };