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

cc/resources/picture.cc

 // 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.
 
 #include "cc/resources/picture.h"
 
 #include <algorithm>
 #include <limits>
 #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"
 #include "cc/base/util.h"
 #include "cc/debug/traced_picture.h"
 #include "cc/debug/traced_value.h"
 #include "cc/layers/content_layer_client.h"
 #include "skia/ext/pixel_ref_utils.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkData.h"
@@ skipping 162 matching lines @@
     pixel_refs_(pixel_refs),
     cell_size_(layer_rect.size()) {
 }
 
 Picture::~Picture() {
   TRACE_EVENT_OBJECT_DELETED_WITH_ID(
     TRACE_DISABLED_BY_DEFAULT("cc.debug"), "cc::Picture", this);
 }
 
 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);
   return thread_index == clones_.size() ? this : clones_[thread_index].get();
 }
 
 bool Picture::IsSuitableForGpuRasterization() const {
   DCHECK(picture_);
 
   // TODO(alokp): SkPicture::suitableForGpuRasterization needs a GrContext.
   // Ideally this GrContext should be the same as that for rasterizing this
   // picture. But we are on the main thread while the rasterization context
   // may be on the compositor or raster thread.
   // SkPicture::suitableForGpuRasterization is not implemented yet.
   // Pass a NULL context for now and discuss with skia folks if the context
   // is really needed.
   return picture_->suitableForGpuRasterization(NULL);
 }
 
 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());
 
   // We can re-use this picture for one raster worker thread.
   raster_thread_checker_.DetachFromThread();
 
   if (num_threads > 1) {
     scoped_ptr<SkPicture[]> clones(new SkPicture[num_threads - 1]);
     picture_->clone(&clones[0], num_threads - 1);
 
     for (int i = 0; i < num_threads - 1; i++) {
       scoped_refptr<Picture> clone = make_scoped_refptr(
           new Picture(skia::AdoptRef(new SkPicture(clones[i])),
                       layer_rect_,
                       opaque_rect_,
                       pixel_refs_));
       clones_.push_back(clone);
 
       clone->EmitTraceSnapshotAlias(this);
       clone->raster_thread_checker_.DetachFromThread();
     }
   }
 }
 
 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",
                AsTraceableRecordData(),
                "recording_mode",
                recording_mode);
 
   DCHECK(!picture_);
   DCHECK(!tile_grid_info.fTileInterval.isEmpty());
   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();
   }
 
   if (canvas) {
     canvas->save();
     canvas->translate(SkFloatToScalar(-layer_rect_.x()),
                       SkFloatToScalar(-layer_rect_.y()));
 
     SkRect layer_skrect = SkRect::MakeXYWH(layer_rect_.x(),
                                            layer_rect_.y(),
                                            layer_rect_.width(),
                                            layer_rect_.height());
     canvas->clipRect(layer_skrect);
   }
 
   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();
 }
 
 void Picture::GatherPixelRefs(
     const SkTileGridPicture::TileGridInfo& tile_grid_info) {
   TRACE_EVENT2("cc", "Picture::GatherPixelRefs",
                "width", layer_rect_.width(),
                "height", layer_rect_.height());
@@ skipping 62 matching lines @@
 
   DCHECK(picture_);
 
   canvas->save();
 
   for (Region::Iterator it(negated_content_region); it.has_rect(); it.next())
     canvas->clipRect(gfx::RectToSkRect(it.rect()), SkRegion::kDifference_Op);
 
   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();
   TRACE_EVENT_END1(
       "cc", "Picture::Raster",
       "num_pixels_rasterized", bounds.width() * bounds.height());
   return bounds.width() * bounds.height();
 }
 
 void Picture::Replay(SkCanvas* canvas) {
   DCHECK(raster_thread_checker_.CalledOnValidThread());
   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",
                    "num_pixels_replayed", bounds.width() * bounds.height());
 }
 
 scoped_ptr<base::Value> Picture::AsValue() const {
   SkDynamicMemoryWStream stream;
 
   // Serialize the picture.
@@ skipping 137 matching lines @@
 
 scoped_refptr<base::debug::ConvertableToTraceFormat>
     Picture::AsTraceableRecordData() const {
   scoped_ptr<base::DictionaryValue> record_data(new base::DictionaryValue());
   record_data->Set("picture_id", TracedValue::CreateIDRef(this).release());
   record_data->Set("layer_rect", MathUtil::AsValue(layer_rect_).release());
   return TracedValue::FromValue(record_data.release());
 }
 
 }  // namespace cc