capture_mode: Handle moving a recorded window to a different display

While a window is being recorded, the user may drag it to
a different display.
  - This event must be propagated to the recording service, so that
    the video capturer can change its target frame sink to that of the
    root window of the display the window was moved to.
The new display may have different bounds than that of the old display.
  - The recording service must also handle this case by reconfiguring
    the VPX video encoder as it will be getting video frames that match
    the new display size.

This CL also fixes a bug where the recorded window shows up as squished
in the output video. This was because we used to initialize the video
encoder with the current window's bounds rather than the root's bounds.

Demo is on the first bug.

BUG=1165702, 1165708
TEST=Manually, added new tests in ash_unittests and browser_tests.

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

ash/capture_mode/capture_mode_controller.cc

@@ -491,6 +491,26 @@ void CaptureModeController::StartVideoRecordingImmediatelyForTesting() {
   OnVideoRecordCountDownFinished();
 }
 
+void CaptureModeController::OnRecordedWindowChangingRoot(
+    aura::Window* window,
+    aura::Window* new_root) {
+  DCHECK(is_recording_in_progress_);
+  DCHECK(video_recording_watcher_);
+  DCHECK_EQ(window, video_recording_watcher_->window_being_recorded());
+  DCHECK(recording_service_remote_);
+  DCHECK(new_root);
+
+  // When a window being recorded changes displays either due to a display
+  // getting disconnected, or moved by the user, the stop-recording button
+  // should follow that window to that display.
+  capture_mode_util::SetStopRecordingButtonVisibility(window->GetRootWindow(),
+                                                      false);
+  capture_mode_util::SetStopRecordingButtonVisibility(new_root, true);
+
+  recording_service_remote_->OnRecordedWindowChangingRoot(
+      new_root->GetFrameSinkId(), new_root->GetBoundsInRootWindow().size());
+}
+
 bool CaptureModeController::ShouldBlockRecordingForContentProtection(
     aura::Window* window) const {
   if (window->IsRootWindow()) {

ash/capture_mode/capture_mode_controller.h

@@ -122,8 +122,19 @@ class ASH_EXPORT CaptureModeController
   // video recording right away for testing purposes.
   void StartVideoRecordingImmediatelyForTesting();
 
+  CaptureModeDelegate* delegate_for_testing() const { return delegate_.get(); }
+
  private:
   friend class CaptureModeTestApi;
+  friend class VideoRecordingWatcher;
+
+  // Called by |video_recording_watcher_| to inform us that the |window| being
+  // recorded (i.e. |is_recording_in_progress_| is true) is about to move to a
+  // |new_root|. This is needed so we can inform the recording service of this
+  // change so that it can switch its capture target to the new root's frame
+  // sink.
+  void OnRecordedWindowChangingRoot(aura::Window* window,
+                                    aura::Window* new_root);
 
   // Returns true if screen recording needs to be blocked due to protected
   // content. |window| is the window being recorded or desired to be recorded.

ash/capture_mode/capture_mode_unittests.cc

@@ -16,6 +16,7 @@
 #include "ash/capture_mode/capture_mode_types.h"
 #include "ash/capture_mode/capture_mode_util.h"
 #include "ash/capture_mode/stop_recording_button_tray.h"
+#include "ash/capture_mode/test_capture_mode_delegate.h"
 #include "ash/display/cursor_window_controller.h"
 #include "ash/display/output_protection_delegate.h"
 #include "ash/display/screen_orientation_controller_test_api.h"
@@ -1498,6 +1499,39 @@ TEST_F(CaptureModeTest, WindowRecordingCaptureId) {
   EXPECT_FALSE(window->subtree_capture_id().is_valid());
 }
 
+TEST_F(CaptureModeTest, MultiDisplayWindowRecording) {
+  UpdateDisplay("400x400,401+0-800x800");
+  auto roots = Shell::GetAllRootWindows();
+  ASSERT_EQ(2u, roots.size());
+
+  auto window = CreateTestWindow(gfx::Rect(200, 200));
+  StartCaptureSession(CaptureModeSource::kWindow, CaptureModeType::kVideo);
+
+  auto* event_generator = GetEventGenerator();
+  event_generator->MoveMouseToCenterOf(window.get());
+  auto* controller = CaptureModeController::Get();
+  controller->StartVideoRecordingImmediatelyForTesting();
+  EXPECT_TRUE(controller->is_recording_in_progress());
+
+  // The capturer should capture from the frame sink of the first display, and
+  // the video size should match the size of the current root window.
+  CaptureModeTestApi test_api;
+  test_api.FlushRecordingServiceForTesting();
+  auto* test_delegate =
+      static_cast<TestCaptureModeDelegate*>(controller->delegate_for_testing());
+  EXPECT_EQ(roots[0]->GetFrameSinkId(), test_delegate->GetCurrentFrameSinkId());
+  EXPECT_EQ(roots[0]->bounds().size(), test_delegate->GetCurrentVideoSize());
+
+  // Moving a window to a different display should be propagated to the service,
+  // with the new root's frame sink ID, and the new root's size.
+  window_util::MoveWindowToDisplay(window.get(),
+                                   roots[1]->GetHost()->GetDisplayId());
+  test_api.FlushRecordingServiceForTesting();
+  ASSERT_EQ(window->GetRootWindow(), roots[1]);
+  EXPECT_EQ(roots[1]->GetFrameSinkId(), test_delegate->GetCurrentFrameSinkId());
+  EXPECT_EQ(roots[1]->bounds().size(), test_delegate->GetCurrentVideoSize());
+}
+
 // Tests the behavior of screen recording with the presence of HDCP secure
 // content on the screen in all capture mode sources (fullscreen, region, and
 // window) depending on the test param.

ash/capture_mode/test_capture_mode_delegate.cc

@@ -4,8 +4,8 @@
 
 #include "ash/capture_mode/test_capture_mode_delegate.h"
 
+#include "ash/capture_mode/capture_mode_types.h"
 #include "ash/services/recording/public/mojom/recording_service.mojom.h"
-#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/threading/thread_restrictions.h"
 
@@ -17,11 +17,15 @@ namespace ash {
 class FakeRecordingService : public recording::mojom::RecordingService {
  public:
   FakeRecordingService() : receiver_(this) {}
-
   FakeRecordingService(const FakeRecordingService&) = delete;
   FakeRecordingService& operator=(const FakeRecordingService&) = delete;
   ~FakeRecordingService() override = default;
 
+  const viz::FrameSinkId& current_frame_sink_id() const {
+    return current_frame_sink_id_;
+  }
+  const gfx::Size& video_size() const { return video_size_; }
+
   void Bind(
       mojo::PendingReceiver<recording::mojom::RecordingService> receiver) {
     receiver_.Bind(std::move(receiver));
@@ -35,6 +39,9 @@ class FakeRecordingService : public recording::mojom::RecordingService {
       const viz::FrameSinkId& frame_sink_id,
       const gfx::Size& fullscreen_size) override {
     remote_client_.Bind(std::move(client));
+    current_frame_sink_id_ = frame_sink_id;
+    current_capture_source_ = CaptureModeSource::kFullscreen;
+    video_size_ = fullscreen_size;
   }
   void RecordWindow(
       mojo::PendingRemote<recording::mojom::RecordingServiceClient> client,
@@ -45,6 +52,9 @@ class FakeRecordingService : public recording::mojom::RecordingService {
       const gfx::Size& initial_window_size,
       const gfx::Size& max_window_size) override {
     remote_client_.Bind(std::move(client));
+    current_frame_sink_id_ = frame_sink_id;
+    current_capture_source_ = CaptureModeSource::kWindow;
+    video_size_ = max_window_size;
   }
   void RecordRegion(
       mojo::PendingRemote<recording::mojom::RecordingServiceClient> client,
@@ -52,17 +62,30 @@ class FakeRecordingService : public recording::mojom::RecordingService {
       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 {
+      const gfx::Rect& crop_region) override {
     remote_client_.Bind(std::move(client));
+    current_frame_sink_id_ = frame_sink_id;
+    current_capture_source_ = CaptureModeSource::kRegion;
+    video_size_ = crop_region.size();
   }
   void StopRecording() override {
     remote_client_->OnRecordingEnded(/*success=*/true);
     remote_client_.FlushForTesting();
   }
+  void OnRecordedWindowChangingRoot(
+      const viz::FrameSinkId& new_frame_sink_id,
+      const gfx::Size& new_max_video_size) override {
+    DCHECK_EQ(current_capture_source_, CaptureModeSource::kWindow);
+    current_frame_sink_id_ = new_frame_sink_id;
+    video_size_ = new_max_video_size;
+  }
 
  private:
   mojo::Receiver<recording::mojom::RecordingService> receiver_;
   mojo::Remote<recording::mojom::RecordingServiceClient> remote_client_;
+  viz::FrameSinkId current_frame_sink_id_;
+  CaptureModeSource current_capture_source_ = CaptureModeSource::kFullscreen;
+  gfx::Size video_size_;
 };
 
 // -----------------------------------------------------------------------------
@@ -77,6 +100,15 @@ TestCaptureModeDelegate::TestCaptureModeDelegate() {
 
 TestCaptureModeDelegate::~TestCaptureModeDelegate() = default;
 
+viz::FrameSinkId TestCaptureModeDelegate::GetCurrentFrameSinkId() const {
+  return fake_service_ ? fake_service_->current_frame_sink_id()
+                       : viz::FrameSinkId();
+}
+
+gfx::Size TestCaptureModeDelegate::GetCurrentVideoSize() const {
+  return fake_service_ ? fake_service_->video_size() : gfx::Size();
+}
+
 base::FilePath TestCaptureModeDelegate::GetActiveUserDownloadsDir() const {
   return fake_downloads_dir_;
 }

ash/capture_mode/test_capture_mode_delegate.h

@@ -10,6 +10,8 @@
 #include "ash/public/cpp/capture_mode_delegate.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
+#include "components/viz/common/surfaces/frame_sink_id.h"
+#include "ui/gfx/geometry/size.h"
 
 namespace ash {
 
@@ -22,6 +24,12 @@ class TestCaptureModeDelegate : public CaptureModeDelegate {
   TestCaptureModeDelegate& operator=(const TestCaptureModeDelegate&) = delete;
   ~TestCaptureModeDelegate() override;
 
+  // Gets the current frame sink id being captured by the fake service.
+  viz::FrameSinkId GetCurrentFrameSinkId() const;
+
+  // Gets the current video size being captured by the fake service.
+  gfx::Size GetCurrentVideoSize() const;
+
   // CaptureModeDelegate:
   base::FilePath GetActiveUserDownloadsDir() const override;
   void ShowScreenCaptureItemInFolder(const base::FilePath& file_path) override;

ash/capture_mode/video_recording_watcher.cc

@@ -6,7 +6,6 @@
 
 #include "ash/capture_mode/capture_mode_controller.h"
 #include "ash/capture_mode/capture_mode_metrics.h"
-#include "ash/capture_mode/capture_mode_util.h"
 #include "base/check.h"
 #include "base/check_op.h"
 #include "base/notreached.h"
@@ -62,12 +61,7 @@ void VideoRecordingWatcher::OnWindowRemovingFromRootWindow(
   if (!new_root)
     return;
 
-  // When a window being recorded changes displays either due to a display
-  // getting disconnected, or moved by the user, the stop-recording button
-  // should follow that window to that display.
-  capture_mode_util::SetStopRecordingButtonVisibility(window->GetRootWindow(),
-                                                      false);
-  capture_mode_util::SetStopRecordingButtonVisibility(new_root, true);
+  controller_->OnRecordedWindowChangingRoot(window_being_recorded_, new_root);
 }
 
 }  // namespace ash

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

@@ -96,4 +96,18 @@ interface RecordingService {
   // Stops an on-going video recording. The remaining frames will continue being
   // provided to the client until OnRecordingEnded() is called.
   StopRecording();
+
+  // Called by the client to let the service know that a window being recorded
+  // is about to move to a different display (i.e a different root window),
+  // giving it the |new_frame_sink_id| of that new root window. It also provides
+  // the |new_max_video_size| in DIPs (since the new display may have different
+  // bounds).
+  // Note that changing the window's root doesn't affect the window's
+  // SubtreeCaptureId, and therefore it doesn't need to be reprovided.
+  // This can *only* be called when the service is already recording a window
+  // (i.e. RecordWindow() has already been called to start the recording of a
+  // window).
+  OnRecordedWindowChangingRoot(
+      viz.mojom.FrameSinkId new_frame_sink_id,
+      gfx.mojom.Size new_max_video_size);
 };

ash/services/recording/recording_encoder_muxer.cc

@@ -45,6 +45,35 @@ base::SequenceBound<RecordingEncoderMuxer> RecordingEncoderMuxer::Create(
       std::move(on_failure_callback));
 }
 
+void RecordingEncoderMuxer::InitializeVideoEncoder(
+    const media::VideoEncoder::Options& video_encoder_options) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Note: The VpxVideoEncoder supports changing the encoding options
+  // dynamically, but it won't work for all frame size changes and may cause
+  // encoding failures. Therefore, it's better to recreate and reinitialize a
+  // new encoder. See media::VpxVideoEncoder::ChangeOptions() for more details.
+
+  if (video_encoder_ && is_video_encoder_initialized_) {
+    auto* encoder_ptr = video_encoder_.get();
+    encoder_ptr->Flush(base::BindOnce(
+        // Holds on to the old encoder until it flushes its buffers, then
+        // destroys it.
+        [](std::unique_ptr<media::VpxVideoEncoder> old_encoder,
+           media::Status status) {},
+        std::move(video_encoder_)));
+  }
+
+  is_video_encoder_initialized_ = false;
+  video_encoder_ = std::make_unique<media::VpxVideoEncoder>();
+  video_encoder_->Initialize(
+      media::VP8PROFILE_ANY, video_encoder_options,
+      base::BindRepeating(&RecordingEncoderMuxer::OnVideoEncoderOutput,
+                          weak_ptr_factory_.GetWeakPtr()),
+      base::BindOnce(&RecordingEncoderMuxer::OnVideoEncoderInitialized,
+                     weak_ptr_factory_.GetWeakPtr(), video_encoder_.get()));
+}
+
 void RecordingEncoderMuxer::EncodeVideo(
     scoped_refptr<media::VideoFrame> frame) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -85,9 +114,9 @@ void RecordingEncoderMuxer::FlushAndFinalize(base::OnceClosure on_done) {
   // asynchronously.
   if (audio_encoder_)
     audio_encoder_->Flush();
-  video_encoder_.Flush(
+  video_encoder_->Flush(
       base::BindOnce(&RecordingEncoderMuxer::OnVideoEncoderFlushed,
-                     base::Unretained(this), std::move(on_done)));
+                     weak_ptr_factory_.GetWeakPtr(), std::move(on_done)));
 }
 
 RecordingEncoderMuxer::RecordingEncoderMuxer(
@@ -95,40 +124,42 @@ RecordingEncoderMuxer::RecordingEncoderMuxer(
     const media::AudioParameters* audio_input_params,
     media::WebmMuxer::WriteDataCB muxer_output_callback,
     FailureCallback on_failure_callback)
-    : audio_encoder_(
-          !audio_input_params
-              ? nullptr
-              : std::make_unique<media::AudioOpusEncoder>(
-                    *audio_input_params,
-                    base::BindRepeating(&RecordingEncoderMuxer::OnAudioEncoded,
-                                        base::Unretained(this)),
-                    base::BindRepeating(&RecordingEncoderMuxer::OnEncoderStatus,
-                                        base::Unretained(this),
-                                        /*for_video=*/false),
-                    // 0 means the encoder picks bitrate automatically.
-                    /*bits_per_second=*/0)),
-      webm_muxer_(media::kCodecOpus,
+    : webm_muxer_(media::kCodecOpus,
                   /*has_video_=*/true,
                   /*has_audio_=*/!!audio_input_params,
                   muxer_output_callback),
       on_failure_callback_(std::move(on_failure_callback)) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  video_encoder_.Initialize(
-      media::VP8PROFILE_ANY, video_encoder_options,
-      base::BindRepeating(&RecordingEncoderMuxer::OnVideoEncoderOutput,
-                          base::Unretained(this)),
-      base::BindOnce(&RecordingEncoderMuxer::OnVideoEncoderInitialized,
-                     base::Unretained(this)));
+  if (audio_input_params) {
+    audio_encoder_ = std::make_unique<media::AudioOpusEncoder>(
+        *audio_input_params,
+        base::BindRepeating(&RecordingEncoderMuxer::OnAudioEncoded,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindRepeating(&RecordingEncoderMuxer::OnEncoderStatus,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            /*for_video=*/false),
+        // 0 means the encoder picks bitrate automatically.
+        /*bits_per_second=*/0);
+  }
+
+  InitializeVideoEncoder(video_encoder_options);
 }
 
 RecordingEncoderMuxer::~RecordingEncoderMuxer() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
-void RecordingEncoderMuxer::OnVideoEncoderInitialized(media::Status status) {
+void RecordingEncoderMuxer::OnVideoEncoderInitialized(
+    media::VpxVideoEncoder* encoder,
+    media::Status status) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
+  // Ignore initialization of encoders that were removed as part of
+  // reinitialization.
+  if (video_encoder_.get() != encoder)
+    return;
+
   if (!status.is_ok()) {
     LOG(ERROR) << "Could not initialize the video encoder: "
                << status.message();
@@ -152,10 +183,10 @@ void RecordingEncoderMuxer::EncodeVideoImpl(
     return;
 
   video_visible_rect_sizes_.push(frame->visible_rect().size());
-  video_encoder_.Encode(
+  video_encoder_->Encode(
       frame, /*key_frame=*/false,
       base::BindOnce(&RecordingEncoderMuxer::OnEncoderStatus,
-                     base::Unretained(this), /*for_video=*/true));
+                     weak_ptr_factory_.GetWeakPtr(), /*for_video=*/true));
 }
 
 void RecordingEncoderMuxer::OnVideoEncoderOutput(

ash/services/recording/recording_encoder_muxer.h

@@ -11,6 +11,7 @@
 #include "base/containers/circular_deque.h"
 #include "base/containers/queue.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/thread_annotations.h"
@@ -78,6 +79,13 @@ class RecordingEncoderMuxer {
     return !on_failure_callback_;
   }
 
+  // Creates and initializes the video encoder if none exists, or recreates and
+  // reinitializes it otherwise. This is useful when the video frame dimensions
+  // may need to change dynamically (such as when a recorded window gets moved
+  // to a display with different bounds).
+  void InitializeVideoEncoder(
+      const media::VideoEncoder::Options& video_encoder_options);
+
   // Encodes and muxes the given video |frame|.
   void EncodeVideo(scoped_refptr<media::VideoFrame> frame);
 
@@ -104,10 +112,11 @@ class RecordingEncoderMuxer {
       FailureCallback on_failure_callback);
   ~RecordingEncoderMuxer();
 
-  // Called when the video encoder is initialized to provide the |status| of the
-  // initialization. If initialization failed, |on_failure_callback_| will
+  // Called when the video |encoder| is initialized to provide the |status| of
+  // the initialization. If initialization failed, |on_failure_callback_| will
   // be triggered.
-  void OnVideoEncoderInitialized(media::Status status);
+  void OnVideoEncoderInitialized(media::VpxVideoEncoder* encoder,
+                                 media::Status status);
 
   // Performs the actual encoding of the given video |frame|. It should never be
   // called before the video encoder is initialized. Video frames received
@@ -138,7 +147,8 @@ class RecordingEncoderMuxer {
   // the value of |for_video|.
   void NotifyFailure(FailureType type, bool for_video);
 
-  media::VpxVideoEncoder video_encoder_ GUARDED_BY_CONTEXT(sequence_checker_);
+  std::unique_ptr<media::VpxVideoEncoder> video_encoder_
+      GUARDED_BY_CONTEXT(sequence_checker_);
 
   std::unique_ptr<media::AudioOpusEncoder> audio_encoder_
       GUARDED_BY_CONTEXT(sequence_checker_);
@@ -173,6 +183,9 @@ class RecordingEncoderMuxer {
       false;
 
   SEQUENCE_CHECKER(sequence_checker_);
+
+  base::WeakPtrFactory<RecordingEncoderMuxer> weak_ptr_factory_
+      GUARDED_BY_CONTEXT(sequence_checker_){this};
 };
 
 }  // namespace recording

ash/services/recording/recording_service.cc

@@ -50,6 +50,20 @@ uint64_t CalculateVpxEncoderBitrate(const gfx::Size& capture_size) {
                                         kBitsPerSecondPerSquarePixel));
 }
 
+// Given the desired |capture_size|, it creates and returns the options needed
+// to configure the video encoder.
+media::VideoEncoder::Options CreateVideoEncoderOptions(
+    const gfx::Size& capture_size) {
+  media::VideoEncoder::Options video_encoder_options;
+  video_encoder_options.bitrate = CalculateVpxEncoderBitrate(capture_size);
+  video_encoder_options.framerate = kMaxFrameRate;
+  video_encoder_options.frame_size = capture_size;
+  // This value, expressed as a number of frames, forces the encoder to code
+  // a keyframe if one has not been coded in the last keyframe_interval frames.
+  video_encoder_options.keyframe_interval = 100;
+  return video_encoder_options;
+}
+
 media::AudioParameters GetAudioParameters() {
   static_assert(kAudioSampleRate % 100 == 0,
                 "Audio sample rate is not divisible by 100");
@@ -101,8 +115,6 @@ void RecordingService::RecordWindow(
     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(
@@ -133,6 +145,27 @@ void RecordingService::StopRecording() {
   audio_capturer_.reset();
 }
 
+void RecordingService::OnRecordedWindowChangingRoot(
+    const viz::FrameSinkId& new_frame_sink_id,
+    const gfx::Size& new_max_video_size) {
+  DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
+
+  if (!current_video_capture_params_) {
+    // A recording might terminate before we signal the client with an
+    // |OnRecordingEnded()| call.
+    return;
+  }
+
+  // If there's a change in the new root's size, we must reconfigure the video
+  // encoder so that output video has the correct dimensions.
+  if (current_video_capture_params_->OnRecordedWindowChangingRoot(
+          video_capturer_remote_, new_frame_sink_id, new_max_video_size)) {
+    encoder_muxer_.AsyncCall(&RecordingEncoderMuxer::InitializeVideoEncoder)
+        .WithArgs(CreateVideoEncoderOptions(
+            current_video_capture_params_->GetCaptureSize()));
+  }
+}
+
 void RecordingService::OnFrameCaptured(
     base::ReadOnlySharedMemoryRegion data,
     media::mojom::VideoFrameInfoPtr info,
@@ -253,19 +286,12 @@ void RecordingService::StartNewRecording(
   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;
-  video_encoder_options.bitrate = CalculateVpxEncoderBitrate(capture_size);
-  video_encoder_options.framerate = kMaxFrameRate;
-  video_encoder_options.frame_size = capture_size;
-  // This value, expressed as a number of frames, forces the encoder to code
-  // a keyframe if one has not been coded in the last keyframe_interval frames.
-  video_encoder_options.keyframe_interval = 100;
-
   const bool should_record_audio = audio_stream_factory.is_valid();
 
   encoder_muxer_ = RecordingEncoderMuxer::Create(
-      encoding_task_runner_, video_encoder_options,
+      encoding_task_runner_,
+      CreateVideoEncoderOptions(
+          current_video_capture_params_->GetCaptureSize()),
       should_record_audio ? &audio_parameters_ : nullptr,
       base::BindRepeating(&RecordingService::OnMuxerWrite,
                           base::Unretained(this)),
@@ -365,14 +391,12 @@ void RecordingService::OnRecordingFailure() {
 
 void RecordingService::OnEncoderMuxerFlushed(bool success) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(encoding_sequence_checker_);
-  DCHECK(encoder_muxer_);
 
   // If flushing the encoders and muxers resulted in some chunks being cached
   // here, we flush them to the client now.
   if (number_of_buffered_chunks_)
     FlushBufferedChunks();
 
-  encoder_muxer_.Reset();
   main_task_runner_->PostNonNestableTask(
       FROM_HERE, base::BindOnce(&RecordingService::SignalRecordingEndedToClient,
                                 base::Unretained(this), success));
@@ -386,7 +410,9 @@ void RecordingService::SignalMuxerOutputToClient(std::string muxer_output) {
 
 void RecordingService::SignalRecordingEndedToClient(bool success) {
   DCHECK_CALLED_ON_VALID_THREAD(main_thread_checker_);
+  DCHECK(encoder_muxer_);
 
+  encoder_muxer_.Reset();
   client_remote_->OnRecordingEnded(success);
 }
 

ash/services/recording/recording_service.h

@@ -64,6 +64,9 @@ class RecordingService : public mojom::RecordingService,
       const gfx::Size& full_capture_size,
       const gfx::Rect& crop_region) override;
   void StopRecording() override;
+  void OnRecordedWindowChangingRoot(
+      const viz::FrameSinkId& new_frame_sink_id,
+      const gfx::Size& new_max_video_size) override;
 
   // viz::mojom::FrameSinkVideoConsumer:
   void OnFrameCaptured(
@@ -190,9 +193,11 @@ class RecordingService : public mojom::RecordingService,
   scoped_refptr<media::AudioCapturerSource> audio_capturer_
       GUARDED_BY_CONTEXT(main_thread_checker_);
 
-  // Performs all encoding and muxing operations on the |encoding_task_runner_|,
-  // and it is bound to the sequence of that task runner.
-  base::SequenceBound<RecordingEncoderMuxer> encoder_muxer_;
+  // Performs all encoding and muxing operations asynchronously on the
+  // |encoding_task_runner_|. However, the |encoder_muxer_| object itself is
+  // constructed, used, and destroyed on the main thread sequence.
+  base::SequenceBound<RecordingEncoderMuxer> encoder_muxer_
+      GUARDED_BY_CONTEXT(main_thread_checker_);
 
   // To avoid doing a ton of IPC calls to the client for each muxed chunk
   // received from |encoder_muxer_| in OnMuxerWrite(), we buffer those chunks

ash/services/recording/video_capture_params.cc

@@ -70,11 +70,43 @@ class WindowCaptureParams : public VideoCaptureParams {
     capturer->SetAutoThrottlingEnabled(true);
   }
 
-  gfx::Size GetCaptureSize() const override { return initial_video_size_; }
+  gfx::Size GetCaptureSize() const override {
+    // For now, the capturer sends us video frames whose sizes are equal to the
+    // size of the root on which the window resides. Therefore,
+    // |max_video_size_| should be used to initialize the video encoder.
+    // Otherwise, the pixels of the output video will be squished. With this
+    // approach, it's possible to resize the window within those bounds without
+    // having to change the size of the output video. However, this may not be
+    // a desired way.
+    // TODO(https://crbug.com/1165708): Investigate how to fix this in the
+    // capturer for M-89 or M-90.
+    return max_video_size_;
+  }
+
+  bool OnRecordedWindowChangingRoot(
+      mojo::Remote<viz::mojom::FrameSinkVideoCapturer>& capturer,
+      viz::FrameSinkId new_frame_sink_id,
+      const gfx::Size& new_max_video_size) override {
+    DCHECK(new_frame_sink_id.is_valid());
+
+    // The video encoder deals with video frames. Changing the frame sink ID
+    // doesn't affect the encoder. What affects it is a change in the video
+    // frames size.
+    const bool should_reconfigure_video_encoder =
+        max_video_size_ != new_max_video_size;
+
+    max_video_size_ = new_max_video_size;
+    frame_sink_id_ = new_frame_sink_id;
+    capturer->SetResolutionConstraints(initial_video_size_, max_video_size_,
+                                       /*use_fixed_aspect_ratio=*/false);
+    capturer->ChangeTarget(frame_sink_id_, subtree_capture_id_);
+
+    return should_reconfigure_video_encoder;
+  }
 
  private:
   const gfx::Size initial_video_size_;
-  const gfx::Size max_video_size_;
+  gfx::Size max_video_size_;
 };
 
 // -----------------------------------------------------------------------------
@@ -161,6 +193,14 @@ gfx::Rect VideoCaptureParams::GetVideoFrameVisibleRect(
   return original_frame_visible_rect;
 }
 
+bool VideoCaptureParams::OnRecordedWindowChangingRoot(
+    mojo::Remote<viz::mojom::FrameSinkVideoCapturer>& capturer,
+    viz::FrameSinkId new_frame_sink_id,
+    const gfx::Size& new_max_video_size) {
+  CHECK(false) << "This can only be called when recording a video";
+  return false;
+}
+
 VideoCaptureParams::VideoCaptureParams(viz::FrameSinkId frame_sink_id,
                                        viz::SubtreeCaptureId subtree_capture_id)
     : frame_sink_id_(frame_sink_id), subtree_capture_id_(subtree_capture_id) {

ash/services/recording/video_capture_params.h

@@ -78,11 +78,24 @@ class VideoCaptureParams {
   // Returns the size in DIPs with which the audio encoder will be initialized.
   virtual gfx::Size GetCaptureSize() const = 0;
 
+  // Called when a window, being recorded by the given |capturer|, is moved to
+  // a different display whose root window has the given |new_frame_sink_id|,
+  // and |new_max_video_size| which matches the new display's size.
+  // The default implementation is to *crash* the service, as this is only valid
+  // when recording a window.
+  // Returns true if the video encoder needs to be reconfigured, which happens
+  // when the bounds of the new display is different than that of the old
+  // display. Returns false otherwise.
+  virtual bool OnRecordedWindowChangingRoot(
+      mojo::Remote<viz::mojom::FrameSinkVideoCapturer>& capturer,
+      viz::FrameSinkId new_frame_sink_id,
+      const gfx::Size& new_max_video_size) WARN_UNUSED_RESULT;
+
  protected:
   explicit VideoCaptureParams(viz::FrameSinkId frame_sink_id,
                               viz::SubtreeCaptureId subtree_capture_id);
 
-  const viz::FrameSinkId frame_sink_id_;
+  viz::FrameSinkId frame_sink_id_;
   const viz::SubtreeCaptureId subtree_capture_id_;
 };
 

chrome/browser/ui/ash/OWNERS

@@ -7,6 +7,8 @@ per-file chrome_keyboard_ui*=yhanada@chromium.org
 per-file *login*=file://ash/login/OWNERS
 per-file keyboard_*=yhanada@chromium.org
 per-file *wallpaper*=file://ash/wallpaper/OWNERS
+per-file *capture_mode*=file://ash/capture_mode/OWNERS
+per-file recording_service_browsertest.cc=file://ash/capture_mode/OWNERS
 
 # Must be updated when a new OS settings section is added.
 per-file chrome_new_window_client*=file://chrome/browser/resources/settings/chromeos/OWNERS

chrome/browser/ui/ash/recording_service_browsertest.cc

@@ -28,6 +28,7 @@
 #include "media/base/mock_media_log.h"
 #include "media/formats/webm/webm_stream_parser.h"
 #include "ui/aura/window.h"
+#include "ui/display/screen.h"
 #include "ui/display/test/display_manager_test_api.h"
 #include "ui/events/test/event_generator.h"
 #include "ui/gfx/geometry/point.h"
@@ -190,6 +191,57 @@ IN_PROC_BROWSER_TEST_F(RecordingServiceBrowserTest, RecordWindow) {
   FinishVideoRecordingTest(&test_api);
 }
 
+IN_PROC_BROWSER_TEST_F(RecordingServiceBrowserTest, RecordWindowMultiDisplay) {
+  display::test::DisplayManagerTestApi(ash::ShellTestApi().display_manager())
+      .UpdateDisplay("300x200,301+0-400x400");
+
+  ash::CaptureModeTestApi capture_mode_test_api;
+  capture_mode_test_api.StartForWindow(/*for_video=*/true);
+  auto* generator = GetEventGenerator();
+  // Move the mouse cursor above the browser window to select it for window
+  // capture (make sure it doesn't hover over the capture bar).
+  generator->MoveMouseTo(GetBrowserWindow()->GetBoundsInScreen().top_center());
+  capture_mode_test_api.PerformCapture();
+  capture_mode_test_api.FlushRecordingServiceForTesting();
+
+  // Moves the browser window to the display at the given |screen_point|.
+  auto move_browser_to_display_at_point = [&](const gfx::Point& screen_point) {
+    auto* screen = display::Screen::GetScreen();
+    aura::Window* new_root =
+        screen->GetWindowAtScreenPoint(screen_point)->GetRootWindow();
+    auto* browser_window = GetBrowserWindow();
+    EXPECT_NE(new_root, browser_window->GetRootWindow());
+    auto* target_container =
+        new_root->GetChildById(browser_window->parent()->id());
+    DCHECK(target_container);
+    target_container->AddChild(browser_window);
+    EXPECT_EQ(new_root, browser_window->GetRootWindow());
+  };
+
+  // Record for a little bit, then move the window to the secondary display.
+  WaitForMilliseconds(600);
+  move_browser_to_display_at_point(gfx::Point(320, 50));
+  capture_mode_test_api.FlushRecordingServiceForTesting();
+
+  // Record for a little bit, then resize the browser window.
+  WaitForMilliseconds(600);
+  GetBrowserWindow()->SetBounds(gfx::Rect(310, 10, 300, 300));
+  capture_mode_test_api.FlushRecordingServiceForTesting();
+
+  // Record for a little bit, then move the browser window back to the smaller
+  // display.
+  WaitForMilliseconds(600);
+  move_browser_to_display_at_point(gfx::Point(0, 0));
+  capture_mode_test_api.FlushRecordingServiceForTesting();
+
+  // Record for a little bit, then end recording. The output video file should
+  // still be valid.
+  WaitForMilliseconds(600);
+  capture_mode_test_api.StopVideoRecording();
+  const base::FilePath video_path = WaitForVideoFileToBeSaved();
+  VerifyVideoFileAndDelete(video_path);
+}
+
 IN_PROC_BROWSER_TEST_F(RecordingServiceBrowserTest, RecordRegion) {
   ash::CaptureModeTestApi test_api;
   test_api.StartForRegion(/*for_video=*/true);