Commit 5913da3a authored by Xiaohui Chen's avatar Xiaohui Chen Committed by Commit Bot

[assistant] Fix DCHECK error on getting screenshot

gfx::Image object is not threadsafe, we process the image async to
avoid blocking UI thread but causes DCHECK error.  Now we explicity
transfer ownership of the object down to the callback.

BUG=None
TEST=running with DCHECK on and no error triggered after change.

Change-Id: I1b3f678ddd9a411c59328e67de44b4e5db7ae865
Reviewed-on: https://chromium-review.googlesource.com/764373
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Reviewed-by: default avatarXiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517128}
parent 1d5b995e
...@@ -131,7 +131,7 @@ class AccessibilityHighlightManagerTest : public InProcessBrowserTest { ...@@ -131,7 +131,7 @@ class AccessibilityHighlightManagerTest : public InProcessBrowserTest {
SkColor average_diff_color() { return average_diff_color_; } SkColor average_diff_color() { return average_diff_color_; }
private: private:
void GotSnapshot(const gfx::Image& image) { void GotSnapshot(gfx::Image image) {
image_ = image; image_ = image;
content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
run_loop_quitter_); run_loop_quitter_);
......
...@@ -42,11 +42,13 @@ ...@@ -42,11 +42,13 @@
#include "components/arc/instance_holder.h" #include "components/arc/instance_holder.h"
#include "components/session_manager/core/session_manager.h" #include "components/session_manager/core/session_manager.h"
#include "content/public/browser/browser_thread.h" #include "content/public/browser/browser_thread.h"
#include "third_party/skia/include/core/SkBitmap.h"
#include "ui/aura/client/aura_constants.h" #include "ui/aura/client/aura_constants.h"
#include "ui/aura/window.h" #include "ui/aura/window.h"
#include "ui/compositor/layer.h" #include "ui/compositor/layer.h"
#include "ui/compositor/layer_owner.h" #include "ui/compositor/layer_owner.h"
#include "ui/compositor/layer_tree_owner.h" #include "ui/compositor/layer_tree_owner.h"
#include "ui/gfx/codec/jpeg_codec.h"
#include "ui/gfx/geometry/rect.h" #include "ui/gfx/geometry/rect.h"
#include "ui/gfx/image/image.h" #include "ui/gfx/image/image.h"
#include "ui/gfx/image/image_util.h" #include "ui/gfx/image/image_util.h"
...@@ -158,18 +160,21 @@ std::unique_ptr<ui::LayerTreeOwner> CreateLayerTreeForSnapshot( ...@@ -158,18 +160,21 @@ std::unique_ptr<ui::LayerTreeOwner> CreateLayerTreeForSnapshot(
void EncodeAndReturnImage( void EncodeAndReturnImage(
ArcVoiceInteractionFrameworkService::CaptureFullscreenCallback callback, ArcVoiceInteractionFrameworkService::CaptureFullscreenCallback callback,
std::unique_ptr<ui::LayerTreeOwner> old_layer_owner, std::unique_ptr<ui::LayerTreeOwner> old_layer_owner,
const gfx::Image& image) { gfx::Image image) {
old_layer_owner.reset(); old_layer_owner.reset();
base::PostTaskWithTraitsAndReplyWithResult( base::PostTaskWithTraitsAndReplyWithResult(
FROM_HERE, FROM_HERE,
base::TaskTraits{base::MayBlock(), base::TaskPriority::USER_BLOCKING}, base::TaskTraits{base::MayBlock(), base::TaskPriority::USER_BLOCKING},
// We use SkBitmap here to avoid passing in gfx::Image directly, which
// shares a single gfx::ImageStorage that's not threadsafe.
// Alternatively, we could also pass in |image| with std::move().
base::BindOnce( base::BindOnce(
[](const gfx::Image& image) -> std::vector<uint8_t> { [](SkBitmap image) -> std::vector<uint8_t> {
std::vector<uint8_t> res; std::vector<uint8_t> res;
gfx::JPEG1xEncodedDataFromImage(image, 100, &res); gfx::JPEGCodec::Encode(image, 100, &res);
return res; return res;
}, },
image), image.AsBitmap()),
std::move(callback)); std::move(callback));
} }
......
...@@ -125,7 +125,7 @@ void DesktopMediaListAsh::CaptureThumbnail(content::DesktopMediaID id, ...@@ -125,7 +125,7 @@ void DesktopMediaListAsh::CaptureThumbnail(content::DesktopMediaID id,
} }
void DesktopMediaListAsh::OnThumbnailCaptured(content::DesktopMediaID id, void DesktopMediaListAsh::OnThumbnailCaptured(content::DesktopMediaID id,
const gfx::Image& image) { gfx::Image image) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
UpdateSourceThumbnail(id, image.AsImageSkia()); UpdateSourceThumbnail(id, image.AsImageSkia());
......
...@@ -37,8 +37,7 @@ class DesktopMediaListAsh : public DesktopMediaListBase { ...@@ -37,8 +37,7 @@ class DesktopMediaListAsh : public DesktopMediaListBase {
void EnumerateSources( void EnumerateSources(
std::vector<DesktopMediaListAsh::SourceDescription>* windows); std::vector<DesktopMediaListAsh::SourceDescription>* windows);
void CaptureThumbnail(content::DesktopMediaID id, aura::Window* window); void CaptureThumbnail(content::DesktopMediaID id, aura::Window* window);
void OnThumbnailCaptured(content::DesktopMediaID id, void OnThumbnailCaptured(content::DesktopMediaID id, gfx::Image image);
const gfx::Image& image);
int pending_window_capture_requests_ = 0; int pending_window_capture_requests_ = 0;
......
...@@ -330,7 +330,7 @@ void NativeDesktopMediaList::CaptureAuraWindowThumbnail( ...@@ -330,7 +330,7 @@ void NativeDesktopMediaList::CaptureAuraWindowThumbnail(
} }
void NativeDesktopMediaList::OnAuraThumbnailCaptured(const DesktopMediaID& id, void NativeDesktopMediaList::OnAuraThumbnailCaptured(const DesktopMediaID& id,
const gfx::Image& image) { gfx::Image image) {
if (!image.IsEmpty()) { if (!image.IsEmpty()) {
// Only new or changed thumbnail need update. // Only new or changed thumbnail need update.
new_aura_thumbnail_hashes_[id] = GetImageHash(image); new_aura_thumbnail_hashes_[id] = GetImageHash(image);
......
...@@ -41,7 +41,7 @@ class NativeDesktopMediaList : public DesktopMediaListBase { ...@@ -41,7 +41,7 @@ class NativeDesktopMediaList : public DesktopMediaListBase {
#if defined(USE_AURA) #if defined(USE_AURA)
void CaptureAuraWindowThumbnail(const content::DesktopMediaID& id); void CaptureAuraWindowThumbnail(const content::DesktopMediaID& id);
void OnAuraThumbnailCaptured(const content::DesktopMediaID& id, void OnAuraThumbnailCaptured(const content::DesktopMediaID& id,
const gfx::Image& image); gfx::Image image);
#endif #endif
// Task runner used for the |worker_|. // Task runner used for the |worker_|.
......
...@@ -2556,7 +2556,7 @@ void RenderWidgetHostImpl::OnSnapshotFromSurfaceReceived( ...@@ -2556,7 +2556,7 @@ void RenderWidgetHostImpl::OnSnapshotFromSurfaceReceived(
} }
void RenderWidgetHostImpl::OnSnapshotReceived(int snapshot_id, void RenderWidgetHostImpl::OnSnapshotReceived(int snapshot_id,
const gfx::Image& image) { gfx::Image image) {
// Any pending snapshots with a lower ID than the one received are considered // Any pending snapshots with a lower ID than the one received are considered
// to be implicitly complete, and returned the same snapshot data. // to be implicitly complete, and returned the same snapshot data.
PendingSnapshotMap::iterator it = pending_browser_snapshots_.begin(); PendingSnapshotMap::iterator it = pending_browser_snapshots_.begin();
......
...@@ -770,7 +770,7 @@ class CONTENT_EXPORT RenderWidgetHostImpl ...@@ -770,7 +770,7 @@ class CONTENT_EXPORT RenderWidgetHostImpl
const SkBitmap& bitmap, const SkBitmap& bitmap,
ReadbackResponse response); ReadbackResponse response);
void OnSnapshotReceived(int snapshot_id, const gfx::Image& image); void OnSnapshotReceived(int snapshot_id, gfx::Image image);
// 1. Grants permissions to URL (if any) // 1. Grants permissions to URL (if any)
// 2. Grants permissions to filenames // 2. Grants permissions to filenames
......
...@@ -4,6 +4,8 @@ ...@@ -4,6 +4,8 @@
#include "ui/snapshot/snapshot.h" #include "ui/snapshot/snapshot.h"
#include <utility>
#include "base/bind.h" #include "base/bind.h"
#include "base/callback.h" #include "base/callback.h"
#include "base/task_scheduler/post_task.h" #include "base/task_scheduler/post_task.h"
...@@ -37,10 +39,10 @@ void EncodeImageAndScheduleCallback( ...@@ -37,10 +39,10 @@ void EncodeImageAndScheduleCallback(
scoped_refptr<base::RefCountedMemory> (*encode_func)(const gfx::Image&), scoped_refptr<base::RefCountedMemory> (*encode_func)(const gfx::Image&),
const base::Callback<void(scoped_refptr<base::RefCountedMemory> data)>& const base::Callback<void(scoped_refptr<base::RefCountedMemory> data)>&
callback, callback,
const gfx::Image& image) { gfx::Image image) {
base::PostTaskWithTraitsAndReplyWithResult( base::PostTaskWithTraitsAndReplyWithResult(
FROM_HERE, {base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}, FROM_HERE, {base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
base::Bind(encode_func, image), callback); base::Bind(encode_func, std::move(image)), callback);
} }
} // namespace } // namespace
......
...@@ -38,7 +38,7 @@ SNAPSHOT_EXPORT bool GrabViewSnapshot(gfx::NativeView view, ...@@ -38,7 +38,7 @@ SNAPSHOT_EXPORT bool GrabViewSnapshot(gfx::NativeView view,
// These functions take a snapshot of |source_rect|, specified in layer space // These functions take a snapshot of |source_rect|, specified in layer space
// coordinates (DIP for desktop, physical pixels for Android), and scale the // coordinates (DIP for desktop, physical pixels for Android), and scale the
// snapshot to |target_size| (in physical pixels), asynchronously. // snapshot to |target_size| (in physical pixels), asynchronously.
typedef base::Callback<void(const gfx::Image& snapshot)> typedef base::Callback<void(gfx::Image snapshot)>
GrabWindowSnapshotAsyncCallback; GrabWindowSnapshotAsyncCallback;
SNAPSHOT_EXPORT void GrabWindowSnapshotAndScaleAsync( SNAPSHOT_EXPORT void GrabWindowSnapshotAndScaleAsync(
gfx::NativeWindow window, gfx::NativeWindow window,
......
...@@ -19,7 +19,7 @@ namespace { ...@@ -19,7 +19,7 @@ namespace {
void OnFrameScalingFinished(const GrabWindowSnapshotAsyncCallback& callback, void OnFrameScalingFinished(const GrabWindowSnapshotAsyncCallback& callback,
const SkBitmap& scaled_bitmap) { const SkBitmap& scaled_bitmap) {
callback.Run(gfx::Image(gfx::ImageSkia::CreateFrom1xBitmap(scaled_bitmap))); callback.Run(gfx::Image::CreateFrom1xBitmap(scaled_bitmap));
} }
SkBitmap ScaleBitmap(const SkBitmap& input_bitmap, SkBitmap ScaleBitmap(const SkBitmap& input_bitmap,
......
...@@ -137,7 +137,7 @@ void GrabViewSnapshotAsync(gfx::NativeView view, ...@@ -137,7 +137,7 @@ void GrabViewSnapshotAsync(gfx::NativeView view,
void GrabLayerSnapshotAsync(ui::Layer* layer, void GrabLayerSnapshotAsync(ui::Layer* layer,
const gfx::Rect& source_rect, const gfx::Rect& source_rect,
const GrabLayerSnapshotCallback& callback) { const GrabWindowSnapshotAsyncCallback& callback) {
MakeAsyncCopyRequest( MakeAsyncCopyRequest(
layer, source_rect, layer, source_rect,
base::BindOnce(&SnapshotAsync::RunCallbackWithCopyOutputResult, base::BindOnce(&SnapshotAsync::RunCallbackWithCopyOutputResult,
......
...@@ -26,15 +26,12 @@ SNAPSHOT_EXPORT void GrabWindowSnapshotAsyncAura( ...@@ -26,15 +26,12 @@ SNAPSHOT_EXPORT void GrabWindowSnapshotAsyncAura(
const gfx::Rect& source_rect, const gfx::Rect& source_rect,
const GrabWindowSnapshotAsyncCallback& callback); const GrabWindowSnapshotAsyncCallback& callback);
using GrabLayerSnapshotCallback =
base::Callback<void(const gfx::Image& snapshot)>;
// Grabs a snapshot of a |layer| and all its descendants. // Grabs a snapshot of a |layer| and all its descendants.
// |source_rect| is the bounds of the snapshot content relative to |layer|. // |source_rect| is the bounds of the snapshot content relative to |layer|.
SNAPSHOT_EXPORT void GrabLayerSnapshotAsync( SNAPSHOT_EXPORT void GrabLayerSnapshotAsync(
Layer* layer, Layer* layer,
const gfx::Rect& source_rect, const gfx::Rect& source_rect,
const GrabLayerSnapshotCallback& callback); const GrabWindowSnapshotAsyncCallback& callback);
} // namespace ui } // namespace ui
......
...@@ -156,16 +156,14 @@ class SnapshotAuraTest : public testing::Test { ...@@ -156,16 +156,14 @@ class SnapshotAuraTest : public testing::Test {
public: public:
SnapshotHolder() : completed_(false) {} SnapshotHolder() : completed_(false) {}
void SnapshotCallback(const gfx::Image& image) { void SnapshotCallback(gfx::Image image) {
DCHECK(!completed_); DCHECK(!completed_);
image_ = image; image_ = image;
completed_ = true; completed_ = true;
run_loop_.Quit(); run_loop_.Quit();
} }
void WaitForSnapshot() { run_loop_.Run(); } void WaitForSnapshot() { run_loop_.Run(); }
bool completed() const { bool completed() const { return completed_; }
return completed_;
};
const gfx::Image& image() const { return image_; } const gfx::Image& image() const { return image_; }
private: private:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment