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

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

BUG=skia:2378
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267209

[Sami, 2014-04-08 10:47:44]

I think this looks great. Probably not relevant for testing, but do we need to
worry about cloning[1]? Maybe just add an assert there.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pictu...

[Sami, 2014-04-08 11:10:53]

Another random idea: let's make this the default if we can get away with it, but
if not, we could tie it to the --enable-bleeding-edge-rendering-fast-paths flag.

[mtklein, 2014-04-08 14:20:23]

On 2014/04/08 11:10:53, Sami wrote:
> Another random idea: let's make this the default if we can get away with it,
but
> if not, we could tie it to the --enable-bleeding-edge-rendering-fast-paths
flag.

Both suggestions done.
  1) Cloning is trivially easy with SkRecord: just draw from the same const
SkRecord.
  2) I hooked it up to the flag (and made it accessible on more than just
Android), but checkdeps yells at me when I try to include
content/public/common/content_switches.h to get access to
switches::kEnableBleedingEdgeRenderingFastPaths.  Any idea how best to handle
this?

Florin's here with me and we've kicked off another run of
rasterize_and_record_micro, forcing both SkRecord and his culling flag, hoping
that will win back lost playback performance.  Will post the results on the bug
like the first run when it's done.

[Sami, 2014-04-08 14:31:20]

On 2014/04/08 14:20:23, mtklein wrote:
>   1) Cloning is trivially easy with SkRecord: just draw from the same const
> SkRecord.

Neat. I was wondering if there was still something that made painting
non-thread-safe but I guess not.

>   2) I hooked it up to the flag (and made it accessible on more than just
> Android), but checkdeps yells at me when I try to include
> content/public/common/content_switches.h to get access to
> switches::kEnableBleedingEdgeRenderingFastPaths.  Any idea how best to handle
> this?

You could add a setting for this in cc::LayerTreeSettings and enable it thusly:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/g...
 
> Florin's here with me and we've kicked off another run of
> rasterize_and_record_micro, forcing both SkRecord and his culling flag, hoping
> that will win back lost playback performance.  Will post the results on the
bug
> like the first run when it's done.

Looking forward to it :)

[mtklein, 2014-04-08 14:32:01]

On 2014/04/08 14:20:23, mtklein wrote:
> On 2014/04/08 11:10:53, Sami wrote:
> > Another random idea: let's make this the default if we can get away with it,
> but
> > if not, we could tie it to the --enable-bleeding-edge-rendering-fast-paths
> flag.
> 
> Both suggestions done.
>   1) Cloning is trivially easy with SkRecord: just draw from the same const
> SkRecord.
>   2) I hooked it up to the flag (and made it accessible on more than just
> Android), but checkdeps yells at me when I try to include
> content/public/common/content_switches.h to get access to
> switches::kEnableBleedingEdgeRenderingFastPaths.  Any idea how best to handle
> this?
> 
> Florin's here with me and we've kicked off another run of
> rasterize_and_record_micro, forcing both SkRecord and his culling flag, hoping
> that will win back lost playback performance.  Will post the results on the
bug
> like the first run when it's done.

Eh, don't get too excited.  I've got some SkRecord plumbing to do before it can
take advantage of Florin's culling.  This run should be just like last night's.

[tomhudson, 2014-04-17 16:44:02]

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:274: SkCanvas* canvas = NULL;
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]

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:255: // HACK HACK HACK HACK: checkdeps won't let me look
at this flag here.  :(
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.

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:274: SkCanvas* canvas = NULL;
On 2014/04/17 16:44:03, tomhudson wrote:
> 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?

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:03]

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:255: // HACK HACK HACK HACK: checkdeps won't let me look
at this flag here.  :(
On 2014/04/17 17:09:49, enne wrote:
> 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.

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

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:274: SkCanvas* canvas = NULL;
On 2014/04/17 17:09:49, enne wrote:
> On 2014/04/17 16:44:03, tomhudson wrote:
> > 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?
> 
> 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!"

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]

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:274: SkCanvas* canvas = NULL;
On 2014/04/17 17:32:04, mtklein wrote:
> On 2014/04/17 17:09:49, enne wrote:
> > On 2014/04/17 16:44:03, tomhudson wrote:
> > > 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?
> > 
> > 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!"
> 
> 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.

You can always ref something that's refcounted. If you don't want the lifetime
extended, then it shouldn't be a refptr.

> 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.

While inefficient that would make it clear that we are not taking over a
reference from skia. Raw pointer is always ambiguous.

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

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:43]

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/180001/cc/resources/picture.cc...
cc/resources/picture.cc:274: SkCanvas* canvas = NULL;
On 2014/04/17 17:09:49, enne wrote:
> On 2014/04/17 16:44:03, tomhudson wrote:
> > 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?
> 
> 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!"

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?

[mtklein, 2014-04-17 17:48:14]

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

SGTM.  I'll get rid of this canvas and rename ownedCanvas back to canvas. 
Getting scoping to deref the canvas before I return the SkRecording* looks a
little tricky.  It's kosher to just .clear() the canvas pointer preemptively,
right?

[danakj, 2014-04-17 17:52:30]

On Thu, Apr 17, 2014 at 1:48 PM, <mtklein@google.com> wrote:

> In this case, why not get rid of |canvas| and just use |ownedCanvas| to
>> store
>> the ref, even for RECORD_WITH_SKRECORD?
>>
>
> SGTM.  I'll get rid of this canvas and rename ownedCanvas back to canvas.
> Getting scoping to deref the canvas before I return the SkRecording* looks
> a
> little tricky.  It's kosher to just .clear() the canvas pointer
> preemptively,
> right?
>

Yep.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mtklein, 2014-04-22 17:13:20]

Hi folks,

Can you take a look at PS 17?  I've reverted all the bits around hooking into
the about:flag for now.  I'd like to land this so the benchmarks pick up the
_skr variant sooner rather than later.  I don't want to hold things up while I
bumble around trying to hook up the flag.

[danakj, 2014-04-22 17:25:52]

just nits from me and complaining about the skia API, the refcounting looks
better thanks.

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
cc/resources/picture.cc:195: if (playback_.get() != NULL) {
nit: if (playback_) is all you need for scoped_ptr

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
cc/resources/picture.cc:267: EXPERIMENTAL::SkRecording* recording = NULL;
given the Delete() API I guess this isn't possible, but I really wish this could
be a scoped_ptr. This API requires manual manipulation of this pointer's
lifetime which makes me sad.

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
cc/resources/picture.cc:316: if (recording != NULL) {
nit: just if (recording)

[enne (OOO), 2014-04-22 17:26:03]

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
cc/resources/picture.cc:267: EXPERIMENTAL::SkRecording* recording = NULL;
Can you help me understand the lifetime issues with recordings and canvases? I
assume that the recording holds a ref on the canvas it returns?

It's a little weird to me that you have to explicitly call Delete on a variable
that's only alive for this scope.  Can this not be a scoped_ptr or some similar
Skia construct?

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
cc/resources/picture.cc:317: canvas.clear();  // Drop ref on canvas before
Delete invalidates it.
Why?

[mtklein, 2014-04-22 17:44:32]

On 2014/04/22 17:26:03, enne wrote:
> https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc
> File cc/resources/picture.cc (right):
> 
>
https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
> cc/resources/picture.cc:267: EXPERIMENTAL::SkRecording* recording = NULL;
> Can you help me understand the lifetime issues with recordings and canvases? I
> assume that the recording holds a ref on the canvas it returns?
> 
> It's a little weird to me that you have to explicitly call Delete on a
variable
> that's only alive for this scope.  Can this not be a scoped_ptr or some
similar
> Skia construct?
> 
>
https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
> cc/resources/picture.cc:317: canvas.clear();  // Drop ref on canvas before
> Delete invalidates it.
> Why?

All good questions.  Delete does double duty here.  It's both destroying the
SkRecording and transmogrifying it into an SkPlayback, which is the thing that
draws, is immutable, and can be held normally by a scoped_ptr.  Once you've got
the SkPlayback, that target canvas you were drawing into no longer exists; if we
allowed it to overlap the life of the SkPlayback, we couldn't guarantee the
SkPlayback's immutability.

I like to think of SkRecording as a somewhat involved constructor call to
ultimately get back an SkPlayback.  Without anything else going on, the code
might look like this:

scoped_ptr<EXPERIMENTAL::SkPlayback> playback;
{
   EXPERIMENTAL::SkRecording* recording = EXPERIMENTAL::SkRecording::Create(w,
h);
   DoTheDrawing(recording->canvas());
   playback.reset(EXPERIMENTAL::SkRecording::Delete(recording));
}

EXPERIMENTAL::{SkRecording,SkPlayback} exist only to provide Chrome a stable
interface to SkRecord and friends, so that we can rejigger all those pieces as
we like without affecting Chrome too much.  So as a corollary, we can mess
around with how the interfaces to SkRecording and SkPlayback work to suit your
needs, desires, whims, whatever.  I'm more than happy to iterate with you guys
here.

If we can bridge the gap between callbacks here and
function-pointer-and-void-pointer for Skia, we could cut this down
significantly:
scoped_ptr<EXPERIMENTAL::SkPlayback> playback_;
...
playback_.reset(new EXPERIMENTAL::SkPlayback(w, h,
SomehowPassACallbackToSkia(DoTheDrawing)));

[mtklein, 2014-04-22 17:48:55]

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/300001/cc/resources/picture.cc...
cc/resources/picture.cc:195: if (playback_.get() != NULL) {
On 2014/04/22 17:25:53, danakj wrote:
> nit: if (playback_) is all you need for scoped_ptr

Done everywhere.

[enne (OOO), 2014-04-22 18:24:51]

I apologize if we're a bit allergic to raw pointers.  There's not much expertise
in Skia code on this side of the fence, and we've continually introduced leaks
and crashes because of missing ref/unrefs.  I think there's some desire to make
things as impossible to screw up as much as possible across API boundaries.

Here's a suggestion for something I think would be cleaner:

    scoped_ptr<EXPERIMENTAL::SkRecording>
recording(EXPERIMENTAL::SkRecording::Create(...));
    skia::RefPtr<SkCanvas> canvas(skia::SharePtr(recording->canvas()));
    // paint into canvas
    scoped_ptr<const EXPERIMENTAL::SkPlayback>
playback_(recording.ReleasePlayback());

The changes here are:
* All raw pointers are stored in scoped_ptr or skia::RefPtr and don't require
any manual clearing or deletion

* If SkRecording doesn't release the playback, then it deletes it itself
(deletion and return values aren't mixed).  Alternatively rather than
ReleasePlayback, even a non-destructive CreatePlayback() would work too if
SkRecordAnnotateCullingPairs can be called on the same SkRecord twice safely.

* Store fRecorder in a ref ptr inside of SkRecording (rather than manually
deleting it) so that it can outlive the Recording and doesn't need to be
manually cleared (I'm sure there's some reason I'm missing why you made the
fRecorder single ownership even though it's a refcounted canvas, but if it's
possible to do something like the above that'd be ideal.)


I also know this is experimental and don't know the shape of what things are
coming.  If this is makework and all of this will evaporate next week, please
let me know.  However, if this is going to stick around for a while, then having
something safer would really be appreciated.

[mtklein, 2014-04-22 18:57:48]

On 2014/04/22 18:24:51, enne wrote:
> I apologize if we're a bit allergic to raw pointers.  There's not much
expertise
> in Skia code on this side of the fence, and we've continually introduced leaks
> and crashes because of missing ref/unrefs.  I think there's some desire to
make
> things as impossible to screw up as much as possible across API boundaries.

Yeah, I understand this is a pain.  I may be getting a bit aggressively ahead of
the curve here by treating that SkCanvas* as exclusively owned, but read on
below for why.

> Here's a suggestion for something I think would be cleaner:
> 
>     scoped_ptr<EXPERIMENTAL::SkRecording>
> recording(EXPERIMENTAL::SkRecording::Create(...));
>     skia::RefPtr<SkCanvas> canvas(skia::SharePtr(recording->canvas()));
>     // paint into canvas
>     scoped_ptr<const EXPERIMENTAL::SkPlayback>
> playback_(recording.ReleasePlayback());

I'm very down for doing this.  I'd like to follow up with that change if that's
alright with you, so as not to let waiting on the implied Skia changes block
this one from landing.

> The changes here are:
> * All raw pointers are stored in scoped_ptr or skia::RefPtr and don't require
> any manual clearing or deletion
> 
> * If SkRecording doesn't release the playback, then it deletes it itself
> (deletion and return values aren't mixed).  Alternatively rather than
> ReleasePlayback, even a non-destructive CreatePlayback() would work too if
> SkRecordAnnotateCullingPairs can be called on the same SkRecord twice safely.

We can call SkRecordOptimize on the same SkRecord twice safely, but it's nice to
run exactly once, and we can't call it concurrently with a playback.  I think
I'm going to go for the ReleasePlayback approach, if only because we wouldn't
yet need to CreatePlayback more than once.  We might as well think of it and
call it a release.

> * Store fRecorder in a ref ptr inside of SkRecording (rather than manually
> deleting it) so that it can outlive the Recording and doesn't need to be
> manually cleared (I'm sure there's some reason I'm missing why you made the
> fRecorder single ownership even though it's a refcounted canvas, but if it's
> possible to do something like the above that'd be ideal.)

This one is a tricky bit.  If someone's holding a ref on that canvas, they can
continue writing into the underlying SkRecord unchecked, which blows away any
notion of threadsafety when playing that SkRecord back (e.g. via SkPlayback). 
We're trying to be very protective of that canvas so that it can't leak out some
place.  It's perfectly fine to ref and unref the canvas while recording if it
makes code feel better, but it'd be a major safety problem if the SkRecorder
actually outlived the SkRecording.  This is why I'm keen on exclusive ownership
and killing it with delete before handing back the SkPlayback: we'll debugfail
if anyone was holding a ref.  That said, of course we can also make
SkRecording's destructor clean up everything too just in case no one ever calls
ReleasePlayback.  This is a place where I'd tend to err on the side of tools and
tests to catch the bug (there's no logical reason to create and SkRecording
except to get its SkPlayback) but I'm happy to make all paths safe.

> I also know this is experimental and don't know the shape of what things are
> coming.  If this is makework and all of this will evaporate next week, please
> let me know.  However, if this is going to stick around for a while, then
having
> something safer would really be appreciated.

Yeah, no matter how much we mark something as experimental, it's inevitably
going to be relied upon.  That's why you see the API that's here: we wanted to
constrain it as much as possible to a single mode of operation.  I do agree we
can probably keep all or most of that constraint while using smart pointers to
provide more safety too.

[enne (OOO), 2014-04-22 19:46:35]

On 2014/04/22 18:57:48, mtklein wrote:

> > Here's a suggestion for something I think would be cleaner:
> > 
> >     scoped_ptr<EXPERIMENTAL::SkRecording>
> > recording(EXPERIMENTAL::SkRecording::Create(...));
> >     skia::RefPtr<SkCanvas> canvas(skia::SharePtr(recording->canvas()));
> >     // paint into canvas
> >     scoped_ptr<const EXPERIMENTAL::SkPlayback>
> > playback_(recording.ReleasePlayback());
> 
> I'm very down for doing this.  I'd like to follow up with that change if
that's
> alright with you, so as not to let waiting on the implied Skia changes block
> this one from landing.

Sounds good, as long as you file a bug and leave a TODO in the code referencing
it.

> > * If SkRecording doesn't release the playback, then it deletes it itself
> > (deletion and return values aren't mixed).  Alternatively rather than
> > ReleasePlayback, even a non-destructive CreatePlayback() would work too if
> > SkRecordAnnotateCullingPairs can be called on the same SkRecord twice
safely.
> 
> We can call SkRecordOptimize on the same SkRecord twice safely, but it's nice
to
> run exactly once, and we can't call it concurrently with a playback.  I think
> I'm going to go for the ReleasePlayback approach, if only because we wouldn't
> yet need to CreatePlayback more than once.  We might as well think of it and
> call it a release.

Sounds good.

> > * Store fRecorder in a ref ptr inside of SkRecording (rather than manually
> > deleting it) so that it can outlive the Recording and doesn't need to be
> > manually cleared (I'm sure there's some reason I'm missing why you made the
> > fRecorder single ownership even though it's a refcounted canvas, but if it's
> > possible to do something like the above that'd be ideal.)
> 
> This one is a tricky bit.  If someone's holding a ref on that canvas, they can
> continue writing into the underlying SkRecord unchecked, which blows away any
> notion of threadsafety when playing that SkRecord back (e.g. via SkPlayback). 

Could ReleasePlayback also clear the fRecord off of the SkRecorder and
SkRecording? (It seems like its an ownership transfer.)  If something tried to
use the SkCanvas/SkRecorder after that it'd crash on the NULL pointer, but
that's a clearer failure than thread raciness.

[nduca, 2014-04-22 23:29:05]

lgtm assuming you get lg from dana or enne

the clear() on the canvas is funky would be good to eliminate plz

also please correct the commit message

https://codereview.chromium.org/225023031/diff/300001/cc/debug/rasterize_and_...
File cc/debug/rasterize_and_record_benchmark.cc (right):

https://codereview.chromium.org/225023031/diff/300001/cc/debug/rasterize_and_...
cc/debug/rasterize_and_record_benchmark.cc:28: "", "_sk_null_canvas",
"_painting_disabled", "_skr"};
_skrecord plz?

[mtklein, 2014-04-22 23:38:59]

On 2014/04/22 23:29:05, nduca wrote:
> lgtm assuming you get lg from dana or enne
> 
> the clear() on the canvas is funky would be good to eliminate plz
This code will change a bit once I land
https://codereview.chromium.org/248033002/, but the .clear() bit on canvas is
going to still be there, and for the same reason.  It's okay if Picture wants to
ref canvas while it's using it, but to be able to call
SkRecording::detachPlayback (to get the SkPlayback* it'll use to draw), it needs
to drop its ref first.  It's not safe to call detachPlayback unless the
SkRecording is the exclusive owner of that canvas pointer, and we enforce that.

> also please correct the commit message
You mean, get rid of the questions?  How's this?

>
https://codereview.chromium.org/225023031/diff/300001/cc/debug/rasterize_and_...
> File cc/debug/rasterize_and_record_benchmark.cc (right):
> 
>
https://codereview.chromium.org/225023031/diff/300001/cc/debug/rasterize_and_...
> cc/debug/rasterize_and_record_benchmark.cc:28: "", "_sk_null_canvas",
> "_painting_disabled", "_skr"};
> _skrecord plz?
Done.

[Sami, 2014-04-23 10:34:37]

Mind adding the benchmark hookup to this patch too? It makes life a little
easier for the bots if we need to revert for some reason.

diff --git a/tools/perf/measurements/rasterize_and_record_micro.py
b/tools/perf/measurements/rasterize_and_record_micro.py
index aa0bfd0..6f85bdc 100644
--- a/tools/perf/measurements/rasterize_and_record_micro.py
+++ b/tools/perf/measurements/rasterize_and_record_micro.py
@@ -106,10 +106,12 @@ class
RasterizeAndRecordMicro(page_measurement.PageMeasurement):
         sys.platform == 'android'):
       record_time_sk_null_canvas = data['record_time_sk_null_canvas_ms']
       record_time_painting_disabled = data['record_time_painting_disabled_ms']
+      record_time_skrecord = data['record_time_skrecord']
       results.Add('record_time_sk_null_canvas', 'ms',
           record_time_sk_null_canvas)
       results.Add('record_time_painting_disabled', 'ms',
           record_time_painting_disabled)
+      results.Add('record_time_skrecord', 'ms', record_time_skrecord)
 
     if self.options.report_detailed_results:
       pixels_rasterized_with_non_solid_color = \

[mtklein, 2014-04-23 14:49:59]

On 2014/04/23 10:34:37, Sami wrote:
> Mind adding the benchmark hookup to this patch too? It makes life a little
> easier for the bots if we need to revert for some reason.
> 
> diff --git a/tools/perf/measurements/rasterize_and_record_micro.py
> b/tools/perf/measurements/rasterize_and_record_micro.py
> index aa0bfd0..6f85bdc 100644
> --- a/tools/perf/measurements/rasterize_and_record_micro.py
> +++ b/tools/perf/measurements/rasterize_and_record_micro.py
> @@ -106,10 +106,12 @@ class
> RasterizeAndRecordMicro(page_measurement.PageMeasurement):
>          sys.platform == 'android'):
>        record_time_sk_null_canvas = data['record_time_sk_null_canvas_ms']
>        record_time_painting_disabled =
data['record_time_painting_disabled_ms']
> +      record_time_skrecord = data['record_time_skrecord']
>        results.Add('record_time_sk_null_canvas', 'ms',
>            record_time_sk_null_canvas)
>        results.Add('record_time_painting_disabled', 'ms',
>            record_time_painting_disabled)
> +      results.Add('record_time_skrecord', 'ms', record_time_skrecord)
>  
>      if self.options.report_detailed_results:
>        pixels_rasterized_with_non_solid_color = \

Done.  Sorry, that must have gotten lost in rebasing.

[mtklein, 2014-04-25 13:29:36]

Hi gang, PTAL at #21?  This can land after Skia's next roll into Chrome.

[enne (OOO), 2014-04-25 17:34:21]

lgtm

https://codereview.chromium.org/225023031/diff/380001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/380001/cc/resources/picture.cc...
cc/resources/picture.cc:318: canvas.clear();  // Drop ref on canvas before
calling releasePlayback().
Can this comment say why and not what?

Alternatively, don't clear the canvas and just leave a comment that says "After
releasing the SkPlayback, the canvas is no longer valid?".  It doesn't get used
here in this function and would blow up immediately if it did post-release.

[mtklein, 2014-04-28 19:03:18]

https://codereview.chromium.org/225023031/diff/380001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/225023031/diff/380001/cc/resources/picture.cc...
cc/resources/picture.cc:318: canvas.clear();  // Drop ref on canvas before
calling releasePlayback().
On 2014/04/25 17:34:22, enne wrote:
> Can this comment say why and not what?
> 
> Alternatively, don't clear the canvas and just leave a comment that says
"After
> releasing the SkPlayback, the canvas is no longer valid?".  It doesn't get
used
> here in this function and would blow up immediately if it did post-release.

Done.

[mtklein, 2014-04-28 19:03:41]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-28 19:04:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/420001

[commit-bot: I haz the power, 2014-04-28 19:11:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:11:42]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[bungeman-skia, 2014-04-28 19:18:54]

lgtm

[mtklein, 2014-04-28 19:19:18]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-28 19:20:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/420001

[commit-bot: I haz the power, 2014-04-28 19:27:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:27:37]

Try jobs failed on following builders:
  tryserver.chromium on android_clang_dbg
  tryserver.chromium on linux_chromium_chromeos_rel

[mtklein, 2014-04-28 19:36:29]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-28 19:38:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/440001

[commit-bot: I haz the power, 2014-04-28 19:46:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 19:46:13]

Try jobs failed on following builders:
  tryserver.chromium on android_clang_dbg

[mtklein, 2014-04-28 20:51:33]

On 2014/04/28 19:46:13, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on android_clang_dbg

FYI, will try this again tomorrow after
https://codereview.chromium.org/253883003 rolls into Chrome.  I think that
should fix the failing builds where Skia's built as an .so.

[mtklein, 2014-04-29 19:52:38]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-29 19:54:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/440001

[commit-bot: I haz the power, 2014-04-30 00:03:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 00:03:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[mtklein, 2014-04-30 00:05:14]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-30 00:08:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/440001

[commit-bot: I haz the power, 2014-04-30 01:01:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 01:01:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[mtklein, 2014-04-30 10:29:57]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-30 10:30:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/460001

[commit-bot: I haz the power, 2014-04-30 10:35:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 10:35:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[mtklein, 2014-04-30 10:36:24]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-30 10:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@chromium.org/225023031/460001

[commit-bot: I haz the power, 2014-04-30 14:53:51]

Change committed as 267209