wip for drawables

wip for drawables

Idea:

1. in its mutable recording state, keep a table of drawables on the side, and store an index in the record list.

2. In "immediate-mode" draw, just call the clients drawable directly (need access to our private list to turn the stored index into a proc)

3. when we "snap", we replace the list of drawables with a list of (sub) pictures, and then during playback of the snapped picture, we invoke a private drawable which just calls "drawPicture" on the index'd subpicture.

Committed: https://skia.googlesource.com/skia/+/6be2aa9a251bf6022570a03140f956655b3ef1da

[reed1, 2014-11-17 16:03:07]

reed@google.com changed reviewers:
+ mtklein@google.com, robertphillips@google.com

[robertphillips, 2014-11-17 20:16:52]

https://codereview.chromium.org/727363003/diff/20001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/727363003/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:59: SkCanvas* canvas,
Why not just pass these in and use them?

https://codereview.chromium.org/727363003/diff/20001/src/gpu/GrRecordReplaceD...
File src/gpu/GrRecordReplaceDraw.cpp (right):

https://codereview.chromium.org/727363003/diff/20001/src/gpu/GrRecordReplaceD...
src/gpu/GrRecordReplaceDraw.cpp:186: 
Here we know we're in the snapped/picture case.

[mtklein, 2014-11-17 20:23:21]

https://codereview.chromium.org/727363003/diff/20001/src/core/SkCanvasDrawable.h
File src/core/SkCanvasDrawable.h (right):

https://codereview.chromium.org/727363003/diff/20001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.h:52: *  return false.
Update docs?

https://codereview.chromium.org/727363003/diff/20001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/727363003/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:59: SkCanvas* canvas,
On 2014/11/17 20:16:51, robertphillips wrote:
> Why not just pass these in and use them?

I assume it's because the don't exist at the time we make the call?

If I'm thinking right, this ultimately amounts to 
  SkASSERT(record.doesn'tContainAnyDrawables());
inside SkRecordPartialDraw.  First one we try to draw will fail on the
SkASSERT(index < count) check, right?

[reed1, 2014-11-17 20:35:59]

https://codereview.chromium.org/727363003/diff/20001/src/core/SkCanvasDrawable.h
File src/core/SkCanvasDrawable.h (right):

https://codereview.chromium.org/727363003/diff/20001/src/core/SkCanvasDrawabl...
src/core/SkCanvasDrawable.h:52: *  return false.
On 2014/11/17 20:23:21, mtklein wrote:
> Update docs?

Done.

https://codereview.chromium.org/727363003/diff/20001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/727363003/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:59: SkCanvas* canvas,
On 2014/11/17 20:23:21, mtklein wrote:
> On 2014/11/17 20:16:51, robertphillips wrote:
> > Why not just pass these in and use them?
> 
> I assume it's because the don't exist at the time we make the call?
> 
> If I'm thinking right, this ultimately amounts to 
>   SkASSERT(record.doesn'tContainAnyDrawables());
> inside SkRecordPartialDraw.  First one we try to draw will fail on the
> SkASSERT(index < count) check, right?

I will need to update all the call-sites for the new API, and I was just lazy
the first time.

[mtklein, 2014-11-17 20:40:33]

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

https://codereview.chromium.org/727363003/diff/40001/include/core/SkPicture.h...
include/core/SkPicture.h:276: SkAutoTUnref<SkData>          fDrawablePicts;
Let's start with SkTDArray<const SkPicture*>, passed by value?

[reed1, 2014-11-17 21:27:44]

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

https://codereview.chromium.org/727363003/diff/40001/include/core/SkPicture.h...
include/core/SkPicture.h:276: SkAutoTUnref<SkData>          fDrawablePicts;
On 2014/11/17 20:40:33, mtklein wrote:
> Let's start with SkTDArray<const SkPicture*>, passed by value?

as we discussed, pass by value is complicated, since the contents would need to
be refAll() each time. Trying the ReleaseProc trick for SkData for now, which
simplifies the current CL (removes code from ~SkPicture()

[reed1, 2014-11-17 21:27:55]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-17 21:28:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727363003/80001

[commit-bot: I haz the power, 2014-11-17 21:28:59]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-11-18 03:28 UTC

[commit-bot: I haz the power, 2014-11-17 21:30:52]

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

[commit-bot: I haz the power, 2014-11-17 21:30:54]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[reed1, 2014-11-17 21:40:28]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-17 21:40:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727363003/20002

[commit-bot: I haz the power, 2014-11-17 21:40:43]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-11-18 03:40 UTC

[reed1, 2014-11-17 21:41:15]

The CQ bit was unchecked by reed@google.com

[mtklein, 2014-11-18 01:07:06]

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

https://codereview.chromium.org/727363003/diff/20002/include/core/SkPicture.h...
include/core/SkPicture.h:263: SkPicture(SkScalar width, SkScalar height,
SkRecord*, SkData* drawablePicts,
I'm still weirded out by passing this as SkData, but this is all hidden away
enough I don't mind landing this and seeing what I come up with as a follow up.

https://codereview.chromium.org/727363003/diff/20002/include/core/SkPicture.h...
include/core/SkPicture.h:268: SkPicture* const drawablePics[], int
drawableCount);
What type are we after here? 
  1) Array of const pointers to SkPicture, or 
  2) array of pointers to const SkPicture?

I think 1) is what's written, 2) is const SkPicture* drawablePics[].

Seems like we want to make sure we're always handling them as const SkPictures?

[reed1, 2014-11-18 18:58:22]

On 2014/11/18 01:07:06, mtklein wrote:
> https://codereview.chromium.org/727363003/diff/20002/include/core/SkPicture.h
> File include/core/SkPicture.h (right):
> 
>
https://codereview.chromium.org/727363003/diff/20002/include/core/SkPicture.h...
> include/core/SkPicture.h:263: SkPicture(SkScalar width, SkScalar height,
> SkRecord*, SkData* drawablePicts,
> I'm still weirded out by passing this as SkData, but this is all hidden away
> enough I don't mind landing this and seeing what I come up with as a follow
up.
> 
>
https://codereview.chromium.org/727363003/diff/20002/include/core/SkPicture.h...
> include/core/SkPicture.h:268: SkPicture* const drawablePics[], int
> drawableCount);
> What type are we after here? 
>   1) Array of const pointers to SkPicture, or 
>   2) array of pointers to const SkPicture?
> 
> I think 1) is what's written, 2) is const SkPicture* drawablePics[].
> 
> Seems like we want to make sure we're always handling them as const
SkPictures?

I was after a const array of picture*, which is what is was I think. I'm
updating a change to make it a const array of const pointers.

[reed1, 2014-11-18 18:58:40]

The CQ bit was checked by reed@google.com

[reed1, 2014-11-18 18:58:47]

PTAL

[commit-bot: I haz the power, 2014-11-18 18:59:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727363003/110001

[commit-bot: I haz the power, 2014-11-18 18:59:36]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-11-19 00:59 UTC

[mtklein, 2014-11-18 19:01:08]

On 2014/11/18 18:59:36, I haz the power (commit-bot) wrote:
> Note for Reviewers:
> The CQ is waiting for an approval.
> If you believe that the CL is not ready yet, or if you would like to L-G-T-M
> with comments then please uncheck the CQ checkbox.
> 
> Waiting for LGTM from valid reviewer(s) till 2014-11-19 00:59 UTC

lgtm

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

Committed patchset #7 (id:110001) as
https://skia.googlesource.com/skia/+/6be2aa9a251bf6022570a03140f956655b3ef1da