Crash fix for desktop capture size calculations, and some minor things.

When stress-testing desktop resize (by running a CrOS Ash desktop in a
window), a problem with the capture frame size calculations was
revealed: The max frame size was being updated for each change in
desktop size.  This meant that once width or height were reduced, they
could never be increased again.  When the width or height were reduced
to zero, Chrome would crash.

This change fixes the size calculation (the max size is made constant
from construction), adds a few extra sanity-checks to prevent crashes on
OOM or zero frame sizes, and also adds re-scaling of the rendered mouse
cursor bitmap whenever the desktop size has changed.

BUG=462799
TEST=Resized CrOS Ash-desktop-in-a-window on my local dev workstation to confirm crash is resolved.

Review URL: https://codereview.chromium.org/965123002

Cr-Commit-Position: refs/heads/master@{#318958}

content/browser/media/capture/content_video_capture_device_core.cc

@@ -49,20 +49,7 @@ ThreadSafeCaptureOracle::ThreadSafeCaptureOracle(
       oracle_(base::TimeDelta::FromMicroseconds(
           static_cast<int64>(1000000.0 / params.requested_format.frame_rate +
                              0.5 /* to round to nearest int */))),
-      params_(params),
-      capture_size_updated_(false) {
-  switch (params_.requested_format.pixel_format) {
-    case media::PIXEL_FORMAT_I420:
-      video_frame_format_ = media::VideoFrame::I420;
-      break;
-    case media::PIXEL_FORMAT_TEXTURE:
-      video_frame_format_ = media::VideoFrame::NATIVE_TEXTURE;
-      break;
-    default:
-      LOG(FATAL) << "Unexpected pixel_format "
-                 << params_.requested_format.pixel_format;
-  }
-}
+      params_(params) {}
 
 ThreadSafeCaptureOracle::~ThreadSafeCaptureOracle() {}
 
@@ -80,14 +67,20 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
   if (!client_)
     return false;  // Capture is stopped.
 
+  const media::VideoFrame::Format video_frame_format =
+      params_.requested_format.pixel_format == media::PIXEL_FORMAT_TEXTURE ?
+          media::VideoFrame::NATIVE_TEXTURE : media::VideoFrame::I420;
+
+  if (capture_size_.IsEmpty())
+    capture_size_ = max_frame_size();
+  const gfx::Size visible_size = capture_size_;
   // Always round up the coded size to multiple of 16 pixels.
   // See http://crbug.com/402151.
-  const gfx::Size visible_size = params_.requested_format.frame_size;
   const gfx::Size coded_size((visible_size.width() + 15) & ~15,
                              (visible_size.height() + 15) & ~15);
 
   scoped_refptr<media::VideoCaptureDevice::Client::Buffer> output_buffer =
-      client_->ReserveOutputBuffer(video_frame_format_, coded_size);
+      client_->ReserveOutputBuffer(video_frame_format, coded_size);
   const bool should_capture =
       oracle_.ObserveEventAndDecideCapture(event, damage_rect, event_time);
   const bool content_is_dirty =
@@ -130,9 +123,9 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
                            "trigger", event_name);
   // NATIVE_TEXTURE frames wrap a texture mailbox, which we don't have at the
   // moment.  We do not construct those frames.
-  if (video_frame_format_ != media::VideoFrame::NATIVE_TEXTURE) {
+  if (video_frame_format != media::VideoFrame::NATIVE_TEXTURE) {
     *storage = media::VideoFrame::WrapExternalPackedMemory(
-        video_frame_format_,
+        video_frame_format,
         coded_size,
         gfx::Rect(visible_size),
         visible_size,
@@ -142,6 +135,7 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
         0,
         base::TimeDelta(),
         base::Closure());
+    DCHECK(*storage);
   }
   *callback = base::Bind(&ThreadSafeCaptureOracle::DidCaptureFrame,
                          this,
@@ -153,30 +147,29 @@ bool ThreadSafeCaptureOracle::ObserveEventAndDecideCapture(
 
 gfx::Size ThreadSafeCaptureOracle::GetCaptureSize() const {
   base::AutoLock guard(lock_);
-  return params_.requested_format.frame_size;
+  return capture_size_.IsEmpty() ? max_frame_size() : capture_size_;
 }
 
 void ThreadSafeCaptureOracle::UpdateCaptureSize(const gfx::Size& source_size) {
   base::AutoLock guard(lock_);
 
-  // If this is the first call to UpdateCaptureSize(), or the receiver supports
-  // variable resolution, then determine the capture size by treating the
-  // requested width and height as maxima.
-  if (!capture_size_updated_ || params_.resolution_change_policy ==
+  // Update |capture_size_| based on |source_size| if either: 1) The resolution
+  // change policy specifies fixed frame sizes and |capture_size_| has not yet
+  // been set; or 2) The resolution change policy specifies dynamic frame
+  // sizes.
+  if (capture_size_.IsEmpty() || params_.resolution_change_policy ==
       media::RESOLUTION_POLICY_DYNAMIC_WITHIN_LIMIT) {
-    // The capture resolution should not exceed the source frame size.
-    // In other words it should downscale the image but not upscale it.
-    if (source_size.width() > params_.requested_format.frame_size.width() ||
-        source_size.height() > params_.requested_format.frame_size.height()) {
-      gfx::Rect capture_rect = media::ComputeLetterboxRegion(
-          gfx::Rect(params_.requested_format.frame_size), source_size);
-      params_.requested_format.frame_size.SetSize(
-          MakeEven(capture_rect.width()), MakeEven(capture_rect.height()));
-    } else {
-      params_.requested_format.frame_size.SetSize(
-          MakeEven(source_size.width()), MakeEven(source_size.height()));
+    capture_size_ = source_size;
+    // The capture size should not exceed the maximum frame size.
+    if (capture_size_.width() > max_frame_size().width() ||
+        capture_size_.height() > max_frame_size().height()) {
+      capture_size_ = media::ComputeLetterboxRegion(
+          gfx::Rect(max_frame_size()), capture_size_).size();
     }
-    capture_size_updated_ = true;
+    // The capture size must be even and not less than the minimum frame size.
+    capture_size_ = gfx::Size(
+        std::max(kMinFrameWidth, MakeEven(capture_size_.width())),
+        std::max(kMinFrameHeight, MakeEven(capture_size_.height())));
   }
 }
 

content/browser/media/capture/content_video_capture_device_core.h

@@ -62,6 +62,10 @@ class ThreadSafeCaptureOracle
     return oracle_.min_capture_period();
   }
 
+  gfx::Size max_frame_size() const {
+    return params_.requested_format.frame_size;
+  }
+
   // Returns the current capture resolution.
   gfx::Size GetCaptureSize() const;
 
@@ -98,13 +102,10 @@ class ThreadSafeCaptureOracle
   VideoCaptureOracle oracle_;
 
   // The video capture parameters used to construct the oracle proxy.
-  media::VideoCaptureParams params_;
-
-  // Indicates if capture size has been updated after construction.
-  bool capture_size_updated_;
+  const media::VideoCaptureParams params_;
 
-  // The current capturing format, as a media::VideoFrame::Format.
-  media::VideoFrame::Format video_frame_format_;
+  // The current video capture size.
+  gfx::Size capture_size_;
 };
 
 // Keeps track of the video capture source frames and executes copying on the

content/browser/media/capture/desktop_capture_device_aura.cc

@@ -145,7 +145,8 @@ class DesktopVideoCaptureMachine
       scoped_ptr<cc::CopyOutputResult> result);
 
   // Helper function to update cursor state.
-  // |region_in_frame| defines the desktop bound in the captured frame.
+  // |region_in_frame| defines where the desktop is rendered in the captured
+  // frame.
   // Returns the current cursor position in captured frame.
   gfx::Point UpdateCursorState(const gfx::Rect& region_in_frame);
 
@@ -172,6 +173,7 @@ class DesktopVideoCaptureMachine
 
   // Cursor state.
   ui::Cursor last_cursor_;
+  gfx::Size desktop_size_when_cursor_last_updated_;
   gfx::Point cursor_hot_point_;
   SkBitmap scaled_cursor_bitmap_;
 
@@ -347,6 +349,7 @@ bool DesktopVideoCaptureMachine::ProcessCopyOutputResponse(
     const ThreadSafeCaptureOracle::CaptureFrameCallback& capture_frame_cb,
     scoped_ptr<cc::CopyOutputResult> result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   if (result->IsEmpty() || result->size().IsEmpty() || !desktop_window_)
     return false;
 
@@ -368,6 +371,8 @@ bool DesktopVideoCaptureMachine::ProcessCopyOutputResponse(
         base::TimeDelta(), false);
     capture_frame_cb.Run(video_frame, start_time, true);
     return true;
+  } else {
+    DCHECK(video_frame.get());
   }
 
   // Compute the dest size we want after the letterboxing resize. Make the
@@ -430,19 +435,33 @@ bool DesktopVideoCaptureMachine::ProcessCopyOutputResponse(
 gfx::Point DesktopVideoCaptureMachine::UpdateCursorState(
     const gfx::Rect& region_in_frame) {
   const gfx::Rect desktop_bounds = desktop_window_->layer()->bounds();
+  if (desktop_bounds.IsEmpty()) {
+    // Return early to prevent divide-by-zero in calculations below.
+    ClearCursorState();
+    return gfx::Point();
+  }
+
   gfx::NativeCursor cursor =
       desktop_window_->GetHost()->last_cursor();
-  if (last_cursor_ != cursor) {
+  if (last_cursor_ != cursor ||
+      desktop_size_when_cursor_last_updated_ != desktop_bounds.size()) {
     SkBitmap cursor_bitmap;
     if (ui::GetCursorBitmap(cursor, &cursor_bitmap, &cursor_hot_point_)) {
+      const int scaled_width = cursor_bitmap.width() *
+          region_in_frame.width() / desktop_bounds.width();
+      const int scaled_height = cursor_bitmap.height() *
+          region_in_frame.height() / desktop_bounds.height();
+      if (scaled_width <= 0 || scaled_height <= 0) {
+        ClearCursorState();
+        return gfx::Point();
+      }
       scaled_cursor_bitmap_ = skia::ImageOperations::Resize(
           cursor_bitmap,
           skia::ImageOperations::RESIZE_BEST,
-          cursor_bitmap.width() * region_in_frame.width() /
-              desktop_bounds.width(),
-          cursor_bitmap.height() * region_in_frame.height() /
-              desktop_bounds.height());
+          scaled_width,
+          scaled_height);
       last_cursor_ = cursor;
+      desktop_size_when_cursor_last_updated_ = desktop_bounds.size();
     } else {
       // Clear cursor state if ui::GetCursorBitmap failed so that we do not
       // render cursor on the captured frame.
@@ -466,6 +485,7 @@ gfx::Point DesktopVideoCaptureMachine::UpdateCursorState(
 
 void DesktopVideoCaptureMachine::ClearCursorState() {
   last_cursor_ = ui::Cursor();
+  desktop_size_when_cursor_last_updated_ = gfx::Size();
   cursor_hot_point_ = gfx::Point();
   scaled_cursor_bitmap_.reset();
 }