Revert "capture_mode: Recording service lifetime and APIs"

This reverts commit 38feda4d.

Reason for revert: Observed test failure on Linux ChromiumOS MSAN build.

use-of-uninitialized-value during CaptureModeController::Start()

This CL makes significant changes to CaptureModeController.

See https://ci.chromium.org/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/21350

Original change's description:
> capture_mode: Recording service lifetime and APIs
>
> 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.
>
> Change-Id: I235cb1f9ee81dbbc42e21d0a222d32bcafa7a0fd
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2536030
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#828574}

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

Change-Id: I80c5b69f89fa4eb32d3cbd065729d314d076ae4d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1126586
Bug: 1147989
Bug: 1143411
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2546348
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Reviewed-by: Jiewei Qian  <qjw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828626}

ash/capture_mode/capture_mode_controller.h

@@ -90,6 +90,11 @@ 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;
 
@@ -100,6 +105,16 @@ 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
@@ -114,22 +129,10 @@ 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,32 +26,18 @@ class FakeRecordingService : public recording::mojom::RecordingService {
   }
 
   // mojom::RecordingService:
-  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 {
+  void SetClient(mojo::PendingRemote<recording::mojom::RecordingServiceClient>
+                     client) 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,
+  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 {
-    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& max_window_size) override {}
+  void RecordRegion(const viz::FrameSinkId& frame_sink_id,
                     const gfx::Size& fullscreen_size,
-      const gfx::Rect& corp_region) override {
-    remote_client_.Bind(std::move(client));
-  }
+                    const gfx::Rect& corp_region) override {}
   void StopRecording() override {
     remote_client_->OnRecordingEnded(/*success=*/true);
     remote_client_.FlushForTesting();
@@ -65,7 +51,8 @@ class FakeRecordingService : public recording::mojom::RecordingService {
 // -----------------------------------------------------------------------------
 // TestCaptureModeDelegate:
 
-TestCaptureModeDelegate::TestCaptureModeDelegate() = default;
+TestCaptureModeDelegate::TestCaptureModeDelegate()
+    : fake_service_(std::make_unique<FakeRecordingService>()) {}
 
 TestCaptureModeDelegate::~TestCaptureModeDelegate() = default;
 
@@ -104,8 +91,7 @@ void TestCaptureModeDelegate::StopObservingRestrictedContent() {}
 void TestCaptureModeDelegate::OpenFeedbackDialog() {}
 
 mojo::Remote<recording::mojom::RecordingService>
-TestCaptureModeDelegate::LaunchRecordingService() {
-  fake_service_ = std::make_unique<FakeRecordingService>();
+TestCaptureModeDelegate::LaunchRecordingService() const {
   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()
-      override;
+      const 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() = 0;
+  LaunchRecordingService() const = 0;
 
   // Binds the given audio StreamFactory |receiver| to the audio service.
   virtual void BindAudioStreamFactory(

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

@@ -15,6 +15,17 @@ 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
@@ -35,11 +46,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 {
-  // 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.
+  // Sets the single client of this service.
+  SetClient(pending_remote<RecordingServiceClient> client);
 
   // 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
@@ -47,11 +58,7 @@ 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(
-      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,
+  RecordFullscreen(viz.mojom.FrameSinkId frame_sink_id,
       gfx.mojom.Size video_size);
 
   // Starts a recording of a window which has the given |frame_sink_id|.
@@ -62,11 +69,7 @@ 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(
-      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,
+  RecordWindow(viz.mojom.FrameSinkId frame_sink_id,
       gfx.mojom.Size initial_video_size,
       gfx.mojom.Size max_video_size);
 
@@ -75,11 +78,7 @@ 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(
-      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,
+  RecordRegion(viz.mojom.FrameSinkId frame_sink_id,
       gfx.mojom.Size full_capture_size,
       gfx.mojom.Rect crop_region);
 

ash/services/recording/recording_service.cc

@@ -26,6 +26,11 @@ 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;
@@ -77,49 +82,37 @@ RecordingService::RecordingService(
 
 RecordingService::~RecordingService() = default;
 
-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,
+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) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
 
-  StartNewRecording(std::move(client), std::move(video_capturer),
-                    std::move(audio_stream_factory),
-                    VideoCaptureParams::CreateForFullscreenCapture(
+  StartNewRecording(VideoCaptureParams::CreateForFullscreenCapture(
       frame_sink_id, 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,
+void RecordingService::RecordWindow(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(std::move(client), std::move(video_capturer),
-                    std::move(audio_stream_factory),
-                    VideoCaptureParams::CreateForWindowCapture(
+  StartNewRecording(VideoCaptureParams::CreateForWindowCapture(
       frame_sink_id, initial_video_size, max_video_size));
 }
 
-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,
+void RecordingService::RecordRegion(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(std::move(client), std::move(video_capturer),
-                    std::move(audio_stream_factory),
-                    VideoCaptureParams::CreateForRegionCapture(
+  StartNewRecording(VideoCaptureParams::CreateForRegionCapture(
       frame_sink_id, full_capture_size, crop_region));
 }
 
@@ -235,9 +228,6 @@ 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_);
 
@@ -246,9 +236,6 @@ 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;
@@ -268,7 +255,11 @@ void RecordingService::StartNewRecording(
       base::BindOnce(&RecordingService::OnEncodingFailure,
                      base::Unretained(this)));
 
-  ConnectAndStartVideoCapturer(std::move(video_capturer));
+  ConnectAndStartVideoCapturer();
+
+  mojo::PendingRemote<audio::mojom::StreamFactory> audio_stream_factory;
+  client_remote_->BindAudioStreamFactory(
+      audio_stream_factory.InitWithNewPipeAndPassReceiver());
 
   audio_capturer_ = audio::CreateInputDevice(
       std::move(audio_stream_factory),
@@ -292,13 +283,17 @@ void RecordingService::TerminateRecording(bool success) {
                                weak_ptr_factory_.GetWeakPtr(), success));
 }
 
-void RecordingService::ConnectAndStartVideoCapturer(
-    mojo::PendingRemote<viz::mojom::FrameSinkVideoCapturer> video_capturer) {
+void RecordingService::ConnectAndStartVideoCapturer() {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
-  DCHECK(current_video_capture_params_);
+
+  if (!current_video_capture_params_) {
+    // No need to reconnect and resume capturing if there's no onging recording.
+    return;
+  }
 
   video_capturer_remote_.reset();
-  video_capturer_remote_.Bind(std::move(video_capturer));
+  client_remote_->BindVideoCapturer(
+      video_capturer_remote_.BindNewPipeAndPassReceiver());
   // 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(
@@ -310,15 +305,13 @@ void RecordingService::ConnectAndStartVideoCapturer(
 
 void RecordingService::OnVideoCapturerDisconnected() {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
-
-  // 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);
+  // 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);
 }
 
 void RecordingService::OnAudioCaptured(

ash/services/recording/recording_service.h

@@ -40,24 +40,14 @@ class RecordingService : public mojom::RecordingService,
   ~RecordingService() override;
 
   // mojom::RecordingService:
-  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,
+  void SetClient(
+      mojo::PendingRemote<mojom::RecordingServiceClient> client) override;
+  void RecordFullscreen(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,
+  void RecordWindow(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,
+  void RecordRegion(const viz::FrameSinkId& frame_sink_id,
                     const gfx::Size& full_capture_size,
                     const gfx::Rect& crop_region) override;
   void StopRecording() override;
@@ -82,21 +72,16 @@ class RecordingService : public mojom::RecordingService,
   void OnCaptureMuted(bool is_muted) override;
 
  private:
-  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);
+  void StartNewRecording(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);
 
-  // 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);
+  // Creates and binds a video capturer and start capturing video according to
+  // the current |current_video_capture_params_|.
+  void ConnectAndStartVideoCapturer();
 
   // 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() {
+ChromeCaptureModeDelegate::LaunchRecordingService() const {
   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()
-      override;
+      const override;
   void BindAudioStreamFactory(
       mojo::PendingReceiver<audio::mojom::StreamFactory> receiver) override;
 };