Remove redundant memory clears when constructing BitmapPlatformDevice instances on Mac and Windows. PlatformCanvas construction is showing up as a performance bottleneck due to unnecessary initialization.

The change moves the clear to the call sites where it is necessary.

Note:  On Linux, cairo always allocates an initialized surface, so there is no way to bypass the performance penalty.

BUG=112009
TEST=All of them

Review URL: http://codereview.chromium.org/9416017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@127196 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 121 insertions, 78 deletions

diff --git a/skia/ext/bitmap_platform_device_android.cc b/skia/ext/bitmap_platform_device_android.cc
index 9749e9e..f49b6a8 100644
--- a/skia/ext/bitmap_platform_device_android.cc
+++ b/skia/ext/bitmap_platform_device_android.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -21,21 +21,25 @@ BitmapPlatformDevice* BitmapPlatformDevice::Create(int width, int height,
   return NULL;
 }
 
+BitmapPlatformDevice* BitmapPlatformDevice::CreateAndClear(int width,
+                                                           int height,
+                                                           bool is_opaque) {
+  BitmapPlatformDevice* device = Create(width, height, is_opaque);
+  if (!is_opaque)
+    device->accessBitmap(true).eraseARGB(0, 0, 0, 0);
+  return device;
+}
+
 BitmapPlatformDevice* BitmapPlatformDevice::Create(int width, int height,
                                                    bool is_opaque,
                                                    uint8_t* data) {
   SkBitmap bitmap;
   bitmap.setConfig(SkBitmap::kARGB_8888_Config, width, height);
-  if (data) {
+  if (data)
     bitmap.setPixels(data);
-  } else {
-    if (!bitmap.allocPixels())
-      return NULL;
-    // Follow the logic in SkCanvas::createDevice(), initialize the bitmap if it
-    // is not opaque.
-    if (!is_opaque)
-      bitmap.eraseARGB(0, 0, 0, 0);
-  }
+  else if (!bitmap.allocPixels())
+    return NULL;
+
   bitmap.setIsOpaque(is_opaque);
   return new BitmapPlatformDevice(bitmap);
 }
diff --git a/skia/ext/bitmap_platform_device_android.h b/skia/ext/bitmap_platform_device_android.h
index a68c9e6..50272eb 100644
--- a/skia/ext/bitmap_platform_device_android.h
+++ b/skia/ext/bitmap_platform_device_android.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -20,9 +20,18 @@ namespace skia {
 // -----------------------------------------------------------------------------
 class BitmapPlatformDevice : public PlatformDevice, public SkDevice {
  public:
+  // Construct a BitmapPlatformDevice. |is_opaque| should be set if the caller
+  // knows the bitmap will be completely opaque and allows some optimizations.
+  // The bitmap is not initialized.
   static BitmapPlatformDevice* Create(int width, int height, bool is_opaque);
 
-  // This doesn't take ownership of |data|
+  // Construct a BitmapPlatformDevice, as above.
+  // If |is_opaque| is false, the bitmap is initialized to 0.
+  static BitmapPlatformDevice* CreateAndClear(int width, int height,
+                                              bool is_opaque);
+
+  // This doesn't take ownership of |data|. If |data| is null, the bitmap
+  // is not initialized to 0.
   static BitmapPlatformDevice* Create(int width, int height, bool is_opaque,
                                       uint8_t* data);
 
diff --git a/skia/ext/bitmap_platform_device_linux.cc b/skia/ext/bitmap_platform_device_linux.cc
index 4145b26..b284469 100644
--- a/skia/ext/bitmap_platform_device_linux.cc
+++ b/skia/ext/bitmap_platform_device_linux.cc
@@ -110,6 +110,14 @@ BitmapPlatformDevice* BitmapPlatformDevice::Create(int width, int height,
   return device;
 }
 
+BitmapPlatformDevice* BitmapPlatformDevice::CreateAndClear(int width,
+                                                           int height,
+                                                           bool is_opaque) {
+  // The Linux port always constructs initialized bitmaps, so there is no extra
+  // work to perform here.
+  return Create(width, height, is_opaque);
+}
+
 BitmapPlatformDevice* BitmapPlatformDevice::Create(int width, int height,
                                                    bool is_opaque,
                                                    uint8_t* data) {
diff --git a/skia/ext/bitmap_platform_device_linux.h b/skia/ext/bitmap_platform_device_linux.h
index 80f4793..02132f0 100644
--- a/skia/ext/bitmap_platform_device_linux.h
+++ b/skia/ext/bitmap_platform_device_linux.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -71,9 +71,19 @@ class BitmapPlatformDevice : public PlatformDevice, public SkDevice {
   BitmapPlatformDevice(const SkBitmap& other, BitmapPlatformDeviceData* data);
   virtual ~BitmapPlatformDevice();
 
+  // Constructs a device with size |width| * |height| with contents initialized
+  // to zero. |is_opaque| should be set if the caller knows the bitmap will be
+  // completely opaque and allows some optimizations.
   static BitmapPlatformDevice* Create(int width, int height, bool is_opaque);
 
-  // This doesn't take ownership of |data|
+  // Performs the same construction as Create.
+  // Other ports require a separate construction routine because Create does not
+  // initialize the bitmap to 0.
+  static BitmapPlatformDevice* CreateAndClear(int width, int height,
+                                              bool is_opaque);
+
+  // This doesn't take ownership of |data|. If |data| is NULL, the contents
+  // of the device are initialized to 0.
   static BitmapPlatformDevice* Create(int width, int height, bool is_opaque,
                                       uint8_t* data);
 
diff --git a/skia/ext/bitmap_platform_device_mac.cc b/skia/ext/bitmap_platform_device_mac.cc
index 3c87fad..826719b 100644
--- a/skia/ext/bitmap_platform_device_mac.cc
+++ b/skia/ext/bitmap_platform_device_mac.cc
@@ -125,11 +125,6 @@ BitmapPlatformDevice* BitmapPlatformDevice::Create(CGContextRef context,
     bitmap.setPixels(data);
   } else {
     data = bitmap.getPixels();
-
-    // Note: The Windows implementation clears the Bitmap later on.
-    // This bears mentioning since removal of this line makes the
-    // unit tests only fail periodically (or when MallocPreScribble is set).
-    bitmap.eraseARGB(0, 0, 0, 0);
   }
 
   bitmap.setIsOpaque(is_opaque);
@@ -160,6 +155,15 @@ BitmapPlatformDevice* BitmapPlatformDevice::Create(CGContextRef context,
   return rv;
 }
 
+BitmapPlatformDevice* BitmapPlatformDevice::CreateAndClear(int width,
+                                                           int height,
+                                                           bool is_opaque) {
+  BitmapPlatformDevice* device = Create(NULL, width, height, is_opaque);
+  if (!is_opaque)
+    device->accessBitmap(true).eraseARGB(0, 0, 0, 0);
+  return device;
+}
+
 BitmapPlatformDevice* BitmapPlatformDevice::CreateWithData(uint8_t* data,
                                                            int width,
                                                            int height,
@@ -242,7 +246,9 @@ SkDevice* BitmapPlatformDevice::onCreateCompatibleDevice(
     SkBitmap::Config config, int width, int height, bool isOpaque,
     Usage /*usage*/) {
   SkASSERT(config == SkBitmap::kARGB_8888_Config);
-  return BitmapPlatformDevice::Create(NULL, width, height, isOpaque);
+  SkDevice* bitmap_device = BitmapPlatformDevice::CreateAndClear(width, height,
+                                                                 isOpaque);
+  return bitmap_device;
 }
 
 }  // namespace skia
diff --git a/skia/ext/bitmap_platform_device_mac.h b/skia/ext/bitmap_platform_device_mac.h
index a9634ad..97d3582 100644
--- a/skia/ext/bitmap_platform_device_mac.h
+++ b/skia/ext/bitmap_platform_device_mac.h
@@ -28,12 +28,22 @@ namespace skia {
 // DEVICE'S PIXEL DATA TO ANOTHER BITMAP, make sure you copy instead.
 class SK_API BitmapPlatformDevice : public PlatformDevice, public SkDevice {
  public:
-  // |context| may be NULL.
+  // Creates a BitmapPlatformDevice instance. |is_opaque| should be set if the
+  // caller knows the bitmap will be completely opaque and allows some
+  // optimizations.
+  // |context| may be NULL. If |context| is NULL, then the bitmap backing store
+  // is not initialized.
   static BitmapPlatformDevice* Create(CGContextRef context,
                                       int width, int height,
                                       bool is_opaque);
 
+  // Creates a BitmapPlatformDevice instance.  If |is_opaque| is false,
+  // then the bitmap is initialzed to 0.
+  static BitmapPlatformDevice* CreateAndClear(int width, int height,
+                                              bool is_opaque);
+
   // Creates a context for |data| and calls Create.
+  // If |data| is NULL, then the bitmap backing store is not initialized.
   static BitmapPlatformDevice* CreateWithData(uint8_t* data,
                                               int width, int height,
                                               bool is_opaque);
diff --git a/skia/ext/bitmap_platform_device_win.cc b/skia/ext/bitmap_platform_device_win.cc
index bcfb366..90fdf30 100644
--- a/skia/ext/bitmap_platform_device_win.cc
+++ b/skia/ext/bitmap_platform_device_win.cc
@@ -87,7 +87,7 @@ void BitmapPlatformDevice::BitmapPlatformDeviceData::LoadConfig() {
 // that we can create the pixel data before calling the constructor. This is
 // required so that we can call the base class' constructor with the pixel
 // data.
-BitmapPlatformDevice* BitmapPlatformDevice::create(
+BitmapPlatformDevice* BitmapPlatformDevice::Create(
     HDC screen_dc,
     int width,
     int height,
@@ -128,18 +128,13 @@ BitmapPlatformDevice* BitmapPlatformDevice::create(
   bitmap.setPixels(data);
   bitmap.setIsOpaque(is_opaque);
 
-  // If we were given data, then don't clobber it!
-  if (!shared_section) {
-    if (is_opaque) {
 #ifndef NDEBUG
-      // To aid in finding bugs, we set the background color to something
-      // obviously wrong so it will be noticable when it is not cleared
-      bitmap.eraseARGB(255, 0, 255, 128);  // bright bluish green
+  // If we were given data, then don't clobber it!
+  if (!shared_section && is_opaque)
+    // To aid in finding bugs, we set the background color to something
+    // obviously wrong so it will be noticable when it is not cleared
+    bitmap.eraseARGB(255, 0, 255, 128);  // bright bluish green
 #endif
-    } else {
-      bitmap.eraseARGB(0, 0, 0, 0);
-    }
-  }
 
   // The device object will take ownership of the HBITMAP. The initial refcount
   // of the data object will be 1, which is what the constructor expects.
@@ -148,17 +143,34 @@ BitmapPlatformDevice* BitmapPlatformDevice::create(
 }
 
 // static
-BitmapPlatformDevice* BitmapPlatformDevice::create(int width,
+BitmapPlatformDevice* BitmapPlatformDevice::Create(int width,
                                                    int height,
                                                    bool is_opaque,
                                                    HANDLE shared_section) {
   HDC screen_dc = GetDC(NULL);
-  BitmapPlatformDevice* device = BitmapPlatformDevice::create(
+  BitmapPlatformDevice* device = BitmapPlatformDevice::Create(
       screen_dc, width, height, is_opaque, shared_section);
   ReleaseDC(NULL, screen_dc);
   return device;
 }
 
+// static
+BitmapPlatformDevice* BitmapPlatformDevice::Create(int width, int height,
+                                                   bool is_opaque) {
+  return Create(width, height, is_opaque, NULL);
+}
+
+// static
+BitmapPlatformDevice* BitmapPlatformDevice::CreateAndClear(int width,
+                                                           int height,
+                                                           bool is_opaque) {
+  BitmapPlatformDevice* device = BitmapPlatformDevice::Create(width, height,
+                                                              is_opaque);
+  if (!is_opaque)
+    device->accessBitmap(true).eraseARGB(0, 0, 0, 0);
+  return device;
+}
+
 // The device will own the HBITMAP, which corresponds to also owning the pixel
 // data. Therefore, we do not transfer ownership to the SkDevice's bitmap.
 BitmapPlatformDevice::BitmapPlatformDevice(
@@ -259,7 +271,9 @@ SkDevice* BitmapPlatformDevice::onCreateCompatibleDevice(
     SkBitmap::Config config, int width, int height, bool isOpaque,
     Usage /*usage*/) {
   SkASSERT(config == SkBitmap::kARGB_8888_Config);
-  return BitmapPlatformDevice::create(width, height, isOpaque, NULL);
+  SkDevice* bitmap_device = BitmapPlatformDevice::CreateAndClear(width, height,
+                                                                 isOpaque);
+  return bitmap_device;
 }
 
 }  // namespace skia
diff --git a/skia/ext/bitmap_platform_device_win.h b/skia/ext/bitmap_platform_device_win.h
index 57f94b6..5c416a1 100644
--- a/skia/ext/bitmap_platform_device_win.h
+++ b/skia/ext/bitmap_platform_device_win.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -32,16 +32,27 @@ class SK_API BitmapPlatformDevice : public PlatformDevice, public SkDevice {
   // be stored beyond this function. is_opaque should be set if the caller
   // knows the bitmap will be completely opaque and allows some optimizations.
   //
-  // The shared_section parameter is optional (pass NULL for default behavior).
-  // If shared_section is non-null, then it must be a handle to a file-mapping
-  // object returned by CreateFileMapping.  See CreateDIBSection for details.
-  static BitmapPlatformDevice* create(HDC screen_dc, int width, int height,
+  // The |shared_section| parameter is optional (pass NULL for default
+  // behavior). If |shared_section| is non-null, then it must be a handle to a
+  // file-mapping object returned by CreateFileMapping.  See CreateDIBSection
+  // for details. If |shared_section| is null, the bitmap backing store is not
+  // initialized.
+  static BitmapPlatformDevice* Create(HDC screen_dc, int width, int height,
                                       bool is_opaque, HANDLE shared_section);
 
   // This version is the same as above but will get the screen DC itself.
-  static BitmapPlatformDevice* create(int width, int height, bool is_opaque,
+  static BitmapPlatformDevice* Create(int width, int height, bool is_opaque,
                                       HANDLE shared_section);
 
+  // Create a BitmapPlatformDevice with no shared section. The bitmap is not
+  // initialized to 0.
+  static BitmapPlatformDevice* Create(int width, int height, bool is_opaque);
+
+  // Creates a BitmapPlatformDevice instance respecting the parameters as above.
+  // If |is_opaque| is false, then the bitmap is initialzed to 0.
+  static BitmapPlatformDevice* CreateAndClear(int width, int height,
+                                              bool is_opaque);
+
   virtual ~BitmapPlatformDevice();
 
   // PlatformDevice overrides
diff --git a/skia/ext/data/vectorcanvastest/uninitialized/00_pc_empty.png b/skia/ext/data/vectorcanvastest/uninitialized/00_pc_empty.png
deleted file mode 100644
index 9cbff6e..0000000
--- a/skia/ext/data/vectorcanvastest/uninitialized/00_pc_empty.png
+++ /dev/null
diff --git a/skia/ext/data/vectorcanvastest/uninitialized/00_vc_empty.png b/skia/ext/data/vectorcanvastest/uninitialized/00_vc_empty.png
deleted file mode 100644
index 9cbff6e..0000000
--- a/skia/ext/data/vectorcanvastest/uninitialized/00_vc_empty.png
+++ /dev/null
diff --git a/skia/ext/platform_canvas_unittest.cc b/skia/ext/platform_canvas_unittest.cc
index 40cdc70..125eb20 100644
--- a/skia/ext/platform_canvas_unittest.cc
+++ b/skia/ext/platform_canvas_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -152,6 +152,7 @@ class LayerSaver {
     bounds.set(SkIntToScalar(x_), SkIntToScalar(y_),
                SkIntToScalar(right()), SkIntToScalar(bottom()));
     canvas_.saveLayer(&bounds, NULL);
+    canvas.clear(SkColorSetARGB(0, 0, 0, 0));
   }
 
   ~LayerSaver() {
diff --git a/skia/ext/platform_canvas_win.cc b/skia/ext/platform_canvas_win.cc
index 371de78..a1fe9a4 100644
--- a/skia/ext/platform_canvas_win.cc
+++ b/skia/ext/platform_canvas_win.cc
@@ -95,7 +95,7 @@ bool PlatformCanvas::initialize(int width,
                                int height,
                                bool is_opaque,
                                HANDLE shared_section) {
-  if (initializeWithDevice(BitmapPlatformDevice::create(width,
+  if (initializeWithDevice(BitmapPlatformDevice::Create(width,
                                                         height,
                                                         is_opaque,
                                                         shared_section)))
diff --git a/skia/ext/vector_canvas_unittest.cc b/skia/ext/vector_canvas_unittest.cc
index e4c7486..23661fe 100644
--- a/skia/ext/vector_canvas_unittest.cc
+++ b/skia/ext/vector_canvas_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -448,26 +448,6 @@ class VectorCanvasTest : public ImageTest {
 ////////////////////////////////////////////////////////////////////////////////
 // Actual tests
 
-TEST_F(VectorCanvasTest, Uninitialized) {
-  // Do a little mubadumba do get uninitialized stuff.
-  VectorCanvasTest::TearDown();
-
-  // The goal is not to verify that have the same uninitialized data.
-  compare_canvas_ = false;
-
-  context_ = new Context();
-  bitmap_ = new Bitmap(*context_, size_, size_);
-  vcanvas_ = new VectorCanvas(VectorPlatformDeviceEmf::CreateDevice(
-      size_, size_, true, context_->context()));
-  pcanvas_ = new PlatformCanvas(size_, size_, false);
-
-  // VectorCanvas default initialization is black.
-  // PlatformCanvas default initialization is almost white 0x01FFFEFD (invalid
-  // Skia color) in both Debug and Release. See magicTransparencyColor in
-  // platform_device.cc
-  EXPECT_EQ(0., ProcessImage(FILE_PATH_LITERAL("empty")));
-}
-
 TEST_F(VectorCanvasTest, BasicDrawing) {
   EXPECT_EQ(Image(*vcanvas_).PercentageDifferent(Image(*pcanvas_)), 0.)
       << L"clean";
diff --git a/skia/ext/vector_platform_device_emf_win.cc b/skia/ext/vector_platform_device_emf_win.cc
index 99f7dc8..55c84fb 100644
--- a/skia/ext/vector_platform_device_emf_win.cc
+++ b/skia/ext/vector_platform_device_emf_win.cc
@@ -26,7 +26,7 @@ SkDevice* VectorPlatformDeviceEmf::CreateDevice(
     // EMF-based VectorDevice and have this device registers the drawing. When
     // playing back the device into a bitmap, do it at the printer's dpi instead
     // of the layout's dpi (which is much lower).
-    return BitmapPlatformDevice::create(width, height, is_opaque,
+    return BitmapPlatformDevice::Create(width, height, is_opaque,
                                         shared_section);
   }
 
diff --git a/skia/ext/vector_platform_device_skia.cc b/skia/ext/vector_platform_device_skia.cc
index 7ee8f769..aab2554 100644
--- a/skia/ext/vector_platform_device_skia.cc
+++ b/skia/ext/vector_platform_device_skia.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 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.
 
@@ -41,19 +41,9 @@ PlatformSurface VectorPlatformDeviceSkia::BeginPlatformPaint() {
   // and return the context from it, then layer on the raster data as an
   // image in EndPlatformPaint.
   DCHECK(raster_surface_ == NULL);
-#if defined(OS_WIN)
-  raster_surface_ = BitmapPlatformDevice::create(width(),
-                                                 height(),
-                                                 false, /* not opaque */
-                                                 NULL);
-#elif defined(OS_POSIX) && !defined(OS_MACOSX)
-  raster_surface_ = BitmapPlatformDevice::Create(width(),
-                                                 height(),
-                                                 false /* not opaque */);
-#endif
+  raster_surface_ = BitmapPlatformDevice::CreateAndClear(width(), height(),
+                                                         false);
   raster_surface_->unref();  // SkRefPtr and create both took a reference.
-
-  SkCanvas canvas(raster_surface_.get());
   return raster_surface_->BeginPlatformPaint();
 }