cc: Do bitmap conversion for RasterBuffer in the worker thread.

cc/resources/resource_provider.cc

Index: cc/resources/resource_provider.cc
diff --git a/cc/resources/resource_provider.cc b/cc/resources/resource_provider.cc
index 944f8c583d710992f115a835acf7bc1acc78df50..a94f14c94a5ee9900855d5580d52aeb91cbde130 100644
--- a/cc/resources/resource_provider.cc
+++ b/cc/resources/resource_provider.cc
@@ -121,6 +121,34 @@ class IdentityAllocator : public SkBitmap::Allocator {
   void* buffer_;
 };
 
+void MakeBitmap(SkBitmap* bitmap,
+                uint8_t* buffer,
+                ResourceFormat format,
+                const gfx::Size& size,
+                int stride) {
+  switch (format) {
+    case RGBA_4444:
+      // Use the default stride if we will eventually convert this
+      // bitmap to 4444.
+      bitmap->allocN32Pixels(size.width(), size.height());
+      break;
+    case RGBA_8888:
+    case BGRA_8888: {
+      SkImageInfo info =
+          SkImageInfo::MakeN32Premul(size.width(), size.height());
+      if (0 == stride)
+        stride = info.minRowBytes();
+      bitmap->installPixels(info, buffer, stride);
+      break;
+    }
+    case LUMINANCE_8:
+    case RGB_565:
+    case ETC1:
+      NOTREACHED();
+      break;
+  }
+}
+
 void CopyBitmap(const SkBitmap& src, uint8_t* dst, SkColorType dst_colorType) {
   SkBitmap dst_bitmap;
   IdentityAllocator allocator(dst);
@@ -402,76 +430,48 @@ ResourceProvider::GpuRasterBuffer::GpuRasterBuffer(
     bool use_distance_field_text)
     : resource_(resource),
       resource_provider_(resource_provider),
-      locked_canvas_(NULL),
-      canvas_save_count_(0),
       surface_generation_id_(0u),
       use_distance_field_text_(use_distance_field_text) {
-}
-
-ResourceProvider::GpuRasterBuffer::~GpuRasterBuffer() {
-}
-
-void ResourceProvider::GpuRasterBuffer::LockForWrite() {
-  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-               "ResourceProvider::GpuRasterBuffer::LockForWrite");
-
-  DCHECK(!locked_canvas_);
-
-  if (!surface_)
-    surface_ = CreateSurface();
+  surface_ = CreateSurface();
   surface_generation_id_ = surface_ ? surface_->generationID() : 0u;
 }
 
-bool ResourceProvider::GpuRasterBuffer::UnlockForWrite() {
-  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-               "ResourceProvider::GpuRasterBuffer::UnlockForWrite");
-
-  DCHECK(!locked_canvas_);
-
-  // generationID returns a non-zero, unique value corresponding to the content
-  // of surface. Hence, a change since LockForWrite was called means the surface
-  // has changed.
-  return surface_ ? surface_generation_id_ != surface_->generationID() : false;
+ResourceProvider::GpuRasterBuffer::~GpuRasterBuffer() {
 }
 
-SkCanvas* ResourceProvider::GpuRasterBuffer::AcquireSkCanvas() {
+skia::RefPtr<SkCanvas> ResourceProvider::GpuRasterBuffer::AcquireSkCanvas() {
   // Note that this function is called from a worker thread.
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "ResourceProvider::GpuRasterBuffer::AcquireSkCanvas");
 
-  DCHECK(!locked_canvas_);
-  locked_canvas_ = surface_ ? surface_->getCanvas() : NULL;
-  canvas_save_count_ = locked_canvas_ ? locked_canvas_->save() : 0;
-  return locked_canvas_;
+  return surface_ ? skia::SharePtr(surface_->getCanvas())
+                  : skia::RefPtr<SkCanvas>();
 }
 
-void ResourceProvider::GpuRasterBuffer::ReleaseSkCanvas() {
+bool ResourceProvider::GpuRasterBuffer::ReleaseSkCanvas(SkCanvas* canvas) {
   // Note that this function is called from a worker thread.
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "ResourceProvider::GpuRasterBuffer::ReleaseSkCanvas");
 
-  if (locked_canvas_) {
-    locked_canvas_->restoreToCount(canvas_save_count_);
-    locked_canvas_ = NULL;
-  }
+  return surface_ ? surface_generation_id_ != surface_->generationID() : false;
 }
 
 skia::RefPtr<SkSurface> ResourceProvider::GpuRasterBuffer::CreateSurface() {

[reveman, 2014/08/13 19:19:50]

Is it still worth having this function? Looks like it might clean things up a
bit by moving this code into the ctor..

[auygun, 2014/08/14 10:35:42]

Done.

-  DCHECK_EQ(GLTexture, resource()->type);
-  DCHECK(resource()->gl_id);
+  DCHECK_EQ(GLTexture, resource_->type);
+  DCHECK(resource_->gl_id);
 
-  class GrContext* gr_context = resource_provider()->GrContext();
+  class GrContext* gr_context = resource_provider_->GrContext();
   // TODO(alokp): Implement TestContextProvider::GrContext().
   if (!gr_context)
     return skia::RefPtr<SkSurface>();
 
   GrBackendTextureDesc desc;
   desc.fFlags = kRenderTarget_GrBackendTextureFlag;
-  desc.fWidth = resource()->size.width();
-  desc.fHeight = resource()->size.height();
-  desc.fConfig = ToGrPixelConfig(resource()->format);
+  desc.fWidth = resource_->size.width();
+  desc.fHeight = resource_->size.height();
+  desc.fConfig = ToGrPixelConfig(resource_->format);
   desc.fOrigin = kTopLeft_GrSurfaceOrigin;
-  desc.fTextureHandle = resource()->gl_id;
+  desc.fTextureHandle = resource_->gl_id;
   skia::RefPtr<GrTexture> gr_texture =
       skia::AdoptRef(gr_context->wrapBackendTexture(desc));
   SkSurface::TextRenderMode text_render_mode =
@@ -481,134 +481,140 @@ skia::RefPtr<SkSurface> ResourceProvider::GpuRasterBuffer::CreateSurface() {
       gr_texture->asRenderTarget(), text_render_mode));
 }
 
-ResourceProvider::BitmapRasterBuffer::BitmapRasterBuffer(
+ResourceProvider::ImageRasterBuffer::ImageRasterBuffer(
     const Resource* resource,
     ResourceProvider* resource_provider)
     : resource_(resource),
       resource_provider_(resource_provider),
-      canvas_save_count_(0),
       mapped_buffer_(NULL),
       raster_bitmap_generation_id_(0u),
-      raster_bitmap_changed_(false),
       stride_(0) {
 }
 
-ResourceProvider::BitmapRasterBuffer::~BitmapRasterBuffer() {}
+ResourceProvider::ImageRasterBuffer::~ImageRasterBuffer() {
+}
 
-void ResourceProvider::BitmapRasterBuffer::LockForWrite() {
+void ResourceProvider::ImageRasterBuffer::LockForWrite() {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-               "ResourceProvider::BitmapRasterBuffer::LockForWrite");
+               "ResourceProvider::ImageRasterBuffer::LockForWrite");
 
   DCHECK(!mapped_buffer_);
-  DCHECK(!raster_canvas_);
 
   stride_ = 0;
-  mapped_buffer_ = MapBuffer(&stride_);
-  raster_bitmap_changed_ = false;
+  mapped_buffer_ = resource_provider_->MapImage(resource_, &stride_);
 }
 
-bool ResourceProvider::BitmapRasterBuffer::UnlockForWrite() {
+void ResourceProvider::ImageRasterBuffer::UnlockForWrite() {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-               "ResourceProvider::BitmapRasterBuffer::UnlockForWrite");
-
-  DCHECK(!raster_canvas_);
+               "ResourceProvider::ImageRasterBuffer::UnlockForWrite");
 
-  UnmapBuffer();
+  resource_provider_->UnmapImage(resource_);
   mapped_buffer_ = NULL;
-  return raster_bitmap_changed_;
 }
 
-SkCanvas* ResourceProvider::BitmapRasterBuffer::AcquireSkCanvas() {
+skia::RefPtr<SkCanvas> ResourceProvider::ImageRasterBuffer::AcquireSkCanvas() {
   // Note that this function is called from a worker thread.
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-               "ResourceProvider::BitmapRasterBuffer::AcquireSkCanvas");
+               "ResourceProvider::ImageRasterBuffer::AcquireSkCanvas");
 
   if (!mapped_buffer_)
-    return NULL;
-
-  switch (resource()->format) {
-    case RGBA_4444:
-      // Use the default stride if we will eventually convert this
-      // bitmap to 4444.
-      raster_bitmap_.allocN32Pixels(resource()->size.width(),
-                                    resource()->size.height());
-      break;
-    case RGBA_8888:
-    case BGRA_8888: {
-      SkImageInfo info = SkImageInfo::MakeN32Premul(resource()->size.width(),
-                                                    resource()->size.height());
-      if (0 == stride_)
-        stride_ = info.minRowBytes();
-      raster_bitmap_.installPixels(info, mapped_buffer_, stride_);
-      break;
-    }
-    case LUMINANCE_8:
-    case RGB_565:
-    case ETC1:
-      NOTREACHED();
-      break;
-  }
+    return skia::RefPtr<SkCanvas>();
 
+  MakeBitmap(&raster_bitmap_,
+             mapped_buffer_,
+             resource_->format,
+             resource_->size,
+             stride_);
   raster_bitmap_generation_id_ = raster_bitmap_.getGenerationID();

[reveman, 2014/08/13 19:19:49]

Let's not bother with the generation Id anymore. That was only useful when
returning a SkCanvas on the compositor thread. Now we can simply assume that the
buffer has been modified if Acquire/ReleaseSkCanvas has been called.

[auygun, 2014/08/14 10:35:42]

Done.

-
-  DCHECK(!raster_canvas_);
-  raster_canvas_ = skia::AdoptRef(new SkCanvas(raster_bitmap_));
-  canvas_save_count_ = raster_canvas_->save();
-  return raster_canvas_.get();
+  return skia::AdoptRef(new SkCanvas(raster_bitmap_));
 }
 
-void ResourceProvider::BitmapRasterBuffer::ReleaseSkCanvas() {
+bool ResourceProvider::ImageRasterBuffer::ReleaseSkCanvas(SkCanvas* canvas) {
   // Note that this function is called from a worker thread.
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-               "ResourceProvider::BitmapRasterBuffer::ReleaseSkCanvas");
-
-  if (raster_canvas_) {
-    raster_canvas_->restoreToCount(canvas_save_count_);
-    raster_canvas_.clear();
-  }
+               "ResourceProvider::ImageRasterBuffer::ReleaseSkCanvas");
 
   // getGenerationID returns a non-zero, unique value corresponding to the
   // pixels in bitmap. Hence, a change since LockForWrite was called means the
   // bitmap has changed.
-  raster_bitmap_changed_ =
+  bool raster_bitmap_changed =
       raster_bitmap_generation_id_ != raster_bitmap_.getGenerationID();

[reveman, 2014/08/13 19:19:50]

Remove generation id related code. It can be assumed that the buffer was
generated if Acquire/ReleaseSkCanvas has been called.

[auygun, 2014/08/14 10:35:42]

Done.

-  if (raster_bitmap_changed_) {
+  if (raster_bitmap_changed) {
     SkColorType buffer_colorType =
-        ResourceFormatToSkColorType(resource()->format);
+        ResourceFormatToSkColorType(resource_->format);
     if (mapped_buffer_ && (buffer_colorType != raster_bitmap_.colorType()))
       CopyBitmap(raster_bitmap_, mapped_buffer_, buffer_colorType);
   }
   raster_bitmap_.reset();
+  return raster_bitmap_changed;
 }
 
-ResourceProvider::ImageRasterBuffer::ImageRasterBuffer(
+ResourceProvider::PixelRasterBuffer::PixelRasterBuffer(
     const Resource* resource,
     ResourceProvider* resource_provider)
-    : BitmapRasterBuffer(resource, resource_provider) {}
+    : resource_(resource),
+      resource_provider_(resource_provider),
+      mapped_buffer_(NULL),
+      raster_bitmap_generation_id_(0u),
+      stride_(0) {
+}
+
+ResourceProvider::PixelRasterBuffer::~PixelRasterBuffer() {
+}
+
+void ResourceProvider::PixelRasterBuffer::LockForWrite() {
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+               "ResourceProvider::PixelRasterBuffer::LockForWrite");
 
-ResourceProvider::ImageRasterBuffer::~ImageRasterBuffer() {}
+  DCHECK(!mapped_buffer_);
 
-uint8_t* ResourceProvider::ImageRasterBuffer::MapBuffer(int* stride) {
-  return resource_provider()->MapImage(resource(), stride);
+  stride_ = 0;
+  mapped_buffer_ = resource_provider_->MapPixelBuffer(resource_, &stride_);
 }
 
-void ResourceProvider::ImageRasterBuffer::UnmapBuffer() {
-  resource_provider()->UnmapImage(resource());
+void ResourceProvider::PixelRasterBuffer::UnlockForWrite() {
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+               "ResourceProvider::PixelRasterBuffer::UnlockForWrite");
+
+  resource_provider_->UnmapPixelBuffer(resource_);
+  mapped_buffer_ = NULL;
 }
 
-ResourceProvider::PixelRasterBuffer::PixelRasterBuffer(
-    const Resource* resource,
-    ResourceProvider* resource_provider)
-    : BitmapRasterBuffer(resource, resource_provider) {}
+skia::RefPtr<SkCanvas> ResourceProvider::PixelRasterBuffer::AcquireSkCanvas() {
+  // Note that this function is called from a worker thread.
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+               "ResourceProvider::PixelRasterBuffer::AcquireSkCanvas");
 
-ResourceProvider::PixelRasterBuffer::~PixelRasterBuffer() {}
+  if (!mapped_buffer_)
+    return skia::RefPtr<SkCanvas>();
 
-uint8_t* ResourceProvider::PixelRasterBuffer::MapBuffer(int* stride) {
-  return resource_provider()->MapPixelBuffer(resource(), stride);
+  MakeBitmap(&raster_bitmap_,
+             mapped_buffer_,
+             resource_->format,
+             resource_->size,
+             stride_);
+  raster_bitmap_generation_id_ = raster_bitmap_.getGenerationID();
+  return skia::AdoptRef(new SkCanvas(raster_bitmap_));
 }
 
-void ResourceProvider::PixelRasterBuffer::UnmapBuffer() {
-  resource_provider()->UnmapPixelBuffer(resource());
+bool ResourceProvider::PixelRasterBuffer::ReleaseSkCanvas(SkCanvas* canvas) {
+  // Note that this function is called from a worker thread.
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+               "ResourceProvider::PixelRasterBuffer::ReleaseSkCanvas");
+
+  // getGenerationID returns a non-zero, unique value corresponding to the
+  // pixels in bitmap. Hence, a change since LockForWrite was called means the
+  // bitmap has changed.
+  bool raster_bitmap_changed =
+      raster_bitmap_generation_id_ != raster_bitmap_.getGenerationID();

[reveman, 2014/08/13 19:19:49]

Same here. Remove generation id related code.

[auygun, 2014/08/14 10:35:42]

Done.

+  if (raster_bitmap_changed) {
+    SkColorType buffer_colorType =
+        ResourceFormatToSkColorType(resource_->format);
+    if (mapped_buffer_ && (buffer_colorType != raster_bitmap_.colorType()))
+      CopyBitmap(raster_bitmap_, mapped_buffer_, buffer_colorType);
+  }
+  raster_bitmap_.reset();
+  return raster_bitmap_changed;
 }
 
 ResourceProvider::Child::Child() : marked_for_deletion(false) {}
@@ -1768,14 +1774,12 @@ RasterBuffer* ResourceProvider::AcquireGpuRasterBuffer(ResourceId id) {
     resource->gpu_raster_buffer.reset(
         new GpuRasterBuffer(resource, this, use_distance_field_text_));
   }
-  resource->gpu_raster_buffer->LockForWrite();
   return resource->gpu_raster_buffer.get();
 }
 
 void ResourceProvider::ReleaseGpuRasterBuffer(ResourceId id) {
   Resource* resource = GetResource(id);
   DCHECK(resource->gpu_raster_buffer.get());
-  resource->gpu_raster_buffer->UnlockForWrite();
   UnlockForWrite(id);
 }
 
@@ -1788,35 +1792,26 @@ RasterBuffer* ResourceProvider::AcquireImageRasterBuffer(ResourceId id) {
   return resource->image_raster_buffer.get();
 }
 
-bool ResourceProvider::ReleaseImageRasterBuffer(ResourceId id) {
+void ResourceProvider::ReleaseImageRasterBuffer(ResourceId id) {
   Resource* resource = GetResource(id);
   resource->dirty_image = true;
-  return resource->image_raster_buffer->UnlockForWrite();
+  resource->image_raster_buffer->UnlockForWrite();
 }
 
-void ResourceProvider::AcquirePixelRasterBuffer(ResourceId id) {
+RasterBuffer* ResourceProvider::AcquirePixelRasterBuffer(ResourceId id) {
   Resource* resource = GetResource(id);
   AcquirePixelBuffer(resource);
   resource->pixel_raster_buffer.reset(new PixelRasterBuffer(resource, this));
-}
-
-void ResourceProvider::ReleasePixelRasterBuffer(ResourceId id) {
-  Resource* resource = GetResource(id);
-  resource->pixel_raster_buffer.reset();
-  ReleasePixelBuffer(resource);
-}
-
-RasterBuffer* ResourceProvider::MapPixelRasterBuffer(ResourceId id) {
-  Resource* resource = GetResource(id);
-  DCHECK(resource->pixel_raster_buffer.get());
   resource->pixel_raster_buffer->LockForWrite();
   return resource->pixel_raster_buffer.get();
 }
 
-bool ResourceProvider::UnmapPixelRasterBuffer(ResourceId id) {
+void ResourceProvider::ReleasePixelRasterBuffer(ResourceId id) {
   Resource* resource = GetResource(id);
   DCHECK(resource->pixel_raster_buffer.get());
-  return resource->pixel_raster_buffer->UnlockForWrite();
+  resource->pixel_raster_buffer->UnlockForWrite();
+  resource->pixel_raster_buffer.reset();
+  ReleasePixelBuffer(resource);
 }
 
 void ResourceProvider::AcquirePixelBuffer(Resource* resource) {
@@ -1952,6 +1947,9 @@ void ResourceProvider::BeginSetPixels(ResourceId id) {
   Resource* resource = GetResource(id);
   DCHECK(!resource->pending_set_pixels);
 
+  DCHECK(resource->pixel_raster_buffer.get());
+  resource->pixel_raster_buffer->UnlockForWrite();
+
   LazyCreate(resource);
   DCHECK(resource->origin == Resource::Internal);
   DCHECK(resource->gl_id || resource->allocated);