Remove SkPicture::kUsePathBoundsForClip_RecordingFlag

Remove SkPicture::kUsePathBoundsForClip_RecordingFlag

The real question is whether we ever want to record a picture without using the path bounds for a conservative (but faster) clip answer?

Committed: https://skia.googlesource.com/skia/+/9f1c241e0d8a756fca1ec2dacb565eec83166d5f

[mtklein, 2014-06-05 13:51:28]

lgtm.  In my experience, you have to be insane not to use kUsePathBoundsForClip.
 It's much slower without.

[robertphillips, 2014-06-05 13:57:06]

Adding Derek & Leon. I think the answer for both Skia & Chrome is that we always
want to use the path bound. The main question is will Android be okay with this
change?

[reed1, 2014-06-05 14:06:24]

non-android lgtm

[reed1, 2014-06-05 14:07:02]

On 2014/06/05 14:06:24, reed1 wrote:
> non-android lgtm

... I wonder. When we have lazy clip evaluation, perhaps this won't be an issue.

[scroggo, 2014-06-05 17:58:16]

lgtm

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

https://codereview.chromium.org/316143003/diff/1/include/core/SkPicture.h#new...
include/core/SkPicture.h:135: #ifdef SK_SUPPORT_LEGACY_RECORDING_FLAG
Android never uses kUsePathBoundsForClip_RecordingFlag, so removing it is fine
on their end.

https://codereview.chromium.org/316143003/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/316143003/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:866:
this->updateClipConservativelyUsingBounds(rrect.getBounds(), op, false);
So if I understand this correctly, the playback will still clip to the RRect on
draw thanks to recordClipRRect above, but this will use the bounds for quick
reject, so it may *not* reject some things that it could have, in order to go
faster?

If that is an overall win for Chrome, I don't see how it could be harmful to
Android.

[robertphillips, 2014-06-05 18:40:24]

https://codereview.chromium.org/316143003/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/316143003/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:866:
this->updateClipConservativelyUsingBounds(rrect.getBounds(), op, false);
Right - it records the original clip and then updates the raster clip using the
bounding box of the path rather then the path itself. The clip is updated
conservatively (based on the operation) to create a clip that is qualitatively
the same.

From Android's perspective it does alter the contract of all the Canvas::clip*
methods in that their return value will be more conservative (e.g., they may
actually report the clip is open even though a more careful accounting would
label it closed). Additionally the contracts of the Canvas getClipBounds and
quickReject entry points would be changed (i.e., their answers would be more
conservative).

[scroggo, 2014-06-05 19:07:39]

On 2014/06/05 18:40:24, robertphillips wrote:
> From Android's perspective it does alter the contract of all the Canvas::clip*
> methods in that their return value will be more conservative (e.g., they may
> actually report the clip is open even though a more careful accounting would
> label it closed). Additionally the contracts of the Canvas getClipBounds and
> quickReject entry points would be changed (i.e., their answers would be more
> conservative).

Arg, right! :-/ I think that's a reasonable change, but it's probably worth
discussing with Android. mtklein@, do you have numbers I can show the Android
team?

In the meantime, if you want to get this in, it seems like it would be easy to
add SK_LEGACY_CLIP_NONCONSERVATIVELY.

[mtklein, 2014-06-05 19:39:03]

On 2014/06/05 19:07:39, scroggo wrote:
> On 2014/06/05 18:40:24, robertphillips wrote:
> > From Android's perspective it does alter the contract of all the
Canvas::clip*
> > methods in that their return value will be more conservative (e.g., they may
> > actually report the clip is open even though a more careful accounting would
> > label it closed). Additionally the contracts of the Canvas getClipBounds and
> > quickReject entry points would be changed (i.e., their answers would be more
> > conservative).
> 
> Arg, right! :-/ I think that's a reasonable change, but it's probably worth
> discussing with Android. mtklein@, do you have numbers I can show the Android
> team?
> 
> In the meantime, if you want to get this in, it seems like it would be easy to
> add SK_LEGACY_CLIP_NONCONSERVATIVELY.

Just did a one-off of recording the .skps I had laying around with and without
this flag.

tl;dr: using the bounds instead of real clip is a ~60% recording time
improvement, sometimes more like ~90%.
https://docs.google.com/spreadsheets/d/1ObTyiJSOADyi9tuyig4K-CzfGVkoXQZq0ovNi...

[scroggo, 2014-06-06 15:13:06]

On 2014/06/05 19:39:03, mtklein wrote:
> On 2014/06/05 19:07:39, scroggo wrote:
> > On 2014/06/05 18:40:24, robertphillips wrote:
> > > From Android's perspective it does alter the contract of all the
> Canvas::clip*
> > > methods in that their return value will be more conservative (e.g., they
may
> > > actually report the clip is open even though a more careful accounting
would
> > > label it closed). Additionally the contracts of the Canvas getClipBounds
and
> > > quickReject entry points would be changed (i.e., their answers would be
more
> > > conservative).
> > 
> > Arg, right! :-/ I think that's a reasonable change, but it's probably worth
> > discussing with Android. mtklein@, do you have numbers I can show the
Android
> > team?
> > 
> > In the meantime, if you want to get this in, it seems like it would be easy
to
> > add SK_LEGACY_CLIP_NONCONSERVATIVELY.
> 
> Just did a one-off of recording the .skps I had laying around with and without
> this flag.
> 
> tl;dr: using the bounds instead of real clip is a ~60% recording time
> improvement, sometimes more like ~90%.
>
https://docs.google.com/spreadsheets/d/1ObTyiJSOADyi9tuyig4K-CzfGVkoXQZq0ovNi...

Android is on board. Feel free to submit!

[robertphillips, 2014-06-06 18:19:01]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-06 18:19:14]

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

[commit-bot: I haz the power, 2014-06-06 18:19:17]

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

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

Failed to apply patch for gyp/skia_for_chromium_defines.gypi:
While running git apply --index -p1;
  error: patch failed: gyp/skia_for_chromium_defines.gypi:19
  error: gyp/skia_for_chromium_defines.gypi: patch does not apply

Patch:       gyp/skia_for_chromium_defines.gypi
Index: gyp/skia_for_chromium_defines.gypi
diff --git a/gyp/skia_for_chromium_defines.gypi
b/gyp/skia_for_chromium_defines.gypi
index
99141a2a36736160943fa13065e14e8093f9bec4..5cb004cb753c70233e77d4978f9fd9afd0876805
100644
--- a/gyp/skia_for_chromium_defines.gypi
+++ b/gyp/skia_for_chromium_defines.gypi
@@ -19,6 +19,7 @@
       'SK_IGNORE_ETC1_SUPPORT',
       'SK_SUPPORT_LEGACY_INSTALLPIXELSPARAMS',
       'SK_SUPPORT_LEGACY_DRAWPICTURE_API',
+      'SK_SUPPORT_LEGACY_RECORDING_FLAG',
     ],
   },
 }

[robertphillips, 2014-06-08 20:04:30]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-08 20:05:33]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/316143003/2...

[robertphillips, 2014-06-08 23:05:33]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-08 23:05:40]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/316143003/4...

[commit-bot: I haz the power, 2014-06-08 23:25:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.chromium
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[commit-bot: I haz the power, 2014-06-08 23:44:10]

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

[commit-bot: I haz the power, 2014-06-08 23:44:11]

Try jobs failed on following builders:
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.chromium
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[robertphillips, 2014-06-09 12:57:54]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-06-09 12:59:01]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/robertphillips@google.com/316143003/6...

[commit-bot: I haz the power, 2014-06-09 13:25:39]

Change committed as 9f1c241e0d8a756fca1ec2dacb565eec83166d5f