cc: Refactor WorkerPoolTaskClient::AcquireBufferForRaster

cc/resources/raster_worker_pool.cc

Index: cc/resources/raster_worker_pool.cc
diff --git a/cc/resources/raster_worker_pool.cc b/cc/resources/raster_worker_pool.cc
index 510c636c0e84a09d5c7b3c5c0896676f5e4eff01..9fb7d737b06121e6fd57ecb90f166ecbfaaa8524 100644
--- a/cc/resources/raster_worker_pool.cc
+++ b/cc/resources/raster_worker_pool.cc
@@ -20,7 +20,6 @@
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkPixelRef.h"
 #include "third_party/skia/include/gpu/GrContext.h"
-#include "third_party/skia/include/gpu/SkGpuDevice.h"
 
 namespace cc {
 namespace {
@@ -94,8 +93,7 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
         source_frame_number_(source_frame_number),
         rendering_stats_(rendering_stats),
         reply_(reply),
-        buffer_(NULL),
-        stride_(0) {}
+        canvas_(NULL) {}
 
   void RunAnalysisOnThread(unsigned thread_index) {
     TRACE_EVENT1("cc",
@@ -122,10 +120,7 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
     analysis_.is_solid_color &= kUseColorEstimator;
   }
 
-  void RunRasterOnThread(unsigned thread_index,
-                         void* buffer,
-                         const gfx::Size& size,
-                         int stride) {
+  void RunRasterOnThread(unsigned thread_index) {
     TRACE_EVENT2(
         "cc",
         "RasterWorkerPoolTaskImpl::RunRasterOnThread",
@@ -138,34 +133,7 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
         devtools_instrumentation::kRasterTask, layer_id_);
 
     DCHECK(picture_pile_.get());
-    DCHECK(buffer);
-
-    SkBitmap bitmap;
-    switch (resource()->format()) {
-      case RGBA_4444:
-        // Use the default stride if we will eventually convert this
-        // bitmap to 4444.
-        bitmap.setConfig(
-            SkBitmap::kARGB_8888_Config, size.width(), size.height());
-        bitmap.allocPixels();
-        break;
-      case RGBA_8888:
-      case BGRA_8888:
-        bitmap.setConfig(
-            SkBitmap::kARGB_8888_Config, size.width(), size.height(), stride);
-        bitmap.setPixels(buffer);
-        break;
-      case LUMINANCE_8:
-      case RGB_565:
-      case ETC1:
-        NOTREACHED();
-        break;
-    }
-
-    SkBitmapDevice device(bitmap);
-    SkCanvas canvas(&device);
-    Raster(picture_pile_->GetCloneForDrawingOnThread(thread_index), &canvas);
-    ChangeBitmapConfigIfNeeded(bitmap, buffer);
+    Raster(picture_pile_->GetCloneForDrawingOnThread(thread_index));
   }
 
   // Overridden from internal::Task:
@@ -173,27 +141,23 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
     // TODO(alokp): For now run-on-worker-thread implies software rasterization.
     DCHECK(!use_gpu_rasterization());
     RunAnalysisOnThread(thread_index);
-    if (buffer_ && !analysis_.is_solid_color)
-      RunRasterOnThread(thread_index, buffer_, resource()->size(), stride_);
+    if (canvas_ && !analysis_.is_solid_color)
+      RunRasterOnThread(thread_index);
   }
 
   // Overridden from internal::WorkerPoolTask:
   virtual void ScheduleOnOriginThread(internal::WorkerPoolTaskClient* client)
       OVERRIDE {
-    if (use_gpu_rasterization())
-      return;
-    DCHECK(!buffer_);
-    buffer_ = client->AcquireBufferForRaster(this, &stride_);
+    DCHECK(!canvas_);
+    canvas_ = client->AcquireCanvasForRaster(this);
   }
   virtual void CompleteOnOriginThread(internal::WorkerPoolTaskClient* client)
       OVERRIDE {
-    if (use_gpu_rasterization())
-      return;
-    buffer_ = NULL;
+    canvas_ = NULL;
     client->OnRasterCompleted(this, analysis_);
   }
   virtual void RunReplyOnOriginThread() OVERRIDE {
-    DCHECK(!buffer_);
+    DCHECK(!canvas_);
     reply_.Run(analysis_, !HasFinishedRunning());
   }
 
@@ -210,31 +174,12 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
             .c_str());
     // TODO(alokp): For now run-on-origin-thread implies gpu rasterization.
     DCHECK(use_gpu_rasterization());
-    ResourceProvider::ScopedWriteLockGL lock(resource_provider,
-                                             resource()->id());
-    DCHECK_NE(lock.texture_id(), 0u);
-
-    GrBackendTextureDesc desc;
-    desc.fFlags = kRenderTarget_GrBackendTextureFlag;
-    desc.fWidth = content_rect_.width();
-    desc.fHeight = content_rect_.height();
-    desc.fConfig = ToGrFormat(resource()->format());
-    desc.fOrigin = kTopLeft_GrSurfaceOrigin;
-    desc.fTextureHandle = lock.texture_id();
-
-    GrContext* gr_context = context_provider->GrContext();
-    skia::RefPtr<GrTexture> texture =
-        skia::AdoptRef(gr_context->wrapBackendTexture(desc));
-    skia::RefPtr<SkGpuDevice> device =
-        skia::AdoptRef(SkGpuDevice::Create(texture.get()));
-    skia::RefPtr<SkCanvas> canvas = skia::AdoptRef(new SkCanvas(device.get()));
-
-    Raster(picture_pile_, canvas.get());
+    Raster(picture_pile_);
     context_provider->ContextGL()->PopGroupMarkerEXT();
   }
 
  protected:
-  virtual ~RasterWorkerPoolTaskImpl() { DCHECK(!buffer_); }
+  virtual ~RasterWorkerPoolTaskImpl() { DCHECK(!canvas_); }
 
  private:
   scoped_ptr<base::Value> DataAsValue() const {
@@ -246,22 +191,7 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
     return res.PassAs<base::Value>();
   }
 
-  static GrPixelConfig ToGrFormat(ResourceFormat format) {
-    switch (format) {
-      case RGBA_8888:
-        return kRGBA_8888_GrPixelConfig;
-      case BGRA_8888:
-        return kBGRA_8888_GrPixelConfig;
-      case RGBA_4444:
-        return kRGBA_4444_GrPixelConfig;
-      default:
-        break;
-    }
-    DCHECK(false) << "Unsupported resource format.";
-    return kSkia8888_GrPixelConfig;
-  }
-
-  void Raster(PicturePileImpl* picture_pile, SkCanvas* canvas) {
+  void Raster(PicturePileImpl* picture_pile) {
     skia::RefPtr<SkDrawFilter> draw_filter;
     switch (raster_mode_) {
       case LOW_QUALITY_RASTER_MODE:
@@ -276,7 +206,7 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
       default:
         NOTREACHED();
     }
-    canvas->setDrawFilter(draw_filter.get());
+    canvas_->setDrawFilter(draw_filter.get());
 
     base::TimeDelta prev_rasterize_time =
         rendering_stats_->impl_thread_rendering_stats().rasterize_time;
@@ -287,7 +217,8 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
     // before we draw and sometimes they aren't)
     RenderingStatsInstrumentation* stats =
         tile_resolution_ == HIGH_RESOLUTION ? rendering_stats_ : NULL;
-    picture_pile->RasterToBitmap(canvas, content_rect_, contents_scale_, stats);
+    picture_pile->RasterToBitmap(
+        canvas_, content_rect_, contents_scale_, stats);
 
     if (rendering_stats_->record_rendering_stats()) {
       base::TimeDelta current_rasterize_time =
@@ -301,19 +232,6 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
     }
   }
 
-  void ChangeBitmapConfigIfNeeded(const SkBitmap& bitmap, void* buffer) {
-    TRACE_EVENT0("cc", "RasterWorkerPoolTaskImpl::ChangeBitmapConfigIfNeeded");
-    SkBitmap::Config config = SkBitmapConfig(resource()->format());
-    if (bitmap.getConfig() != config) {
-      SkBitmap bitmap_dest;
-      IdentityAllocator allocator(buffer);
-      bitmap.copyTo(&bitmap_dest, config, &allocator);
-      // TODO(kaanb): The GL pipeline assumes a 4-byte alignment for the
-      // bitmap data. This check will be removed once crbug.com/293728 is fixed.
-      CHECK_EQ(0u, bitmap_dest.rowBytes() % 4);
-    }
-  }

[reveman, 2014/02/11 02:00:44]

Where does this bitmap conversion code go?

[alokp, 2014/02/11 07:05:21]

It somehow got lost while moving code! Added in BitmapRasterBuffer.

-
   PicturePileImpl::Analysis analysis_;
   scoped_refptr<PicturePileImpl> picture_pile_;
   gfx::Rect content_rect_;
@@ -325,8 +243,7 @@ class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
   int source_frame_number_;
   RenderingStatsInstrumentation* rendering_stats_;
   const RasterWorkerPool::RasterTask::Reply reply_;
-  void* buffer_;
-  int stride_;
+  SkCanvas* canvas_;
 
   DISALLOW_COPY_AND_ASSIGN(RasterWorkerPoolTaskImpl);
 };
@@ -616,6 +533,19 @@ RasterWorkerPool::Task RasterWorkerPool::CreateImageDecodeTask(
       pixel_ref, layer_id, rendering_stats, reply));
 }
 
+SkCanvas* RasterWorkerPool::AcquireCanvasForRaster(
+    internal::RasterWorkerPoolTask* task) {
+  DCHECK(task->use_gpu_rasterization());
+  return resource_provider()->MapDirectRasterBuffer(task->resource()->id());
+}
+
+void RasterWorkerPool::OnRasterCompleted(
+    internal::RasterWorkerPoolTask* task,
+    const PicturePileImpl::Analysis& analysis) {
+  DCHECK(task->use_gpu_rasterization());
+  resource_provider()->UnmapDirectRasterBuffer(task->resource()->id());
+}

[reveman, 2014/02/11 02:00:44]

I think RasterWorkerPool should be an interface and not an implementation. It's
sort of half right now and that's proven confusing and sometimes hard to work
with. This is my fault but I'm hoping we can correct it.

I'd like to get RasterWorkerPool to a place where it's just the interface and a
set of static helper functions and helper classes. All members need to be
implemented by Image or PixelBuffer instances.

In this case, I think you can just remove these member functions and move the
code to Image/PixelBufferRWP and not add any helper functions.

[alokp, 2014/02/11 07:05:21]

Done.

+
 void RasterWorkerPool::SetClient(RasterWorkerPoolClient* client) {
   client_ = client;
 }