From db73faeca6f1a3a6cb2923afc2e0bc3ea995aee7 Mon Sep 17 00:00:00 2001 From: miu Date: Tue, 3 Mar 2015 15:11:55 -0800 Subject: [PATCH] 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_video_capture_device_core.cc | 63 +++++++++---------- .../content_video_capture_device_core.h | 13 ++-- .../capture/desktop_capture_device_aura.cc | 32 ++++++++-- 3 files changed, 61 insertions(+), 47 deletions(-) diff --git a/content/browser/media/capture/content_video_capture_device_core.cc b/content/browser/media/capture/content_video_capture_device_core.cc index d9956f1fec93d2..c43c623db08a20 100644 --- a/content/browser/media/capture/content_video_capture_device_core.cc +++ b/content/browser/media/capture/content_video_capture_device_core.cc @@ -49,20 +49,7 @@ ThreadSafeCaptureOracle::ThreadSafeCaptureOracle( oracle_(base::TimeDelta::FromMicroseconds( static_cast(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 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()))); } } diff --git a/content/browser/media/capture/content_video_capture_device_core.h b/content/browser/media/capture/content_video_capture_device_core.h index 1543028251f6c0..00ac6249d53d2e 100644 --- a/content/browser/media/capture/content_video_capture_device_core.h +++ b/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 diff --git a/content/browser/media/capture/desktop_capture_device_aura.cc b/content/browser/media/capture/desktop_capture_device_aura.cc index 37979613dddf05..de1a1093b38f82 100644 --- a/content/browser/media/capture/desktop_capture_device_aura.cc +++ b/content/browser/media/capture/desktop_capture_device_aura.cc @@ -145,7 +145,8 @@ class DesktopVideoCaptureMachine scoped_ptr 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 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(); }