Skip to content

Commit

Permalink
Change PlatformBitmap memory ownership story. Fix CopyFromBackingStor…
Browse files Browse the repository at this point in the history 
…e threading issues.

PlatformBitmap: restructure this class so that, on all platforms,
it is safe to use an SkBitmap derived from the PlatformBitmap
even after the PlatformBitmap is destroyed. In practice
this means changes to Linux (use a different cairo
creation routine that allows us to allocate the memory) and
Windows (the HDC is owned by PlatformBitmap, the HBITMAP
by the SkPixelRef).

CopyFromBackingStore: instead of requiring the caller to
provide a PlatformBitmap, modify the signature so that the
completion callback accepts an SkBitmap. Sometimes, the backing store
copiers will allocate a PlatformBitmap and pass its SkBitmap
to the completion callback, but this becomes merely an
implementation detail. Meanwhile, in the accelerated case,
it is not at all necessary to allocate a PlatformBitmap,
so don't.

This fixes a bug on Linux where the cairo surface context
was being freed on a thread other than the UI thread.
PlatformBitmap is basically not thread safe on Linux,
and this change fixes that. Also fixed is a Dr. Memory
GDI usage warning -- we are sure to de-select the
HBITMAP before deleting the memory DC.

Lastly, moving CopyFromBackingStore's interface to use a
callee-managed SkBitmap conditions the architecture for doing
RGBA->YUV on the GPU, prior to readback.

BUG=109963,159234,161537

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@180271 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
nick@chromium.org committed Feb 2, 2013
1 parent 89ae13e commit d748d0f
Show file tree
Hide file tree
Showing 38 changed files with 366 additions and 280 deletions.
11 changes: 4 additions & 7 deletions chrome/browser/extensions/api/tabs/tabs_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1708,23 +1708,20 @@ bool TabsCaptureVisibleTabFunction::RunImpl() {
error_ = keys::kInternalVisibleTabCaptureError;
return false;
}
skia::PlatformBitmap* temp_bitmap = new skia::PlatformBitmap;
render_view_host->CopyFromBackingStore(
gfx::Rect(),
view->GetViewBounds().size(),
base::Bind(&TabsCaptureVisibleTabFunction::CopyFromBackingStoreComplete,
this,
base::Owned(temp_bitmap)),
temp_bitmap);
this));
return true;
}

void TabsCaptureVisibleTabFunction::CopyFromBackingStoreComplete(
skia::PlatformBitmap* bitmap,
bool succeeded) {
bool succeeded,
const SkBitmap& bitmap) {
if (succeeded) {
VLOG(1) << "captureVisibleTab() got image from backing store.";
SendResultFromBitmap(bitmap->GetBitmap());
SendResultFromBitmap(bitmap);
return;
}

Expand Down
8 changes: 2 additions & 6 deletions chrome/browser/extensions/api/tabs/tabs_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ struct InjectDetails;
} // namespace api
} // namespace extensions

namespace skia {
class PlatformBitmap;
}

// Windows
class WindowsGetFunction : public SyncExtensionFunction {
virtual ~WindowsGetFunction() {}
Expand Down Expand Up @@ -201,8 +197,8 @@ class TabsCaptureVisibleTabFunction : public AsyncExtensionFunction,
void SendResultFromBitmap(const SkBitmap& screen_capture);

private:
void CopyFromBackingStoreComplete(skia::PlatformBitmap* bitmap,
bool succeeded);
void CopyFromBackingStoreComplete(bool succeeded,
const SkBitmap& bitmap);

content::NotificationRegistrar registrar_;

Expand Down
11 changes: 4 additions & 7 deletions chrome/browser/prerender/prerender_tab_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,23 @@ class PrerenderTabHelper::PixelStats {
return;
}

skia::PlatformBitmap* temp_bitmap = new skia::PlatformBitmap;
web_contents->GetRenderViewHost()->CopyFromBackingStore(
gfx::Rect(),
gfx::Size(),
base::Bind(&PrerenderTabHelper::PixelStats::HandleBitmapResult,
weak_factory_.GetWeakPtr(),
bitmap_type,
web_contents,
base::Owned(temp_bitmap)),
temp_bitmap);
web_contents));
}

private:
void HandleBitmapResult(BitmapType bitmap_type,
WebContents* web_contents,
skia::PlatformBitmap* temp_bitmap,
bool succeeded) {
bool succeeded,
const SkBitmap& canvas_bitmap) {
scoped_ptr<SkBitmap> bitmap;
if (succeeded) {
const SkBitmap& canvas_bitmap = temp_bitmap->GetBitmap();
// TODO(nick): This copy may now be unnecessary.
bitmap.reset(new SkBitmap());
canvas_bitmap.copyTo(bitmap.get(), SkBitmap::kARGB_8888_Config);
}
Expand Down
16 changes: 7 additions & 9 deletions chrome/browser/thumbnails/thumbnail_tab_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "content/public/browser/notification_types.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/render_widget_host_view.h"
#include "skia/ext/platform_canvas.h"
#include "ui/gfx/color_utils.h"
#include "ui/gfx/size_conversions.h"
#include "ui/gfx/screen.h"
Expand All @@ -29,6 +28,8 @@

DEFINE_WEB_CONTENTS_USER_DATA_KEY(ThumbnailTabHelper);

class SkBitmap;

// Overview
// --------
// This class provides a service for updating thumbnails to be used in
Expand Down Expand Up @@ -61,15 +62,14 @@ void UpdateThumbnail(const ThumbnailingContext& context,
<< context.score.ToString();
}

void ProcessCanvas(ThumbnailingContext* context,
ThumbnailingAlgorithm* algorithm,
skia::PlatformBitmap* temp_bitmap,
bool succeeded) {
void ProcessCapturedBitmap(ThumbnailingContext* context,
ThumbnailingAlgorithm* algorithm,
bool succeeded,
const SkBitmap& bitmap) {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
if (!succeeded)
return;

SkBitmap bitmap = temp_bitmap->GetBitmap();
algorithm->ProcessBitmap(context, base::Bind(&UpdateThumbnail), bitmap);
}

Expand Down Expand Up @@ -116,12 +116,10 @@ void AsyncProcessThumbnail(content::WebContents* web_contents,
&copy_rect,
&copy_size);

skia::PlatformBitmap* temp_bitmap = new skia::PlatformBitmap;
render_widget_host->CopyFromBackingStore(
copy_rect,
copy_size,
base::Bind(&ProcessCanvas, context, algorithm, base::Owned(temp_bitmap)),
temp_bitmap);
base::Bind(&ProcessCapturedBitmap, context, algorithm));
}

} // namespace
Expand Down
26 changes: 14 additions & 12 deletions content/browser/renderer_host/compositing_iosurface_mac.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/synchronization/lock.h"
#include "base/time.h"
#include "base/timer.h"
#include "third_party/skia/include/core/SkBitmap.h"
#include "ui/gfx/native_widget_types.h"
#include "ui/gfx/rect.h"
#include "ui/gfx/size.h"
Expand Down Expand Up @@ -58,14 +59,14 @@ class CompositingIOSurfaceMac {
// into |out|. The copied region is specified with |src_pixel_subrect| and
// the data is transformed so that it fits in |dst_pixel_size|.
// |src_pixel_subrect| and |dst_pixel_size| are not in DIP but in pixel.
// Caller must ensure that |out| is allocated with the size no less than
// |4 * dst_pixel_size.width() * dst_pixel_size.height()| bytes.
// Caller must ensure that |out| is allocated to dimensions that match
// dst_pixel_size, with no additional padding.
// |callback| is invoked when the operation is completed or failed.
// Do no call this method again before |callback| is invoked.
void CopyTo(const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
void* out,
const base::Callback<void(bool)>& callback);
const SkBitmap& out,
const base::Callback<void(bool, const SkBitmap&)>& callback);

// Unref the IOSurface and delete the associated GL texture. If the GPU
// process is no longer referencing it, this will delete the IOSurface.
Expand Down Expand Up @@ -170,7 +171,7 @@ class CompositingIOSurfaceMac {
pixel_buffer = 0;
use_fence = false;
fence = 0;
out_buf = NULL;
out_buf.reset();
callback.Reset();
}

Expand All @@ -183,8 +184,8 @@ class CompositingIOSurfaceMac {
GLuint fence;
gfx::Rect src_rect;
gfx::Size dest_size;
void* out_buf;
base::Callback<void(bool)> callback;
SkBitmap out_buf;
base::Callback<void(bool, const SkBitmap&)> callback;
};

CompositingIOSurfaceMac(IOSurfaceSupport* io_surface_support,
Expand Down Expand Up @@ -221,11 +222,12 @@ class CompositingIOSurfaceMac {
// Two implementations of CopyTo() in synchronous and asynchronous mode.
bool SynchronousCopyTo(const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
void* out);
bool AsynchronousCopyTo(const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
void* out,
const base::Callback<void(bool)>& callback);
const SkBitmap& out);
bool AsynchronousCopyTo(
const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
const SkBitmap& out,
const base::Callback<void(bool, const SkBitmap&)>& callback);
void FinishCopy();
void CleanupResourcesForCopy();

Expand Down
27 changes: 14 additions & 13 deletions content/browser/renderer_host/compositing_iosurface_mac.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,
// Make sure we still run the callback if we are being destroyed with an
// active copy_timer_ that has not yet fired.
if (copy_context_.started)
copy_context_.callback.Run(false);
copy_context_.callback.Run(false, SkBitmap());

CVDisplayLinkRelease(display_link_);
CGLSetCurrentContext(cglContext_);
Expand Down Expand Up @@ -435,8 +435,8 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,
void CompositingIOSurfaceMac::CopyTo(
const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
void* out,
const base::Callback<void(bool)>& callback) {
const SkBitmap& out,
const base::Callback<void(bool, const SkBitmap&)>& callback) {
CGLSetCurrentContext(cglContext_);

// Using PBO crashes on Intel drivers but not on newer Mountain Lion
Expand All @@ -457,9 +457,9 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,

if (async_copy) {
if (!ret)
callback.Run(false);
callback.Run(false, SkBitmap());
} else {
callback.Run(ret);
callback.Run(ret, out);
}
}

Expand Down Expand Up @@ -637,7 +637,7 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,
bool CompositingIOSurfaceMac::SynchronousCopyTo(
const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
void* out) {
const SkBitmap& out) {
if (!MapIOSurfaceToTexture(io_surface_handle_))
return false;

Expand Down Expand Up @@ -701,7 +701,7 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,
CGLFlushDrawable(cglContext_);

glReadPixels(0, 0, dst_pixel_size.width(), dst_pixel_size.height(),
GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, out);
GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, out.getPixels());

glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0); CHECK_GL_ERROR();

Expand All @@ -713,8 +713,8 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,
bool CompositingIOSurfaceMac::AsynchronousCopyTo(
const gfx::Rect& src_pixel_subrect,
const gfx::Size& dst_pixel_size,
void* out,
const base::Callback<void(bool)>& callback) {
const SkBitmap& out,
const base::Callback<void(bool, const SkBitmap&)>& callback) {
if (copy_context_.started)
return false;

Expand Down Expand Up @@ -840,24 +840,25 @@ CVReturn DisplayLinkCallback(CVDisplayLinkRef display_link,
}
}
copy_timer_.Stop();

glBindBufferARB(GL_PIXEL_PACK_BUFFER_ARB, copy_context_.pixel_buffer);
CHECK_GL_ERROR();

void* buf = glMapBuffer(GL_PIXEL_PACK_BUFFER_ARB, GL_READ_ONLY_ARB);
CHECK_GL_ERROR();

if (buf) {
memcpy(copy_context_.out_buf, buf, copy_context_.dest_size.GetArea() * 4);
SkAutoLockPixels bitmap_lock(copy_context_.out_buf);
memcpy(copy_context_.out_buf.getPixels(), buf,
copy_context_.dest_size.GetArea() * 4);
glUnmapBufferARB(GL_PIXEL_PACK_BUFFER_ARB); CHECK_GL_ERROR();
}
glBindBufferARB(GL_PIXEL_PACK_BUFFER_ARB, 0); CHECK_GL_ERROR();

base::Callback<void(bool)> callback = copy_context_.callback;
base::Callback<void(bool, const SkBitmap&)> callback = copy_context_.callback;
CleanupResourcesForCopy();
CGLSetCurrentContext(0);

callback.Run(buf != NULL);
callback.Run(buf != NULL, copy_context_.out_buf);
}

void CompositingIOSurfaceMac::CleanupResourcesForCopy() {
Expand Down
Loading

0 comments on commit d748d0f

Please sign in to comment.