Port suitableForGpuRasterization to SkRecord

Move the code over using the same template type approach previously used for willPlayBackBitmaps in http://skbug.com/2702.

Also unifies that flag and this one into a struct so they and others can be computed together. The struct is stored const to enforce lifetime expectations. Adds a few new cases to the unit test.

BUG=skia:2700
R=mtklein@google.com,reed@google.com,robertphillips@google.com

Committed: https://skia.googlesource.com/skia/+/60c2a79cfa8ceebcbafc243407564dc71f5e3b4f

Committed: https://skia.googlesource.com/skia/+/3a0f27916712bb3226874aeaa268e30f565880de

[tomhudson, 2014-07-18 16:06:26]

Parking this while I'm on vacation; it's just the boilerplate, not the guts.

[tomhudson, 2014-08-18 15:51:45]

Still want to cleanup the implementation a little bit, but this is a working
prototype with unit test.

reed@: Current API change is only removing a bool from SkPicture, migrating it
into implementation. Worst case is we replace that const bool with a const
struct of bools.

[reed1, 2014-08-18 16:01:12]

api -- lgtm

[mtklein, 2014-08-18 16:34:58]

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

https://codereview.chromium.org/364823009/diff/60001/src/core/SkRecord.h#newc...
src/core/SkRecord.h:122: SkRecordAnalysis fAccelerationInfo;
Let's keep this parallel to SkRecord like fWillPlaybackBitmaps used to be?  I'm
suspicious of having a public member, especially one that's not actually used by
any code in SkRecord.

[tomhudson, 2014-08-18 16:58:47]

There are suitableForGpuRasterization() checks in PictureTest; I suppose if we
move this structure off of SkRecord then the tests can be migrated there.

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

https://codereview.chromium.org/364823009/diff/60001/src/core/SkRecord.h#newc...
src/core/SkRecord.h:122: SkRecordAnalysis fAccelerationInfo;
On 2014/08/18 16:34:58, mtklein wrote:
> Let's keep this parallel to SkRecord like fWillPlaybackBitmaps used to be? 
I'm
> suspicious of having a public member, especially one that's not actually used
by
> any code in SkRecord.

(This was intended to be a private member with accessors; that's part of the
cleanup noted.)

Ideally this struct would be a const member of SkPicture, but as I tried to
explain last week I was reluctant to add yet another header to include/core/.
I'll throw up a version of that so you can see what it looks like.

[tomhudson, 2014-08-18 17:32:28]

Now appearing as struct SkPicture::Analysis.

At this point I'd like to get rid of SkRecordAnalysis.{cpp,h} and put that code
in an anonymous namespace inside SkPicture.cpp.

If this is the approach we want to take, the new unit tests should probably
migrate from RecordTest.cpp to PictureTest.cpp.

[reed1, 2014-08-18 18:38:13]

not lgtm (yet) -- why is the new type public?

[tomhudson, 2014-08-18 18:41:26]

On 2014/08/18 18:38:13, reed1 wrote:
> not lgtm (yet) -- why is the new type public?

Heh, OtherMike and I talked about that - I think you might like patch set 7
better? SkPicture::Analysis is private.

The *only* downside I've found so far is that unit tests are wordier.

[reed1, 2014-08-18 18:44:04]

privateer lgtm

https://codereview.chromium.org/364823009/diff/120001/tests/RecordTest.cpp
File tests/RecordTest.cpp (right):

https://codereview.chromium.org/364823009/diff/120001/tests/RecordTest.cpp#ne...
tests/RecordTest.cpp:11: #include "SkDashPathEffect.h"
can remove these adds...

[tomhudson, 2014-08-18 19:01:20]

Mike K: your plan was cunning indeed. Good to go?

Rob: I think your scope here is that I added new test coverage for
suitableForGpuRasterization() - which has a TODO that the heuristic needs
tuning.

[mtklein, 2014-08-18 19:22:14]

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

https://codereview.chromium.org/364823009/diff/140001/include/core/SkPicture....
include/core/SkPicture.h:306: };
if you're feeling super snazzy

} const fAnalysis;

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:66: static const SkPaint* AsPtr(const SkPaint& p) {
return &p; }
Might promote these guys out of the structs, now that there are two copies?

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:136: bool operator()(const T& r) {
void's okay too.  Just use record.visit<void>(...)

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:136: bool operator()(const T& r) {
I'd structure this a little differently:

void checkPaint(const SkPaint* paint) { ... }

void operator()(const DrawPoints& op) {
  this->checkPaint(&op.paint);
  // ... whatever you do for DrawPoints specially
}

void operator()(const DrawPath& op) {
  this->checkPaint(&op.paint);
  // ... whatever you do for DrawPath specially
}

template <typename T>
void operator()(const T& op) {
  this->checkOther(op);
}

template <typename T>
SK_WHEN(HasMember_paint<T>, void) checkOther(const T& op) {
  this->checkPaint(AsPtr(op.paint));
}

template <typename T>
SK_WHEN(!HasMember_paint<T>, void) checkOther(const T&) { /* do nothing */}

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:193: const char** reason = NULL,
Funky indent?

[tomhudson, 2014-08-18 21:37:00]

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

https://codereview.chromium.org/364823009/diff/140001/include/core/SkPicture....
include/core/SkPicture.h:306: };
On 2014/08/18 19:22:14, mtklein wrote:
> if you're feeling super snazzy
> 
> } const fAnalysis;

Done.

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:66: static const SkPaint* AsPtr(const SkPaint& p) {
return &p; }
On 2014/08/18 19:22:14, mtklein wrote:
> Might promote these guys out of the structs, now that there are two copies?

Done.

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:136: bool operator()(const T& r) {
On 2014/08/18 19:22:14, mtklein wrote:
> void's okay too.  Just use record.visit<void>(...)

Done.

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:136: bool operator()(const T& r) {
On 2014/08/18 19:22:14, mtklein wrote:
> I'd structure this a little differently:
> 
> void checkPaint(const SkPaint* paint) { ... }
> 
> void operator()(const DrawPoints& op) {
>   this->checkPaint(&op.paint);
>   // ... whatever you do for DrawPoints specially
> }
> 
> void operator()(const DrawPath& op) {
>   this->checkPaint(&op.paint);
>   // ... whatever you do for DrawPath specially
> }
> 
> template <typename T>
> void operator()(const T& op) {
>   this->checkOther(op);
> }
> 
> template <typename T>
> SK_WHEN(HasMember_paint<T>, void) checkOther(const T& op) {
>   this->checkPaint(AsPtr(op.paint));
> }
> 
> template <typename T>
> SK_WHEN(!HasMember_paint<T>, void) checkOther(const T&) { /* do nothing */}

How about this, which gets rid of the indirection through checkOther()? I like
having one layer of template specialization rather than two nested layers, seems
easier to reason about whether or not it's a simpler compiled construct.

https://codereview.chromium.org/364823009/diff/140001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:193: const char** reason = NULL,
On 2014/08/18 19:22:14, mtklein wrote:
> Funky indent?

Done.

[mtklein, 2014-08-18 21:50:55]

lgtm

https://codereview.chromium.org/364823009/diff/180001/src/core/SkPicture.cpp
File src/core/SkPicture.cpp (right):

https://codereview.chromium.org/364823009/diff/180001/src/core/SkPicture.cpp#...
src/core/SkPicture.cpp:139: if (effect) {
I like to scope these when possible:

if (const SkPathEffect* effect = op.paint.getPathEffect()) {
  ...
}

YMMV

[tomhudson, 2014-08-18 22:00:01]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-08-18 22:00:40]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/364823009/220001

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

Committed patchset #12 (220001) as 60c2a79cfa8ceebcbafc243407564dc71f5e3b4f

[mtklein, 2014-08-18 22:30:40]

A revert of this CL (patchset #12) has been created in
https://codereview.chromium.org/470633003/ by mtklein@google.com.

The reason for reverting is: test_gpu_veto(reporter, true) failing on many bots.

[tomhudson, 2014-08-18 23:08:35]

> The reason for reverting is: test_gpu_veto(reporter, true) failing on many
bots.

Sigh - I'd been running the wrong set of unit tests. This version caches the
reason returned when suitableForGpuRasterization() is false.

However, something that doesn't seem to be hit in the unit tests is
reconsidering suitability given different multisampling sample counts. If we're
going to support that, this approach of const caching the result value is
insufficient; we'll also have to have a route to recomputing it for non-0 sample
counts, which in turn suggests cacheing all the intermediate data that was
formerly on SkPictureContentInfo.

[mtklein, 2014-08-18 23:24:24]

> However, something that doesn't seem to be hit in the unit tests is
> reconsidering suitability given different multisampling sample counts. If
we're
> going to support that, this approach of const caching the result value is
> insufficient; we'll also have to have a route to recomputing it for non-0
sample
> counts, which in turn suggests cacheing all the intermediate data that was
> formerly on SkPictureContentInfo.

Storing the intermediate counts (4 ints right, something like that?) sounds just
fine to me.
I'd check with Robert, but you may also be able to assume sample count is in {
0, 4, 16 } and store each bool proactively.

[robertphillips, 2014-08-19 12:22:09]

Probably MSAA/non-MSAA would be sufficient. When MSAA is available it is
tempting to say we'll always just return true but it, unfortunately, probably
isn't going to be that easy.

[robertphillips, 2014-08-19 12:28:00]

test changes lgtm 

We usually also test out suitableForGpuRasterization changes by generating the
confusion matrix of the local set of skps using the gpuveto tool.

https://codereview.chromium.org/364823009/diff/240001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/364823009/diff/240001/tests/PictureTest.cpp#n...
tests/PictureTest.cpp:746: #if SK_SUPPORT_GPU
Can we just define this once and move the first #undef further down the file ?

https://codereview.chromium.org/364823009/diff/240001/tests/PictureTest.cpp#n...
tests/PictureTest.cpp:750: 
All on 1 line ?

[tomhudson, 2014-08-19 20:19:27]

Patch set 14 supports MSAA, more-or-less. I think the intended behavior is that
we query the GrContext
for its multisample count and pass that through to our decision function.

It's a bit uglier than patch set 13, and Chrome doesn't seem to be using that
entry point; the GrContext
is always passed as NULL. +ErnstM@ to see whether they will reasonably soon.


On 2014/08/19 12:28:00, robertphillips wrote:
> We usually also test out suitableForGpuRasterization changes by generating the
> confusion matrix of the local set of skps using the gpuveto tool.

Skia bug filed.

> https://codereview.chromium.org/364823009/diff/240001/tests/PictureTest.cpp
> File tests/PictureTest.cpp (right):
> 
>
https://codereview.chromium.org/364823009/diff/240001/tests/PictureTest.cpp#n...
> tests/PictureTest.cpp:746: #if SK_SUPPORT_GPU
> Can we just define this once and move the first #undef further down the file ?

Done.

>
https://codereview.chromium.org/364823009/diff/240001/tests/PictureTest.cpp#n...
> tests/PictureTest.cpp:750: 
> All on 1 line ?

As 1 line is 129 characters.

[mtklein, 2014-08-19 21:01:01]

Still LGTM

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

https://codereview.chromium.org/364823009/diff/280001/include/core/SkPicture....
include/core/SkPicture.h:310: Analysis()
Maybe merge these false and 0s into init()?  Either way seems clear to me.

https://codereview.chromium.org/364823009/diff/280001/include/core/SkPicture....
include/core/SkPicture.h:328: } const                       fAnalysis;
The indented fAnalysis perhaps has lost its meaning so far removed from its
friends?

[tomhudson, 2014-08-20 12:23:06]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-08-20 12:23:26]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/364823009/320001

[commit-bot: I haz the power, 2014-08-20 12:29:46]

Committed patchset #17 (320001) as 3a0f27916712bb3226874aeaa268e30f565880de