split SkPictureRecorder out of SkPicture

split SkPictureRecorder out of SkPicture.

The most interesting changes are in SkPicture.h and then propagate out from there.

In terms of staging, we'll have to land something like this, update Chromium and then remove the old SkPicture entry points.

[reed1, 2014-03-27 20:52:01]

is it too soon to introduce a guard to hide the recording api on SkPicture?

#ifdef SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD
    SkCanvas* beginRecording(...);
    ...
#endif

What is the point of the factory? Is that just for the bloody bbox hierarchy
subclass?

[mtklein, 2014-03-27 21:09:10]

I think we might want to get that BBH issue sorted out before landing this. 
Things become rather simpler if we don't have to think about picture factories,
no?

https://codereview.chromium.org/214953003/diff/60001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/214953003/diff/60001/include/core/SkPicture.h...
include/core/SkPicture.h:395: class SK_API SkPictureRecorder : public SkRefCnt {
What's this extra level of indirection get us?  I'd have expected

class SkPictureRecorder : public SkCanvas {
  SkPictureRecorder(int w, int h);
  SkPicture* endRecording();
  ...
};

Are we ever refcounting SkPictureRecorders?  I'm seeing most of them on the
stack.

[reed1, 2014-03-27 21:11:04]

On 2014/03/27 21:09:10, mtklein wrote:
> I think we might want to get that BBH issue sorted out before landing this. 
> Things become rather simpler if we don't have to think about picture
factories,
> no?
> 
> https://codereview.chromium.org/214953003/diff/60001/include/core/SkPicture.h
> File include/core/SkPicture.h (right):
> 
>
https://codereview.chromium.org/214953003/diff/60001/include/core/SkPicture.h...
> include/core/SkPicture.h:395: class SK_API SkPictureRecorder : public SkRefCnt
{
> What's this extra level of indirection get us?  I'd have expected
> 
> class SkPictureRecorder : public SkCanvas {
>   SkPictureRecorder(int w, int h);
>   SkPicture* endRecording();
>   ...
> };
> 
> Are we ever refcounting SkPictureRecorders?  I'm seeing most of them on the
> stack.

Good question.

Related -- there is a drive to make SkCanvas *not* be ref-counted...

[mtklein, 2014-03-27 21:14:05]

Stepping back a second... why are we doing this?  Can we do it without changing
SkPicture's public API, or is the idea to change the public API?

If so, let's wait.  If we're going to change our recording APIs in Chrome and
Blink, I'd rather do it once to my new recording code than twice.  I'd been
figuring I'd just hide everything behind SkPicture's interface as it sits today,
but splitting apart recording, playback and the picture itself is very natural
for me too.

[reed1, 2014-03-27 21:37:57]

On 2014/03/27 21:14:05, mtklein wrote:
> Stepping back a second... why are we doing this?  Can we do it without
changing
> SkPicture's public <span id="abbrex-spnAbbr_6929" class="abbrex-Abbr
abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span>, or is
the idea to change the public <span id="abbrex-spnAbbr_6930" class="abbrex-Abbr
abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span>?
> 
> If so, let's wait.  If we're going to change our recording <span
id="abbrex-spnAbbr_6931" class="abbrex-Abbr abbrex-Abbr_API abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">API</span>s in Chrome and
> Blink, I'd rather do it once to my new recording code than twice.  I'd been
> figuring I'd just hide everything behind SkPicture's interface as it sits
today,
> but splitting apart recording, playback and the picture itself is very natural
> for me too.

Now I'm confused. Perhaps we 3 can draw pictures on the board tomorrow.

One motivation is to reinforce the following notions:
- playable pictures are CONST
- a recording can't re-record and affect the previous playable

[mtklein, 2014-03-27 21:40:49]

On 2014/03/27 21:37:57, reed1 wrote:
> On 2014/03/27 21:14:05, mtklein wrote:
> > Stepping back a second... why are we doing this?  Can we do it without
> changing
> > SkPicture's public <span id="abbrex-spnAbbr_6929" class="abbrex-Abbr
> abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span>, or
is
> the idea to change the public <span id="abbrex-spnAbbr_6930"
class="abbrex-Abbr
> abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span>?
> > 
> > If so, let's wait.  If we're going to change our recording <span
> id="abbrex-spnAbbr_6931" class="abbrex-Abbr abbrex-Abbr_API abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">API</span>s in Chrome and
> > Blink, I'd rather do it once to my new recording code than twice.  I'd been
> > figuring I'd just hide everything behind SkPicture's interface as it sits
> today,
> > but splitting apart recording, playback and the picture itself is very
natural
> > for me too.
> 
> Now I'm confused. Perhaps we 3 can draw pictures on the board tomorrow.
> 
> One motivation is to reinforce the following notions:
> - playable pictures are CONST
> - a recording can't re-record and affect the previous playable

Ah, yeah, let's draw then.  I'm strongly in favor of both notions.  This may
then be a case of me walking you guys through my proposed code, which supports
these two notions natively.  My request here is that if we're going to change
the APIs, let's do it once.

[mtklein, 2014-03-27 21:42:54]

On 2014/03/27 21:40:49, mtklein wrote:
> On 2014/03/27 21:37:57, reed1 wrote:
> > On 2014/03/27 21:14:05, mtklein wrote:
> > > Stepping back a second... why are we doing this?  Can we do it without
> > changing
> > > SkPicture's public <span id="abbrex-spnAbbr_6929" class="abbrex-Abbr
> > abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span>, or
> is
> > the idea to change the public <span id="abbrex-spnAbbr_6930"
> class="abbrex-Abbr
> > abbrex-Abbr_API abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">API</span>?
> > > 
> > > If so, let's wait.  If we're going to change our recording <span
> > id="abbrex-spnAbbr_6931" class="abbrex-Abbr abbrex-Abbr_API
abbrex-AbbrUnknown
> > abbrex-AbbrUnknownVisible">API</span>s in Chrome and
> > > Blink, I'd rather do it once to my new recording code than twice.  I'd
been
> > > figuring I'd just hide everything behind SkPicture's interface as it sits
> > today,
> > > but splitting apart recording, playback and the picture itself is very
> natural
> > > for me too.
> > 
> > Now I'm confused. Perhaps we 3 can draw pictures on the board tomorrow.
> > 
> > One motivation is to reinforce the following notions:
> > - playable pictures are CONST
> > - a recording can't re-record and affect the previous playable
> 
> Ah, yeah, let's draw then.  I'm strongly in favor of both notions.  This may
> then be a case of me walking you guys through my proposed code, which supports
> these two notions natively.  My request here is that if we're going to change
> the APIs, let's do it once.

(It may very well be this is the API change we want to do once, and I replace
the guts behind this.)

[robertphillips, 2014-03-27 23:15:45]

I view this patch as the first in a long chain (in only a somewhat sequential
order):

  land this
  update Chromium to use SkPictureRecorder
  hide old interface on SkPicture
  remove internal calls to endRecording
         may need to delay until SkPicturePlayback is split out (since it
contains some state)
         also need some discussion of Surface/Image future w.r.t. SkPictures
  see how much of the recording logic we can move out of SkPicture
      hopefully all of it and just access recordDrawRect-like entry points (via
a friend relationship)
  force entirety of public SkPicture interface to be const
  update SkCanvas::drawPicture to take a const
  hide all SkPicture-derived classes (only leave factories visible)
  separate RTree so it is treated like the other SkPicture-derived classes
  hide the kOptimizeForClippedPlayback_RecordingFlag so only factories can set
it
       maybe hide kUsePathBoundsForClip_RecordingFlag at the same time?
  see about killing SkPicture-derived classes and having factories just create a
helper object that is stored and used by the recorder
  try to move all data transfered between the recorder and playback into
SkPicture
  split SkPicturePlayback into its own object (not publicly visible)
  SkPicture::draw then becomes instantiating a new SkPicturePlayback object and
letting it iterate over the picture data
  Update the SkGPUDevice's optimized drawPicture call to use a derived
SkPicturePlayback object to skip over saveLayers that have already been rendered
(replacing them with a drawBitmap)
  
  On a related note, right now SkGpuDevice's optimize call is forced to playback
the SkPicture to extract the saveLayer range information. At some point I would
like to get the picture to a state where I can just iterate over the operations
(probably via a minimal SkPicturePlayback-derived object) and just note the
offsets of the saveLayers and matching restores.
  
For the specific questions:

  I will add the guard in this CL
  The factories are for the BBH classes but (unless they go away entirely) we
will probably need some form of factory passed in to the recorder (e.g., a
factory to create the BBH object itself rather then an SkPicture-derived object)
  The BBH factories don't complicate things too much and I think they will
become easier to sort (modulo the distant possibility of deleting them entirely)
once they are hidden
  I will make SkPictureRecorder non-ref-counted (don't know what I was thinking)
  The idea is to change SkPicture's API
      this change clarifies when the product of the recording process is
available
  I don't think we should wait - I think this answers a separate need from
recording speed and seems like it will make any future changes we need to make
easier
  Every API change in Chrome & Android is a dance and is going to take several
passes. I think this gets us closer to where we want to be. Landing a full on
split of SkPicture into 3 components plus a radical alteration of its behavior
all at once would be somewhat difficult to stage.

[mtklein, 2014-03-28 02:10:11]

On 2014/03/27 23:15:45, robertphillips wrote:
> I view this patch as the first in a long chain (in only a somewhat sequential
> order):
> 
>   land this
>   update Chromium to use SkPictureRecorder
>   hide old interface on SkPicture
>   remove internal calls to endRecording
>          may need to delay until SkPicturePlayback is split out (since it
> contains some state)
>          also need some discussion of Surface/Image future w.r.t. SkPictures
>   see how much of the recording logic we can move out of SkPicture
>       hopefully all of it and just access recordDrawRect-like entry points
(via
> a friend relationship)
>   force entirety of public SkPicture interface to be const
>   update SkCanvas::drawPicture to take a const
>   hide all SkPicture-derived classes (only leave factories visible)
>   separate RTree so it is treated like the other SkPicture-derived classes
>   hide the kOptimizeForClippedPlayback_RecordingFlag so only factories can set
> it
>        maybe hide kUsePathBoundsForClip_RecordingFlag at the same time?
>   see about killing SkPicture-derived classes and having factories just create
a
> helper object that is stored and used by the recorder
>   try to move all data transfered between the recorder and playback into
> SkPicture
>   split SkPicturePlayback into its own object (not publicly visible)
>   SkPicture::draw then becomes instantiating a new SkPicturePlayback object
and
> letting it iterate over the picture data
>   Update the SkGPUDevice's optimized drawPicture call to use a derived
> SkPicturePlayback object to skip over saveLayers that have already been
rendered
> (replacing them with a drawBitmap)
>   
>   On a related note, right now SkGpuDevice's optimize call is forced to
playback
> the SkPicture to extract the saveLayer range information. At some point I
would
> like to get the picture to a state where I can just iterate over the
operations
> (probably via a minimal SkPicturePlayback-derived object) and just note the
> offsets of the saveLayers and matching restores.
>   
> For the specific questions:
> 
>   I will add the guard in this CL
>   The factories are for the BBH classes but (unless they go away entirely) we
> will probably need some form of factory passed in to the recorder (e.g., a
> factory to create the BBH object itself rather then an SkPicture-derived
object)
>   The BBH factories don't complicate things too much and I think they will
> become easier to sort (modulo the distant possibility of deleting them
entirely)
> once they are hidden
>   I will make SkPictureRecorder non-ref-counted (don't know what I was
thinking)
>   The idea is to change SkPicture's API
>       this change clarifies when the product of the recording process is
> available
>   I don't think we should wait - I think this answers a separate need from
> recording speed and seems like it will make any future changes we need to make
> easier
>   Every API change in Chrome & Android is a dance and is going to take several
> passes. I think this gets us closer to where we want to be. Landing a full on
> split of SkPicture into 3 components plus a radical alteration of its behavior
> all at once would be somewhat difficult to stage.

We'd better stop now and make a coordinated plan for the future of recorded
canvas commands.  I think we've got the roughly the same vision of the goal but
we're majorly stepping on each others' execution toes now.  I would prefer we do
none of this, start from scratch on a well-factored recording and playback
system, relegating pictures to a feature frozen serialization format.

[robertphillips, 2014-03-28 11:39:34]

I also believe we have a similar vision of where we want to end up. I however
would greatly prefer to do things incrementally and shift SkPicture to where we
want it to be.

[reed1, 2014-03-28 14:19:03]

On 2014/03/28 11:39:34, robertphillips wrote:
> I also believe we have a similar vision of where we want to end up. I however
> would greatly prefer to do things incrementally and shift SkPicture to where
we
> want it to be.

Whiteboard time!

[robertphillips, 2014-04-07 13:44:35]

PTAL, this patch adds a guard and makes SkPictureRecorder non-ref-counted.

[reed1, 2014-04-07 14:49:56]

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:408: SkPictureRecorder(SkPictureFactory* factory =
NULL) {
icky icky factory. why is this here? Is it just for bbox stuff?

[robertphillips, 2014-04-07 15:05:37]

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:408: SkPictureRecorder(SkPictureFactory* factory =
NULL) {
Yes. Ultimately this should change to "SkAccelFactory" though. That way
SkPicture doesn't have to be sub-classed just to get a different acceleration
structure.

[mtklein, 2014-04-07 15:18:11]

Remind me, what's the big picture goal here?  Why is it bad for SkPicture to be
able to record?  If we want an SkPicture that can't record, shouldn't that just
be const SkPicture?

https://codereview.chromium.org/214953003/diff/80001/bench/PicturePlaybackBen...
File bench/PicturePlaybackBench.cpp (right):

https://codereview.chromium.org/214953003/diff/80001/bench/PicturePlaybackBen...
bench/PicturePlaybackBench.cpp:40: SkPictureRecorder recorder;
The more I read these edits, the more I prefer the old code.   Are you sure
SkPictureRecorder really needs to exist?  It's mostly there to let us pass a
SkPictureFactory and then to proxy calls down to SkPicture, right?  Can't the
client code just do that themselves?

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:403: virtual SkPicture* create(int width, int height) =
0;
If you keep factories, it might be less obtrusive to pass this around as an
SkPicture* (*SkPictureFactory)(int w, int h)?  No need to deal with refcounting
then.

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:406: class SK_API SkPictureRecorder : public
SkNoncopyable {
For SkNoncopyable to work, it's got to be private inheritance, no?  : private
SkNoncopyable or : SkNoncopyable?

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:406: class SK_API SkPictureRecorder : public
SkNoncopyable {
I'm still somewhat surprised that SkPictureRecorder isn't an SkCanvas itself. 
What's the extra step between the constructor and beginRecording() get us?

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:423: SkCanvas* beginRecording(int width, int height,
uint32_t recordFlags = 0) {
Can we just pass in the SkPicture to record into?
    SkCanvas* beginRecording(SkPicture*, int w, int h, uint32_t flags = 0);
Then there's no need for SkPictureRecorder to know about these factories at all;
it'll never make its own SkPicture.

[reed1, 2014-04-07 15:28:36]

one meta motivation is to make pictures immutable once they are "snapped" (i.e.
visible to the client who might say "draw" on them).

[mtklein, 2014-04-07 15:30:58]

On 2014/04/07 15:28:36, reed1 wrote:
> one meta motivation is to make pictures immutable once they are "snapped"
(i.e.
> visible to the client who might say "draw" on them).

So, isn't that const SkPicture?

[reed1, 2014-04-07 15:38:22]

On 2014/04/07 15:30:58, mtklein wrote:
> On 2014/04/07 15:28:36, reed1 wrote:
> > one meta motivation is to make pictures immutable once they are "snapped"
> (i.e.
> > visible to the client who might say "draw" on them).
> 
> So, isn't that const SkPicture?

SkPicture pic;
SkCanvas* c = pic.beginRecording(...);
c->draw...
pic.endRecording()

otherCanvas->drawPicture(pic)

pic->beginRecording()
1. now what is the state of the 'pic' that was sent to otherCanvas?
2. if it is changed, that's unfortunate. If its not, then how can the caller
herself get a hold of that previous version?

[mtklein, 2014-04-07 16:06:46]

On 2014/04/07 15:38:22, reed1 wrote:
> On 2014/04/07 15:30:58, mtklein wrote:
> > On 2014/04/07 15:28:36, reed1 wrote:
> > > one meta motivation is to make pictures immutable once they are "snapped"
> > (i.e.
> > > visible to the client who might say "draw" on them).
> > 
> > So, isn't that const SkPicture?
> 
> SkPicture pic;
> SkCanvas* c = pic.beginRecording(...);
> c->draw...
> pic.endRecording()
> 
> otherCanvas->drawPicture(pic)
> 
> pic->beginRecording()
> 1. now what is the state of the 'pic' that was sent to otherCanvas?
> 2. if it is changed, that's unfortunate. If its not, then how can the caller
> herself get a hold of that previous version?

Right.  Though the top-level code can always just not make that second
beginRecording() call, I agree it'd be nice to have a convenient API that didn't
allow it.  I still think there are simpler solutions to be found if we lean more
on const SkPicture:

template <typename Draw, typename Factory=JustCreateANormalSkPicture>
const SkPicture* SkRecordPicture(Draw draw, int w, int h, uint32_t flags = 0,
Factory factory=Factory()) {
    SkPicture* pic = factory(w, h);
    draw(pic->beginRecording(w, h, flags));
    pic->endRecording();
    return pic;
}
....
SkAutoTDelete<const SkPicture> picture(SkRecordPicture(&draw_my_picture, 640,
480));
otherCanvas->drawPicture(*picture);
picture->beginRecording();  // Won't compile.

[robertphillips, 2014-04-07 16:26:55]

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:403: virtual SkPicture* create(int width, int height) =
0;
The TileGrid ctor requires extra parameters.

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:406: class SK_API SkPictureRecorder : public
SkNoncopyable {
If that's true we're doing it wrong all over the place.

[mtklein, 2014-04-07 16:29:43]

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h
File include/core/SkPicture.h (right):

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:403: virtual SkPicture* create(int width, int height) =
0;
On 2014/04/07 16:26:56, robertphillips wrote:
> The TileGrid ctor requires extra parameters.

OK then, pass as a functor?

https://codereview.chromium.org/214953003/diff/80001/include/core/SkPicture.h...
include/core/SkPicture.h:406: class SK_API SkPictureRecorder : public
SkNoncopyable {
On 2014/04/07 16:26:56, robertphillips wrote:
> If that's true we're doing it wrong all over the place.

Gross.

[reed1, 2014-04-07 17:28:48]

On 2014/04/07 16:06:46, mtklein wrote:
> On 2014/04/07 15:38:22, reed1 wrote:
> > On 2014/04/07 15:30:58, mtklein wrote:
> > > On 2014/04/07 15:28:36, reed1 wrote:
> > > > one meta motivation is to make pictures immutable once they are
"snapped"
> > > (i.e.
> > > > visible to the client who might say "draw" on them).
> > > 
> > > So, isn't that const SkPicture?
> > 
> > SkPicture pic;
> > SkCanvas* c = pic.beginRecording(...);
> > c->draw...
> > pic.endRecording()
> > 
> > otherCanvas->drawPicture(pic)
> > 
> > pic->beginRecording()
> > 1. now what is the state of the 'pic' that was sent to otherCanvas?
> > 2. if it is changed, that's unfortunate. If its not, then how can the caller
> > herself get a hold of that previous version?
> 
> Right.  Though the top-level code can always just not make that second
> beginRecording() call, I agree it'd be nice to have a convenient API that
didn't
> allow it.  I still think there are simpler solutions to be found if we lean
more
> on const SkPicture:
> 
> template <typename Draw, typename Factory=JustCreateANormalSkPicture>
> const SkPicture* SkRecordPicture(Draw draw, int w, int h, uint32_t flags = 0,
> Factory factory=Factory()) {
>     SkPicture* pic = factory(w, h);
>     draw(pic->beginRecording(w, h, flags));
>     pic->endRecording();
>     return pic;
> }
> ....
> SkAutoTDelete<const SkPicture> picture(SkRecordPicture(&draw_my_picture, 640,
> 480));
> otherCanvas->drawPicture(*picture);
> picture->beginRecording();  // Won't compile.

If the model is that you can build a thing, and then once its built you can pass
the answer around constly, we should try to create an API/class system that most
naturally reflects that.

Do you think there are a lot of call-sites we need to change?

[mtklein, 2014-04-07 17:47:56]

On 2014/04/07 17:28:48, reed1 wrote:
> On 2014/04/07 16:06:46, mtklein wrote:
> > On 2014/04/07 15:38:22, reed1 wrote:
> > > On 2014/04/07 15:30:58, mtklein wrote:
> > > > On 2014/04/07 15:28:36, reed1 wrote:
> > > > > one meta motivation is to make pictures immutable once they are
> "snapped"
> > > > (i.e.
> > > > > visible to the client who might say "draw" on them).
> > > > 
> > > > So, isn't that const SkPicture?
> > > 
> > > SkPicture pic;
> > > SkCanvas* c = pic.beginRecording(...);
> > > c->draw...
> > > pic.endRecording()
> > > 
> > > otherCanvas->drawPicture(pic)
> > > 
> > > pic->beginRecording()
> > > 1. now what is the state of the 'pic' that was sent to otherCanvas?
> > > 2. if it is changed, that's unfortunate. If its not, then how can the
caller
> > > herself get a hold of that previous version?
> > 
> > Right.  Though the top-level code can always just not make that second
> > beginRecording() call, I agree it'd be nice to have a convenient API that
> didn't
> > allow it.  I still think there are simpler solutions to be found if we lean
> more
> > on const SkPicture:
> > 
> > template <typename Draw, typename Factory=JustCreateANormalSkPicture>
> > const SkPicture* SkRecordPicture(Draw draw, int w, int h, uint32_t flags =
0,
> > Factory factory=Factory()) {
> >     SkPicture* pic = factory(w, h);
> >     draw(pic->beginRecording(w, h, flags));
> >     pic->endRecording();
> >     return pic;
> > }
> > ....
> > SkAutoTDelete<const SkPicture> picture(SkRecordPicture(&draw_my_picture,
640,
> > 480));
> > otherCanvas->drawPicture(*picture);
> > picture->beginRecording();  // Won't compile.
> 
> If the model is that you can build a thing, and then once its built you can
pass
> the answer around constly, we should try to create an API/class system that
most
> naturally reflects that.
> 
> Do you think there are a lot of call-sites we need to change?

I'd hesitate to try to make SkPicture immutable.  We really want to be able to
fiddle around inside it more and more lately, and it could be awkward to shove
all that into a single record stage.  If you're asking me the question about
call sites we need to change, I'd say, don't change anything.  If there's a
piece of code that's tripped us up before where we've modified an SkPicture
while it's being used by some subroutine, we could spot fix that by giving it
only a way to create const SkPictures.  I just don't think this is a good
constraint to blanket enforce on a whole type.  Immutable types mean the callers
and callees don't have to trust each other, sure, but callees already and will
still have to trust their callers with a lot more than mutability: the lifetime
of the arguments, that the argument is valid, etc.  Passing a const type gives
the caller all the trust we need that the callee can't muck with the argument.

[robertphillips, 2014-04-07 17:57:10]

Let's talk about picture immutability in person next time you're in.

[robertphillips, 2014-04-10 18:14:15]

Okay - this patch fixes the inheritance from noncopyable.

The factory is ref counted since SkPictureRecorder needs to hang on to it.

Please take another look.

[reed1, 2014-04-10 19:34:15]

api lgtm

[mtklein, 2014-04-10 20:00:11]

lgtm

[robertphillips, 2014-04-11 13:30:18]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-04-11 13:30:25]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/214953003/1...

[commit-bot: I haz the power, 2014-04-11 13:30:35]

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

[commit-bot: I haz the power, 2014-04-11 13:30:35]

Failed to apply patch for gyp/skia_for_chromium_defines.gypi:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file gyp/skia_for_chromium_defines.gypi
  Hunk #1 FAILED at 17.
  1 out of 1 hunk FAILED -- saving rejects to file
gyp/skia_for_chromium_defines.gypi.rej

Patch:       gyp/skia_for_chromium_defines.gypi
Index: gyp/skia_for_chromium_defines.gypi
===================================================================
--- gyp/skia_for_chromium_defines.gypi	(revision 13963)
+++ gyp/skia_for_chromium_defines.gypi	(working copy)
@@ -17,6 +17,7 @@
       'SK_SUPPORT_LEGACY_GETCLIPTYPE',
       'SK_SUPPORT_LEGACY_GETTOTALCLIP',
       'SK_SUPPORT_LEGACY_GETTOPDEVICE',
+      'SK_SUPPORT_LEGACY_PICTURE_CAN_RECORD',
     ],
   },
 }

[robertphillips, 2014-04-11 18:16:01]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-04-11 18:16:09]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/214953003/1...

[commit-bot: I haz the power, 2014-04-11 18:22:07]

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

[commit-bot: I haz the power, 2014-04-11 18:22:08]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests, BuildTools
http://skia-tree-status.appspot.com/redirect/compile-buildbots/buildstatus?bu...

[robertphillips, 2014-04-11 18:53:35]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-04-11 18:53:43]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/robertphillips@google.com/214953003/1...

[robertphillips, 2014-04-13 20:01:17]

The CQ bit was unchecked by robertphillips@google.com

[robertphillips, 2014-04-13 20:01:46]

committed as r14171

[bungeman-chromium, 2014-04-15 21:07:07]

SampleApp crashed on me, looks like it's this.

https://codereview.chromium.org/214953003/diff/140001/samplecode/SampleTiling...
File samplecode/SampleTiling.cpp (right):

https://codereview.chromium.org/214953003/diff/140001/samplecode/SampleTiling...
samplecode/SampleTiling.cpp:108: if (fTextPicture->width() == 0) {
fTextPicture is still uninitialized here!