From f55c314bfa6db980941f8d0ab485beb2e08589ab Mon Sep 17 00:00:00 2001
From: mtklein <mtklein@chromium.org>
Date: Thu, 11 Dec 2014 12:43:04 -0800
Subject: [PATCH] Enforce thread-safety of bitmaps in pictures via the type.

No runtime difference here, but it makes it impossible to forget to make
a shallow copy; you can't get at the full bitmap without it.

NOTREECHECKS=true

BUG=skia:

Review URL: https://codereview.chromium.org/799603002
---
 src/core/SkRecordDraw.cpp | 15 +++++----------
 src/core/SkRecords.h      | 17 +++++++++++++----
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index 7f575d1..b714e75 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -74,11 +74,6 @@ void SkRecordPartialDraw(const SkRecord& record, SkCanvas* canvas,
 
 namespace SkRecords {
 
-// FIXME: SkBitmaps are stateful, so we need to copy them to play back in multiple threads.
-static SkBitmap shallow_copy(const SkBitmap& bitmap) {
-    return bitmap;
-}
-
 // NoOps draw nothing.
 template <> void Draw::draw(const NoOp&) {}
 
@@ -100,13 +95,13 @@ DRAW(BeginCommentGroup, beginCommentGroup(r.description));
 DRAW(AddComment, addComment(r.key, r.value));
 DRAW(EndCommentGroup, endCommentGroup());
 
-DRAW(DrawBitmap, drawBitmap(shallow_copy(r.bitmap), r.left, r.top, r.paint));
-DRAW(DrawBitmapNine, drawBitmapNine(shallow_copy(r.bitmap), r.center, r.dst, r.paint));
+DRAW(DrawBitmap, drawBitmap(r.bitmap.shallowCopy(), r.left, r.top, r.paint));
+DRAW(DrawBitmapNine, drawBitmapNine(r.bitmap.shallowCopy(), r.center, r.dst, r.paint));
 DRAW(DrawBitmapRectToRect,
-        drawBitmapRectToRect(shallow_copy(r.bitmap), r.src, r.dst, r.paint,
+        drawBitmapRectToRect(r.bitmap.shallowCopy(), r.src, r.dst, r.paint,
                              SkCanvas::kNone_DrawBitmapRectFlag));
 DRAW(DrawBitmapRectToRectBleed,
-        drawBitmapRectToRect(shallow_copy(r.bitmap), r.src, r.dst, r.paint,
+        drawBitmapRectToRect(r.bitmap.shallowCopy(), r.src, r.dst, r.paint,
                              SkCanvas::kBleed_DrawBitmapRectFlag));
 DRAW(DrawDRRect, drawDRRect(r.outer, r.inner, r.paint));
 DRAW(DrawImage, drawImage(r.image, r.left, r.top, r.paint));
@@ -121,7 +116,7 @@ DRAW(DrawPosText, drawPosText(r.text, r.byteLength, r.pos, r.paint));
 DRAW(DrawPosTextH, drawPosTextH(r.text, r.byteLength, r.xpos, r.y, r.paint));
 DRAW(DrawRRect, drawRRect(r.rrect, r.paint));
 DRAW(DrawRect, drawRect(r.rect, r.paint));
-DRAW(DrawSprite, drawSprite(shallow_copy(r.bitmap), r.left, r.top, r.paint));
+DRAW(DrawSprite, drawSprite(r.bitmap.shallowCopy(), r.left, r.top, r.paint));
 DRAW(DrawText, drawText(r.text, r.byteLength, r.x, r.y, r.paint));
 DRAW(DrawTextBlob, drawTextBlob(r.blob, r.x, r.y, r.paint));
 DRAW(DrawTextOnPath, drawTextOnPath(r.text, r.byteLength, r.path, &r.matrix, r.paint));
diff --git a/src/core/SkRecords.h b/src/core/SkRecords.h
index d6fc723..d8dfb2b 100644
--- a/src/core/SkRecords.h
+++ b/src/core/SkRecords.h
@@ -183,15 +183,24 @@ private:
 
 // Like SkBitmap, but deep copies pixels if they're not immutable.
 // Using this, we guarantee the immutability of all bitmaps we record.
-struct ImmutableBitmap : public SkBitmap {
+class ImmutableBitmap : SkNoncopyable {
+public:
     explicit ImmutableBitmap(const SkBitmap& bitmap) {
         if (bitmap.isImmutable()) {
-            *(SkBitmap*)this = bitmap;
+            fBitmap = bitmap;
         } else {
-            bitmap.copyTo(this);
+            bitmap.copyTo(&fBitmap);
         }
-        this->setImmutable();
+        fBitmap.setImmutable();
     }
+
+    int width()  const { return fBitmap.width();  }
+    int height() const { return fBitmap.height(); }
+
+    // While the pixels are immutable, SkBitmap itself is not thread-safe, so return a copy.
+    SkBitmap shallowCopy() const { return fBitmap; }
+private:
+    SkBitmap fBitmap;
 };
 
 // SkPath::getBounds() isn't thread safe unless we precache the bounds in a singlethreaded context.
-- 
2.7.4