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

cc/resources/resource_provider.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CC_RESOURCES_RESOURCE_PROVIDER_H_
 #define CC_RESOURCES_RESOURCE_PROVIDER_H_
 
 #include <deque>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/threading/thread_checker.h"
 #include "cc/base/cc_export.h"
 #include "cc/output/context_provider.h"
 #include "cc/output/output_surface.h"
+#include "cc/resources/raster_buffer.h"
 #include "cc/resources/release_callback.h"
 #include "cc/resources/resource_format.h"
 #include "cc/resources/return_callback.h"
 #include "cc/resources/shared_bitmap.h"
 #include "cc/resources/single_release_callback.h"
 #include "cc/resources/texture_mailbox.h"
 #include "cc/resources/transferable_resource.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ skipping 273 matching lines @@
     virtual bool HasPassed() = 0;
 
    protected:
     friend class base::RefCounted<Fence>;
     virtual ~Fence() {}
 
    private:
     DISALLOW_COPY_AND_ASSIGN(Fence);
   };
 
-  // Returns a canvas for gpu rasterization.
-  // Call Unmap before the resource can be read or used for compositing.
+  // Returns a RasterBuffer for gpu rasterization.
+  // Call Release before the resource can be read or used for compositing.
   // It is used for direct gpu rasterization.
-  SkCanvas* MapGpuRasterBuffer(ResourceId id);
-  void UnmapGpuRasterBuffer(ResourceId id);
+  RasterBuffer* AcquireGpuRasterBuffer(ResourceId id);
+  void ReleaseGpuRasterBuffer(ResourceId id);
 
-  // Returns a canvas backed by an image buffer. UnmapImageRasterBuffer
-  // returns true if canvas was written to while mapped.
-  // Rasterizing to the canvas writes the content into the image buffer,
+  // Returns a RasterBuffer backed by an image buffer. ReleaseImageRasterBuffer
+  // returns true if RasterBuffer was written to while acquired.
+  // Rasterizing to the RasterBuffer writes the content into the image buffer,
   // which is internally bound to the underlying resource when read.
-  // Call Unmap before the resource can be read or used for compositing.
+  // Call Release before the resource can be read or used for compositing.
   // It is used by ImageRasterWorkerPool.
-  SkCanvas* MapImageRasterBuffer(ResourceId id);
-  bool UnmapImageRasterBuffer(ResourceId id);
+  RasterBuffer* AcquireImageRasterBuffer(ResourceId id);
+  bool ReleaseImageRasterBuffer(ResourceId id);
 
-  // Returns a canvas backed by pixel buffer. UnmapPixelRasterBuffer
-  // returns true if canvas was written to while mapped.
+  // Returns a RasterBuffer backed by pixel buffer. UnmapPixelRasterBuffer
+  // returns true if RasterBuffer was written to while acquired.
   // The pixel buffer needs to be uploaded to the underlying resource
   // using BeginSetPixels before the resouce can be used for compositing.
   // It is used by PixelRasterWorkerPool.
   void AcquirePixelRasterBuffer(ResourceId id);
   void ReleasePixelRasterBuffer(ResourceId id);
-  SkCanvas* MapPixelRasterBuffer(ResourceId id);
+  RasterBuffer* MapPixelRasterBuffer(ResourceId id);
   bool UnmapPixelRasterBuffer(ResourceId id);

[reveman, 2014/08/12 14:31:26]

Can we refactor this to be consistent with
{Acquire|Release}{Gpu|Image}RasterBuffer? I'm thinking that Map/Unmap can be
implemented as RasterBuffer::Acquire/ReleaseSkCanvas instead.

[auygun, 2014/08/12 15:24:01]

I don't think Map/UnmapPixelBuffer can be called from a worker thread. It didn't
work for me at least. Or did I do something wrong?

[reveman, 2014/08/12 15:54:28]

Right. That doesn't work at the moment but we like to change that in the future.
At least for ImageRasterBuffer.

How about having AcquirePixelRasterBuffer map the buffer for now and
BeginSetPixels/ReleasePixelRasterBuffer implicitly unmap it? That would match
how we want this to work in the future.

[auygun, 2014/08/13 14:18:53]

This involves some more change. PixelBufferRasterWorkerPool needs to know if the
content was changed before calling BeginSetPixel. I'll see what I can do.

 
   // Asynchronously update pixels from acquired pixel buffer.
   void BeginSetPixels(ResourceId id);
   void ForceSetPixelsToComplete(ResourceId id);
   bool DidSetPixelsComplete(ResourceId id);
 
   // For tests only! This prevents detecting uninitialized reads.
   // Use SetPixels or LockForWrite to allocate implicitly.
   void AllocateForTesting(ResourceId id);
 
@@ skipping 88 matching lines @@
     ResourceType type;
     ResourceFormat format;
     SharedBitmapId shared_bitmap_id;
     SharedBitmap* shared_bitmap;
     linked_ptr<GpuRasterBuffer> gpu_raster_buffer;
     linked_ptr<ImageRasterBuffer> image_raster_buffer;
     linked_ptr<PixelRasterBuffer> pixel_raster_buffer;
   };
   typedef base::hash_map<ResourceId, Resource> ResourceMap;
 
-  class RasterBuffer {
-   public:
-    virtual ~RasterBuffer();
-
-    SkCanvas* LockForWrite();
-    // Returns true if canvas was written to while locked.
-    bool UnlockForWrite();
-
-   protected:
-    RasterBuffer(const Resource* resource, ResourceProvider* resource_provider);
-    const Resource* resource() const { return resource_; }
-    ResourceProvider* resource_provider() const { return resource_provider_; }
-
-    virtual SkCanvas* DoLockForWrite() = 0;
-    virtual bool DoUnlockForWrite() = 0;
-
-   private:
-    const Resource* resource_;
-    ResourceProvider* resource_provider_;
-    SkCanvas* locked_canvas_;
-    int canvas_save_count_;
-  };
-
   class GpuRasterBuffer : public RasterBuffer {
    public:
     GpuRasterBuffer(const Resource* resource,
                     ResourceProvider* resource_provider,
                     bool use_distance_field_text);
     virtual ~GpuRasterBuffer();
 
+    void LockForWrite();
+    // Returns true if canvas was written to while locked.
+    bool UnlockForWrite();

[reveman, 2014/08/12 14:31:27]

Do we need these functions or can we just handle this as part of
Acquire/ReleaseSkCanvas instead?

[auygun, 2014/08/12 15:24:01]

Can GpuRasterBuffer::CreateSurface() be called from a worker thread?

[reveman, 2014/08/12 15:54:28]

Good point. I'm not sure that will work so let's not change that as part of this
patch. Can we create the surface as the buffer is created?

[auygun, 2014/08/13 14:18:53]

We can move GpuRasterBuffer::CreateSurface() to ResourceProvider and create the
surface and pass it to the buffer as it's created. Is that what you mean?

+
    protected:
-    virtual SkCanvas* DoLockForWrite() OVERRIDE;
-    virtual bool DoUnlockForWrite() OVERRIDE;
+    const Resource* resource() const { return resource_; }
+    ResourceProvider* resource_provider() const { return resource_provider_; }

[reveman, 2014/08/12 14:31:27]

do we really need these functions?

[auygun, 2014/08/13 14:18:53]

Acknowledged.

+
     skia::RefPtr<SkSurface> CreateSurface();
 
+    virtual SkCanvas* AcquireSkCanvas() OVERRIDE;
+    virtual void ReleaseSkCanvas() OVERRIDE;
+
    private:
+    const Resource* resource_;
+    ResourceProvider* resource_provider_;
+    SkCanvas* locked_canvas_;

[reveman, 2014/08/12 14:31:26]

If the canvas is passed to ReleaseSkCanvas? Can we just remove this a create a
new canvas each time?

[auygun, 2014/08/13 14:18:54]

Acknowledged.

+    int canvas_save_count_;

[reveman, 2014/08/12 14:31:26]

I'd like to remove this. If we create a new SkCanvas each time then it's not
necessary. If we need to cache the canvas then I think it should be a
requirement of the RasterBuffer client to have a balanced number of save/restore
calls.

[auygun, 2014/08/13 14:18:53]

Acknowledged.

     skia::RefPtr<SkSurface> surface_;
     uint32_t surface_generation_id_;
     const bool use_distance_field_text_;
 
     DISALLOW_COPY_AND_ASSIGN(GpuRasterBuffer);
   };
 
   class BitmapRasterBuffer : public RasterBuffer {
    public:
     virtual ~BitmapRasterBuffer();
 
+    void LockForWrite();
+    // Returns true if canvas was written to while locked.
+    bool UnlockForWrite();

[reveman, 2014/08/12 14:31:27]

I prefer if we just handle this as part of
Acquire/ReleaseSkCanvas instead. It's really nasty to have a derived class call
a base class function and that function is then calling a virtual function
implemented by the derived class.

I would much prefer if the final derived class implemented
Acquire/ReleaseSkCanvas and we add whatever utility functions we need to share
code between implementations but no other virtual functions.

[auygun, 2014/08/12 15:24:01]

I think Lock/UnlockForWrite need to be called from compositor thread. Or am I
wrong?
We can make them virtual and implement in the final derived class and remove
Map/UnmapBuffer virtuals.

[reveman, 2014/08/12 15:54:28]

I prefer if we remove the BitmapRasterBuffer class and instead add some explicit
functions to the ImageRasterBuffer and PixelRasterBuffer classes if needed. The
current inheritance is not needed for code reuse and is just confusing imo. 

[auygun, 2014/08/13 14:18:53]

Acknowledged.

+
    protected:
     BitmapRasterBuffer(const Resource* resource,
                        ResourceProvider* resource_provider);
 
-    virtual SkCanvas* DoLockForWrite() OVERRIDE;
-    virtual bool DoUnlockForWrite() OVERRIDE;
+    const Resource* resource() const { return resource_; }
+    ResourceProvider* resource_provider() const { return resource_provider_; }
+
+    virtual SkCanvas* AcquireSkCanvas() OVERRIDE;
+    virtual void ReleaseSkCanvas() OVERRIDE;
 
     virtual uint8_t* MapBuffer(int* stride) = 0;
     virtual void UnmapBuffer() = 0;
 
    private:
+    const Resource* resource_;
+    ResourceProvider* resource_provider_;
+    int canvas_save_count_;
     uint8_t* mapped_buffer_;
     SkBitmap raster_bitmap_;
     uint32_t raster_bitmap_generation_id_;
+    bool raster_bitmap_changed_;
+    int stride_;
     skia::RefPtr<SkCanvas> raster_canvas_;
   };
 
   class ImageRasterBuffer : public BitmapRasterBuffer {
    public:
     ImageRasterBuffer(const Resource* resource,
                       ResourceProvider* resource_provider);
     virtual ~ImageRasterBuffer();
 
    protected:
@@ skipping 177 matching lines @@
   return format_gl_data_format[format];
 }
 
 inline GLenum GLInternalFormat(ResourceFormat format) {
   return GLDataFormat(format);
 }
 
 }  // namespace cc
 
 #endif  // CC_RESOURCES_RESOURCE_PROVIDER_H_