Hack SkRecord in as an alternate recording mode in cc/resources/picture.

cc/resources/picture.cc

Index: cc/resources/picture.cc
diff --git a/cc/resources/picture.cc b/cc/resources/picture.cc
index 93b59aea7435532456ab03b9248113b8e5c8d46c..4bb3bca1a26cbe5e2f50c965337801dca1d790ec 100644
--- a/cc/resources/picture.cc
+++ b/cc/resources/picture.cc
@@ -9,6 +9,7 @@
 #include <set>
 
 #include "base/base64.h"
+#include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/values.h"
 #include "cc/base/math_util.h"
@@ -191,6 +192,11 @@ Picture::~Picture() {
 }
 
 Picture* Picture::GetCloneForDrawingOnThread(unsigned thread_index) {
+  // We don't need clones to draw from multiple threads with SkRecord.
+  if (playback_.get() != NULL) {
+    return this;
+  }
+
   // SkPicture is not thread-safe to rasterize with, this returns a clone
   // to rasterize with on a specific thread.
   CHECK_GE(clones_.size(), thread_index);
@@ -213,6 +219,11 @@ bool Picture::IsSuitableForGpuRasterization() const {
 void Picture::CloneForDrawing(int num_threads) {
   TRACE_EVENT1("cc", "Picture::CloneForDrawing", "num_threads", num_threads);
 
+  // We don't need clones to draw from multiple threads with SkRecord.
+  if (playback_.get() != NULL) {
+    return;
+  }
+
   DCHECK(picture_);
   DCHECK(clones_.empty());
 
@@ -240,6 +251,13 @@ void Picture::CloneForDrawing(int num_threads) {
 void Picture::Record(ContentLayerClient* painter,
                      const SkTileGridPicture::TileGridInfo& tile_grid_info,
                      RecordingMode recording_mode) {
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  // HACK HACK HACK HACK: checkdeps won't let me look at this flag here.  :(

[enne (OOO), 2014/04/17 17:09:49]

If you want to make this patch more real, I'd suggest adding a flag on
LayerTreeSettings.  Check the command line and set the LayerTreeSettings flag
outside of cc, and then pass along the LayerTreeSettings flag to the
PicturePile(Base) constructor which can set it on Picture in turn.

[mtklein, 2014/04/17 17:32:04]

Ah, thanks, figuring this out was actually my hangup here.  I'll try this.

+  if (command_line.HasSwitch("enable-bleeding-edge-rendering-fast-paths")) {
+    recording_mode = RECORD_WITH_SKRECORD;
+  }
+  recording_mode = RECORD_WITH_SKRECORD;
+
   TRACE_EVENT2("cc",
                "Picture::Record",
                "data",
@@ -252,22 +270,33 @@ void Picture::Record(ContentLayerClient* painter,
   picture_ = skia::AdoptRef(new SkTileGridPicture(
       layer_rect_.width(), layer_rect_.height(), tile_grid_info));
 
-  skia::RefPtr<SkCanvas> canvas;
+  skia::RefPtr<SkCanvas> ownedCanvas;
+  SkCanvas* canvas = NULL;

[tomhudson, 2014/04/17 16:44:03]

cc/ dogma considers any naked SkCanvas pointer to be a bug. Since it's
ref-counted, they *always* want it to be wrapped in a skia::RefPtr<>. This will
need finesse?

[enne (OOO), 2014/04/17 17:09:49]

And, on the third day, the clouds opened and Peevish Nit, high goddess of
compositors, descended from the heavens and said, "Lo, hide thy naked pointers
and clothe them always in counters."

The records are unclear at this point, but many accounts say that at this moment
she paused, coughed nervously, and then mumbled under her breath, "Well, I
guess, except when you're not transferring ownership or preserving lifetimes and
somebody else owns them, then I guess those naked pointers are totally ok.  But
only on alternate Thursdays, and be careful!"

[mtklein, 2014/04/17 17:32:04]

Do you guys have an UnownedPtr?  Or a MaybeOwnedPtr?  In the
RECORD_WITH_SKRECORD case, you don't own it, and you aren't allowed to extend
its lifetime by reffing.

We could of course ->ref() and ->unref() it in perfect balance before it dies in
SkRecording::Delete(), but that seems sort of equally pointlessly inefficient
and misleading.

Ignoring refcounting, what would the idiom be here for a pointer which was
conditionally exclusively owned?

[danakj, 2014/04/17 17:36:56]

You can always ref something that's refcounted. If you don't want the lifetime
extended, then it shouldn't be a refptr.
While inefficient that would make it clear that we are not taking over a
reference from skia. Raw pointer is always ambiguous.
skia::SharePtr() is the least likely thing to end up in a memory leak or a use
after free IMO.

[enne (OOO), 2014/04/17 17:41:44]

More seriously, raw Skia objects do have some code smell because they're always
a potential memory leak.  The Skia API doesn't have any documented (let alone
in-code) way of saying that some pointer it hands off is already owned
elsewhere.  So, not wrapping raw Skia pointers means you get bugs like
https://chromiumcodereview.appspot.com/19267024 and as a result there's some
allergy to this class of problems.

In this case, why not get rid of |canvas| and just use |ownedCanvas| to store
the ref, even for RECORD_WITH_SKRECORD?

+  EXPERIMENTAL::SkRecording* recording = NULL;
+
   switch (recording_mode) {
     case RECORD_NORMALLY:
-      canvas = skia::SharePtr(picture_->beginRecording(
+      ownedCanvas = skia::SharePtr(picture_->beginRecording(
           layer_rect_.width(),
           layer_rect_.height(),
           SkPicture::kUsePathBoundsForClip_RecordingFlag |
               SkPicture::kOptimizeForClippedPlayback_RecordingFlag));
+      canvas = ownedCanvas.get();
       break;
     case RECORD_WITH_SK_NULL_CANVAS:
-      canvas = skia::AdoptRef(SkCreateNullCanvas());
+      ownedCanvas = skia::AdoptRef(SkCreateNullCanvas());
+      canvas = ownedCanvas.get();
       break;
     case RECORD_WITH_PAINTING_DISABLED:
       // Blink's GraphicsContext will disable painting when given a NULL
       // canvas.
       break;
+    case RECORD_WITH_SKRECORD: {
+      recording = EXPERIMENTAL::SkRecording::Create(layer_rect_.width(),
+                                                    layer_rect_.height());
+      canvas = recording->canvas();
+      break;
+    }
     default:
       NOTREACHED();
   }
@@ -285,13 +314,17 @@ void Picture::Record(ContentLayerClient* painter,
   }
 
   gfx::RectF opaque_layer_rect;
-  painter->PaintContents(canvas.get(), layer_rect_, &opaque_layer_rect);
+  painter->PaintContents(canvas, layer_rect_, &opaque_layer_rect);
 
   if (canvas)
     canvas->restore();
   if (picture_->getRecordingCanvas())
     picture_->endRecording();
 
+  if (recording != NULL) {
+    playback_.reset(EXPERIMENTAL::SkRecording::Delete(recording));
+  }
+
   opaque_rect_ = gfx::ToEnclosedRect(opaque_layer_rect);
 
   EmitTraceSnapshot();
@@ -374,7 +407,11 @@ int Picture::Raster(
 
   canvas->scale(contents_scale, contents_scale);
   canvas->translate(layer_rect_.x(), layer_rect_.y());
-  picture_->draw(canvas, callback);
+  if (playback_.get() != NULL) {
+    playback_->draw(canvas);
+  } else {
+    picture_->draw(canvas, callback);
+  }
   SkIRect bounds;
   canvas->getClipDeviceBounds(&bounds);
   canvas->restore();
@@ -389,7 +426,11 @@ void Picture::Replay(SkCanvas* canvas) {
   TRACE_EVENT_BEGIN0("cc", "Picture::Replay");
   DCHECK(picture_);
 
-  picture_->draw(canvas);
+  if (playback_.get() != NULL) {
+    playback_->draw(canvas);
+  } else {
+    picture_->draw(canvas);
+  }
   SkIRect bounds;
   canvas->getClipDeviceBounds(&bounds);
   TRACE_EVENT_END1("cc", "Picture::Replay",