option to return drawable from recording

option to return drawable from recording

patch from issue 747033005 at patchset 80001 (http://crrev.com/747033005#ps80001)

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/1bdfd3f4f09e47364f76d3f08177b1ce844ac786

[reed1, 2014-11-24 13:40:05]

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

[reed1, 2014-11-24 15:11:37]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 15:12:33]

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

[commit-bot: I haz the power, 2014-11-24 15:12:34]

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-24 21:12 UTC

[reed1, 2014-11-24 15:15:55]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-11-24 15:28:03]

sorry for the follow-the-bouncing-CL. I believe this one is ready for review.

[robertphillips, 2014-11-24 15:42:02]

https://codereview.chromium.org/732653004/diff/20001/include/core/SkPictureRe...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/732653004/diff/20001/include/core/SkPictureRe...
include/core/SkPictureRecorder.h:75: 
// Is it too early for a comment ?

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp
File samplecode/SampleArc.cpp (right):

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:116: 
DrawRectWithLines ?

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:124: 
DrawLabel ?

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:140: 
DrawArcs ?

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:183: 
drawRoot due to fRect usage ?

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:196: 
SK_OVERRIDE ?

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:110: protected:
one line ?

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:127: if (fDrawableList) {
Are we going to plumb down the BBH and SVI preferences here ?

[reed1, 2014-11-24 17:10:48]

https://codereview.chromium.org/732653004/diff/20001/include/core/SkPictureRe...
File include/core/SkPictureRecorder.h (right):

https://codereview.chromium.org/732653004/diff/20001/include/core/SkPictureRe...
include/core/SkPictureRecorder.h:75: 
On 2014/11/24 15:42:02, robertphillips wrote:
> // Is it too early for a comment ?

Done.

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp
File samplecode/SampleArc.cpp (right):

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:116: 
On 2014/11/24 15:42:02, robertphillips wrote:
> DrawRectWithLines ?

Done.

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:124: 
On 2014/11/24 15:42:02, robertphillips wrote:
> DrawLabel ?

Done.

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:140: 
On 2014/11/24 15:42:02, robertphillips wrote:
> DrawArcs ?

Done.

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:183: 
On 2014/11/24 15:42:02, robertphillips wrote:
> drawRoot due to fRect usage ?

Done.

https://codereview.chromium.org/732653004/diff/20001/samplecode/SampleArc.cpp...
samplecode/SampleArc.cpp:196: 
On 2014/11/24 15:42:02, robertphillips wrote:
> SK_OVERRIDE ?

Done.

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:110: protected:
On 2014/11/24 15:42:02, robertphillips wrote:
> one line ?

Done.

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:127: if (fDrawableList) {
On 2014/11/24 15:42:02, robertphillips wrote:
> Are we going to plumb down the BBH and SVI preferences here ?

No idea -- will leave as a TODO for now.

[reed1, 2014-11-24 17:10:51]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 17:11:12]

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

[commit-bot: I haz the power, 2014-11-24 17:11:13]

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-24 23:11 UTC

[robertphillips, 2014-11-24 17:26:10]

This l g t m - would anyone else like to chime in?

https://codereview.chromium.org/732653004/diff/40001/src/core/SkPictureRecord...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/732653004/diff/40001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:42: SkPicture*
SkPictureRecorder::endRecordingAsPicture() {
Might as well remove this dead code.

[mtklein, 2014-11-24 17:29:41]

Think it's time to start pillaging src/animator for names, e.g. SkDrawable?

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:127: if (fDrawableList) {
On 2014/11/24 15:42:02, robertphillips wrote:
> Are we going to plumb down the BBH and SVI preferences here ?

Seems like if we're going to plumb BBHs further, we'll need to either hang on to
the BBH factory, or add something like
  SkBBH* newOfSameType() const = 0;
to the SkBBH interface.

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:140: // bounds in 'fBBH'
fBBH doesn't necessarily contain those bounds any more, nor necessarily have an
efficient way of mapping opID to bounds, only bounds to opIDs.

Sounds like we want to explicitly fill a bounds array, then use that to fill in
the BBH and to help the SaveLayer computation?  This is what happens anyway...
just need to reorganize the interfaces.

There's a small wrinkle that BBHs as written optionally can take ownership of
their input bounds array.  None do, so we can drop that if it makes things
easier.

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

https://codereview.chromium.org/732653004/diff/20001/src/core/SkRecord.h#newc...
src/core/SkRecord.h:243: SK_COMPILE_ASSERT(sizeof(SkRecord) <= 56,
SkRecordSize);
4 dead bytes. :(

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

https://codereview.chromium.org/732653004/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:140:
fCanvas->EXPERIMENTAL_drawDrawable(fDrawables[r.index]);
This is to support immediate mode drawing right?  There's sort of enough going
on here already that I think supporting immediate mode might be better left to
another CL.  In particular, the constraint on passing only one of drawables or
drawable picts is pretty tricky.

[reed1, 2014-11-24 18:29:09]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-11-24 19:12:08]

PTAL #4

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:127: if (fDrawableList) {
On 2014/11/24 17:29:41, mtklein wrote:
> On 2014/11/24 15:42:02, robertphillips wrote:
> > Are we going to plumb down the BBH and SVI preferences here ?
> 
> Seems like if we're going to plumb BBHs further, we'll need to either hang on
to
> the BBH factory, or add something like
>   SkBBH* newOfSameType() const = 0;
> to the SkBBH interface.

Agreed, though in the lastest patch I added a TODO to carry out that
investigation.

https://codereview.chromium.org/732653004/diff/20001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:140: // bounds in 'fBBH'
On 2014/11/24 17:29:41, mtklein wrote:
> fBBH doesn't necessarily contain those bounds any more, nor necessarily have
an
> efficient way of mapping opID to bounds, only bounds to opIDs.
> 
> Sounds like we want to explicitly fill a bounds array, then use that to fill
in
> the BBH and to help the SaveLayer computation?  This is what happens anyway...
> just need to reorganize the interfaces.
> 
> There's a small wrinkle that BBHs as written optionally can take ownership of
> their input bounds array.  None do, so we can drop that if it makes things
> easier.

Is this something that should be figured out now, or can it be done after this
CL lands?

https://codereview.chromium.org/732653004/diff/40001/src/core/SkPictureRecord...
File src/core/SkPictureRecorder.cpp (right):

https://codereview.chromium.org/732653004/diff/40001/src/core/SkPictureRecord...
src/core/SkPictureRecorder.cpp:42: SkPicture*
SkPictureRecorder::endRecordingAsPicture() {
On 2014/11/24 17:26:10, robertphillips wrote:
> Might as well remove this dead code.

Done.

[mtklein, 2014-11-24 19:36:53]

lgtm

[reed1, 2014-11-24 20:00:06]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 20:00:54]

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

[reed1, 2014-11-24 20:04:41]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 20:04:44]

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

[commit-bot: I haz the power, 2014-11-24 20:15:25]

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

[commit-bot: I haz the power, 2014-11-24 20:15:26]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[reed1, 2014-11-24 20:20:43]

On 2014/11/24 20:15:26, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
>
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

Will time recording perf before landing...

[mtklein, 2014-11-24 20:26:32]

On 2014/11/24 20:20:43, reed1 wrote:
> On 2014/11/24 20:15:26, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
> >
>
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)
> 
> Will time recording perf before landing...
      tabl_culturalsolutions.skp	 396us ->  649us	1.6x
             tabl_deviantart.skp	 142us ->  232us	1.6x
                desk_booking.skp	1.16ms -> 1.76ms	1.5x
           tabl_cuteoverload.skp	 541us ->  785us	1.5x
              desk_pinterest.skp	 153us ->  221us	1.4x
          desk_css3gradients.skp	 251us ->  355us	1.4x
               tabl_slashdot.skp	 137us ->  191us	1.4x
               desk_linkedin.skp	 345us ->  477us	1.4x
             tabl_googleblog.skp	 304us ->  418us	1.4x
                tabl_mozilla.skp	2.36ms -> 3.22ms	1.4x
          tabl_androidpolice.skp	 668us ->  911us	1.4x
         tabl_googlecalendar.skp	 210us ->  286us	1.4x
               tabl_frantzen.skp	  58us -> 78.7us	1.4x
            tabl_gamedeksiam.skp	 785us -> 1.06ms	1.4x
            desk_silkfinance.skp	78.1us ->  105us	1.3x
               tabl_engadget.skp	 629us ->  843us	1.3x
                    tabl_mlb.skp	 344us ->  459us	1.3x
                desk_wowwiki.skp	1.62ms -> 2.15ms	1.3x
                 desk_mapsvg.skp	1.59ms ->  2.1ms	1.3x
           desk_yahooanswers.skp	 173us ->  228us	1.3x
              desk_wordpress.skp	 827us -> 1.09ms	1.3x
                 tabl_pravda.skp	 237us ->  311us	1.3x
            desk_pokemonwiki.skp	9.16ms ->   12ms	1.3x
                tabl_nytimes.skp	 130us ->  169us	1.3x
           tabl_transformice.skp	 159us ->  207us	1.3x
                  tabl_gspro.skp	71.7us -> 93.5us	1.3x
                   tabl_digg.skp	 933us ->  1.2ms	1.3x
               desk_tigersvg.skp	 185us ->  237us	1.3x
             desk_chalkboard.skp	 909us -> 1.16ms	1.3x
                tabl_sahadan.skp	95.9us ->  122us	1.3x
                    tabl_cnn.skp	 214us ->  272us	1.3x
             desk_forecastio.skp	95.8us ->  121us	1.3x
               desk_facebook.skp	 582us ->  738us	1.3x
               desk_samoasvg.skp	 741us ->  938us	1.3x
                   desk_ebay.skp	 244us ->  309us	1.3x
                  tabl_ukwsj.skp	 589us ->  743us	1.3x
                  desk_baidu.skp	 223us ->  280us	1.3x
                desk_weather.skp	 287us ->  361us	1.3x
     desk_jsfiddlehumperclip.skp	45.1us -> 56.5us	1.3x
                desk_blogger.skp	 561us ->  702us	1.3x
         desk_jsfiddlebigcar.skp	41.4us -> 51.8us	1.3x
      desk_googlespreadsheet.skp	 559us ->  697us	1.2x
                   tabl_cnet.skp	 143us ->  177us	1.2x
               desk_fontwipe.skp	40.8us -> 50.5us	1.2x
                desk_twitter.skp	 478us ->  589us	1.2x
                   desk_espn.skp	 281us ->  346us	1.2x
                    desk_gws.skp	 194us ->  238us	1.2x
             desk_mobilenews.skp	 505us ->  618us	1.2x
desk_googlespreadsheetdashed.skp	1.67ms -> 2.03ms	1.2x
                 desk_amazon.skp	 121us ->  147us	1.2x
                 desk_sfgate.skp	 578us ->  698us	1.2x
           tabl_worldjournal.skp	 238us ->  287us	1.2x
                 tabl_nofolo.skp	  77us -> 92.1us	1.2x
            desk_gmailthread.skp	 334us ->  398us	1.2x
                  tabl_gmail.skp	  20us -> 23.8us	1.2x
                desk_youtube.skp	 602us ->  713us	1.2x
                   tabl_hsfi.skp	9.89us -> 11.7us	1.2x
               tabl_techmeme.skp	 127us ->  150us	1.2x
             desk_googleplus.skp	 905us -> 1.06ms	1.2x
                 desk_carsvg.skp	 542us ->  625us	1.2x
             desk_googlehome.skp	60.1us -> 68.8us	1.1x

[reed1, 2014-11-24 22:28:33]

with optimized end-recording-as-picture code restored. PTAL

[reed1, 2014-11-24 22:28:45]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 22:29:51]

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

[reed1, 2014-11-24 22:30:22]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-11-24 22:33:11]

mtklein, can you re-compare the perf w/ the old code restored?

[reed1, 2014-11-24 22:37:15]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 22:38:03]

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

[mtklein, 2014-11-24 22:38:35]

On 2014/11/24 22:38:03, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/732653004/100001

Super neutral.

[commit-bot: I haz the power, 2014-11-24 22:41:55]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/1bdfd3f4f09e47364f76d3f08177b1ce844ac786