Expose SkPicture::numSlowPaths() in the public API.

Expose SkPicture::numSlowPaths() in the public API.

BUG=614724
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000423007

[Stephen White, 2016-05-25 15:19:28]

Description was changed from

==========
Expose SkPicture::numSlowPaths() in the public API.

BUG=614724
==========

to

==========
Expose SkPicture::numSlowPaths() in the public API.

BUG=614724
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000423007
==========

[Stephen White, 2016-05-25 15:19:50]

Description was changed from

==========
Expose SkPicture::numSlowPaths() in the public API.

BUG=614724
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000423007
==========

to

==========
Expose SkPicture::numSlowPaths() in the public API.

BUG=614724
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2000423007
==========

[Stephen White, 2016-05-25 15:19:51]

senorblanco@chromium.org changed reviewers:
+ bsalomon@google.com

[Stephen White, 2016-05-25 15:20:17]

Brian: PTAL. Thanks!

Feel free to bikeshed on the name, etc.

[Stephen White, 2016-05-25 15:26:53]

The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 15:27:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000423007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000423007/40001

[commit-bot: I haz the power, 2016-05-25 15:40:45]

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

[commit-bot: I haz the power, 2016-05-25 15:40:45]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 15:40:54]

Dry run: None

[Stephen White, 2016-05-25 20:36:03]

senorblanco@chromium.org changed reviewers:
+ reed@google.com
- bsalomon@google.com

[Stephen White, 2016-05-25 20:36:44]

Mike: PTAL. Thanks!

Feel free to bikeshed on the public name, etc.

[reed1, 2016-05-25 20:40:34]

reed@google.com changed reviewers:
+ bsalomon@google.com, fmalita@chromium.org

[reed1, 2016-05-25 20:40:35]

What is the use-case for this externally, and how does it want to define "slow"?

If this is meant to be part of a predicate for deciding to use ganesh, I think
Florin's recent work is our preferred pattern/direction.

If this is meant to collect status for debugging etc. I think a custom canvas is
more general, and doesn't bake in today's notion of "slow".

[Stephen White, 2016-05-26 00:04:50]

On 2016/05/25 20:40:35, reed1 wrote:
> What is the use-case for this externally, and how does it want to define
"slow"?
> 
> If this is meant to be part of a predicate for deciding to use ganesh, I think
> Florin's recent work is our preferred pattern/direction.
> 
> If this is meant to collect status for debugging etc. I think a custom canvas
is
> more general, and doesn't bake in today's notion of "slow".

The question we're trying to answer is "why did this page get vetoed?". To do
that,
we need to know what elements caused it. Yes, we could use a custom canvas, 
iterate through the elements, and ask SkPictureAnalyzer in turn: analyzePath()
(doesn't
exist yet, but could be added), and then check exactly when the veto gets
tripped.
It just feels like we're replicating logic that already exists in SkPicture. It
would also 
only tell us the last item that triggered it, not how many there are in total.

And given that more stuff is moving out of SkPicture and into display list items
(e.g.,
ClipPathDisplayItem), we already have to replicate this logic a bit for the
items which
aren't in pictures. So it seemed like it would be clearer to move the guts of 
SkPictureAnalyzer into cc, and implement a numSlowCommands() on DisplayListItem.
Then cc could just sum them all. That would also let us tweak the parameters for
MSAA
vs non-MSAA vetoes independently.

[f(malita), 2016-05-26 12:36:00]

On 2016/05/26 00:04:50, Stephen White wrote:

> The question we're trying to answer is "why did this page get vetoed?".

The current feedback mechanism is the optional outparam in
SkPictureGpuAnalyzer::suitableForGpuRasterization(), but yeah - that doesn't
tell you the individual contribution to the veto.


> And given that more stuff is moving out of SkPicture and into display list
items
> (e.g.,
> ClipPathDisplayItem), we already have to replicate this logic a bit for the
> items which
> aren't in pictures. So it seemed like it would be clearer to move the guts of 
> SkPictureAnalyzer into cc, and implement a numSlowCommands() on
DisplayListItem.
> Then cc could just sum them all. That would also let us tweak the parameters
for
> MSAA
> vs non-MSAA vetoes independently.

Assuming we can get numSlowPaths for SKP-backed display items
(DrawingDisplayItems), how do we get the equivalent metric for non-SKP based
items (say ClipPathDisplayItems)?

Would it make more sense to have a SkPictureGpuAnalyzer::numSlowPaths() which
lets us compute individual fragment contributions as we feed everything through
it?

[Stephen White, 2016-05-26 15:11:21]

On 2016/05/26 12:36:00, f(malita) wrote:
> On 2016/05/26 00:04:50, Stephen White wrote:
> 
> > The question we're trying to answer is "why did this page get vetoed?".
> 
> The current feedback mechanism is the optional outparam in
> SkPictureGpuAnalyzer::suitableForGpuRasterization(), but yeah - that doesn't
> tell you the individual contribution to the veto.
> 
> 
> > And given that more stuff is moving out of SkPicture and into display list
> items
> > (e.g.,
> > ClipPathDisplayItem), we already have to replicate this logic a bit for the
> > items which
> > aren't in pictures. So it seemed like it would be clearer to move the guts
of 
> > SkPictureAnalyzer into cc, and implement a numSlowCommands() on
> DisplayListItem.
> > Then cc could just sum them all. That would also let us tweak the parameters
> for
> > MSAA
> > vs non-MSAA vetoes independently.
> 
> Assuming we can get numSlowPaths for SKP-backed display items
> (DrawingDisplayItems), how do we get the equivalent metric for non-SKP based
> items (say ClipPathDisplayItems)?

I was thinking we'd just move the logic into cc (ie., check if the clip path is
concave 
in ClipPathDisplayItem::numSlowCommands() (virtual)), Getting the veto wrong
either way 
isn't a disaster, and Telemetry should tell us if we mess up.

> Would it make more sense to have a SkPictureGpuAnalyzer::numSlowPaths() which
> lets us compute individual fragment contributions as we feed everything
through
> it?

We could do that, but why not just move the SkPictureGpuAnalyzer logic into cc?
It seems so much simpler than going back & forth from cc to Skia, and the logic
is clearer in CC, especially if we want to tweak the thresholds independently:

if (we have MSAA and #slow cmds is > MSAA threshold)
  use MSAA
else if (#slow cmds is > software threshold)
  use software
else
  use non-MSAA

And then I can get the number of slow commands for stats as well.

where as right now it's

if (!isSuitableForGpuRasterization())
  if (have MSAA)
    use MSAA
  else
    use software
else
  use non-MSAA

I suppose if folks really want to keep it in Skia, one way to handle the
fine-grained
veto would be to have SkPictureAnalyzer take a parameter for the number of slow 
commands to veto (this is what I want to tweak). Then it'd be

if (!isSuitableForGpuRasterization(haveMSAA ? MSAA_threshold :
software_threshhold))
  if (have MSAA)
    use MSAA
  else
    use software
else
  use non-MSAA

still seems confusing. If we could come up with a better name, it might be
better, but
still wouldn't solve the stats thing.

Another alternative would be for Skia to capture the stats somehow (and we could
do
even better, such as how many times each path renderer was used). Is there a 
precedent for Skia returning stats to Chrome somehow? Or directly to UMA
histograms?

[f(malita), 2016-05-26 15:55:59]

On 2016/05/26 15:11:21, Stephen White wrote:
> On 2016/05/26 12:36:00, f(malita) wrote:
> > On 2016/05/26 00:04:50, Stephen White wrote:
> > 
> > > The question we're trying to answer is "why did this page get vetoed?".
> > 
> > The current feedback mechanism is the optional outparam in
> > SkPictureGpuAnalyzer::suitableForGpuRasterization(), but yeah - that doesn't
> > tell you the individual contribution to the veto.
> > 
> > 
> > > And given that more stuff is moving out of SkPicture and into display list
> > items
> > > (e.g.,
> > > ClipPathDisplayItem), we already have to replicate this logic a bit for
the
> > > items which
> > > aren't in pictures. So it seemed like it would be clearer to move the guts
> of 
> > > SkPictureAnalyzer into cc, and implement a numSlowCommands() on
> > DisplayListItem.
> > > Then cc could just sum them all. That would also let us tweak the
parameters
> > for
> > > MSAA
> > > vs non-MSAA vetoes independently.
> > 
> > Assuming we can get numSlowPaths for SKP-backed display items
> > (DrawingDisplayItems), how do we get the equivalent metric for non-SKP based
> > items (say ClipPathDisplayItems)?
> 
> I was thinking we'd just move the logic into cc (ie., check if the clip path
is
> concave 
> in ClipPathDisplayItem::numSlowCommands() (virtual)), Getting the veto wrong
> either way 
> isn't a disaster, and Telemetry should tell us if we mess up.

SkPicture cannot access cc's logic, so we'll have to either duplicate the
internal PathCounter heuristics (ugh) or stop using SkPicture::numSlowPaths -
no?

In my mind, the arguments for keeping the cost heuristics in Skia are

- the details of what is "slow" are highly implementation-dependent, and subject
to change; makes sense to keep the heuristic within the same codebase for
reduced maintenance overhead

- IIUC the GPU team is planning the expand the logic and take into account stuff
like GPU capabilities (based on GrContext); external clients may not have
visibility into these low-level features

- Skia can access SKP draws most efficiently (via SkRecords); if we are to
externalize this logic, we'd have to go through some sort of analysis canvas -
which seems significantly less efficient - or maintain duplicated logic in
SKP/CC.


SkPictureGpuAnalyzer is trying to solve the problem of veto-ing multiple
external fragments while keeping the cost heuristics inside Skia.  I think we
can tweak the API arbitrarily to expose any semantics needed for Chrome:

* pass in more context - MSAA availability, actual GrContext to veto against,
etc.
* expose cost metrics for debugging/tracing
* return a more sophisticated answer, not just veto bool:
USE_NATIVE/USE_MSAA/USE_SOFTWARE
* ?

It may also make sense to split the logic and the discussion between a) cost
heuristics/cost metrics computation and b) the actual veto heuristic (currently
numSlowPaths > 5).  I can't think of a good reason why the latter cannot live in
Chrome - then maybe SkPictureGpuAnalyzer could evolve into a glorified cost
metrics accumulator and Chrome can look at the cost and decide what path to
take.

[Stephen White, 2016-05-26 16:11:35]

On 2016/05/26 15:55:59, f(malita) wrote:
> On 2016/05/26 15:11:21, Stephen White wrote:
> > On 2016/05/26 12:36:00, f(malita) wrote:
> > > On 2016/05/26 00:04:50, Stephen White wrote:
> > > 
> > > > The question we're trying to answer is "why did this page get vetoed?".
> > > 
> > > The current feedback mechanism is the optional outparam in
> > > SkPictureGpuAnalyzer::suitableForGpuRasterization(), but yeah - that
doesn't
> > > tell you the individual contribution to the veto.
> > > 
> > > 
> > > > And given that more stuff is moving out of SkPicture and into display
list
> > > items
> > > > (e.g.,
> > > > ClipPathDisplayItem), we already have to replicate this logic a bit for
> the
> > > > items which
> > > > aren't in pictures. So it seemed like it would be clearer to move the
guts
> > of 
> > > > SkPictureAnalyzer into cc, and implement a numSlowCommands() on
> > > DisplayListItem.
> > > > Then cc could just sum them all. That would also let us tweak the
> parameters
> > > for
> > > > MSAA
> > > > vs non-MSAA vetoes independently.
> > > 
> > > Assuming we can get numSlowPaths for SKP-backed display items
> > > (DrawingDisplayItems), how do we get the equivalent metric for non-SKP
based
> > > items (say ClipPathDisplayItems)?
> > 
> > I was thinking we'd just move the logic into cc (ie., check if the clip path
> is
> > concave 
> > in ClipPathDisplayItem::numSlowCommands() (virtual)), Getting the veto wrong
> > either way 
> > isn't a disaster, and Telemetry should tell us if we mess up.
> 
> SkPicture cannot access cc's logic, so we'll have to either duplicate the
> internal PathCounter heuristics (ugh) or stop using SkPicture::numSlowPaths -
> no?

Sorry, I meant we should move the logic only for ClipPathDisplayItem (and
anything else that doesn't have a picture). Regular display items would just 
return picture.numSlowCommands() (that logic would remain in Skia). Would that 
work?

> In my mind, the arguments for keeping the cost heuristics in Skia are
> 
> - the details of what is "slow" are highly implementation-dependent, and
subject
> to change; makes sense to keep the heuristic within the same codebase for
> reduced maintenance overhead

I agree for the most part. I don't want to pull all of it over.

> - IIUC the GPU team is planning the expand the logic and take into account
stuff
> like GPU capabilities (based on GrContext); external clients may not have
> visibility into these low-level features

Sounds reasonable. I don't want to hamstring the GPU team's ability to do that.

> - Skia can access SKP draws most efficiently (via SkRecords); if we are to
> externalize this logic, we'd have to go through some sort of analysis canvas -
> which seems significantly less efficient - or maintain duplicated logic in
> SKP/CC.

I don't want to do that -- see above.

> SkPictureGpuAnalyzer is trying to solve the problem of veto-ing multiple
> external fragments while keeping the cost heuristics inside Skia.  I think we
> can tweak the API arbitrarily to expose any semantics needed for Chrome:
> 
> * pass in more context - MSAA availability, actual GrContext to veto against,
> etc.
> * expose cost metrics for debugging/tracing
> * return a more sophisticated answer, not just veto bool:
> USE_NATIVE/USE_MSAA/USE_SOFTWARE
> * ?
> 
> It may also make sense to split the logic and the discussion between a) cost
> heuristics/cost metrics computation and b) the actual veto heuristic
(currently
> numSlowPaths > 5).  I can't think of a good reason why the latter cannot live
in
> Chrome - then maybe SkPictureGpuAnalyzer could evolve into a glorified cost
> metrics accumulator and Chrome can look at the cost and decide what path to
> take.

That would work too. So we'd call SkPictureAnalyzer for each of the pictures,
and
each of the clip paths (plus whatever else we think up), and it returns
numSlowCommands() which we trigger off in cc. If we can keep and return a count
of num slow clip paths separately (something else I'd like for stats), that'd be

gravy.

[f(malita), 2016-05-26 16:51:32]

On 2016/05/26 16:11:35, Stephen White wrote:
> On 2016/05/26 15:55:59, f(malita) wrote:
> > On 2016/05/26 15:11:21, Stephen White wrote:
> > > On 2016/05/26 12:36:00, f(malita) wrote:
> > > > On 2016/05/26 00:04:50, Stephen White wrote:
> > > > 
> > > > > The question we're trying to answer is "why did this page get
vetoed?".
> > > > 
> > > > The current feedback mechanism is the optional outparam in
> > > > SkPictureGpuAnalyzer::suitableForGpuRasterization(), but yeah - that
> doesn't
> > > > tell you the individual contribution to the veto.
> > > > 
> > > > 
> > > > > And given that more stuff is moving out of SkPicture and into display
> list
> > > > items
> > > > > (e.g.,
> > > > > ClipPathDisplayItem), we already have to replicate this logic a bit
for
> > the
> > > > > items which
> > > > > aren't in pictures. So it seemed like it would be clearer to move the
> guts
> > > of 
> > > > > SkPictureAnalyzer into cc, and implement a numSlowCommands() on
> > > > DisplayListItem.
> > > > > Then cc could just sum them all. That would also let us tweak the
> > parameters
> > > > for
> > > > > MSAA
> > > > > vs non-MSAA vetoes independently.
> > > > 
> > > > Assuming we can get numSlowPaths for SKP-backed display items
> > > > (DrawingDisplayItems), how do we get the equivalent metric for non-SKP
> based
> > > > items (say ClipPathDisplayItems)?
> > > 
> > > I was thinking we'd just move the logic into cc (ie., check if the clip
path
> > is
> > > concave 
> > > in ClipPathDisplayItem::numSlowCommands() (virtual)), Getting the veto
wrong
> > > either way 
> > > isn't a disaster, and Telemetry should tell us if we mess up.
> > 
> > SkPicture cannot access cc's logic, so we'll have to either duplicate the
> > internal PathCounter heuristics (ugh) or stop using SkPicture::numSlowPaths
-
> > no?
> 
> Sorry, I meant we should move the logic only for ClipPathDisplayItem (and
> anything else that doesn't have a picture). Regular display items would just 
> return picture.numSlowCommands() (that logic would remain in Skia). Would that

> work?

Ah, got you, I guess that would work with the current/trivial heuristic.

My only concerns would be about long-term maintenance (circling back to only
Skia knows what's slow, and maybe in the future the predicate would evolve into
something more complex than isConcave && AA), and the additional contract it
imposes on SP (we don't expect concave paths to show in SKPs).


> > SkPictureGpuAnalyzer is trying to solve the problem of veto-ing multiple
> > external fragments while keeping the cost heuristics inside Skia.  I think
we
> > can tweak the API arbitrarily to expose any semantics needed for Chrome:
> > 
> > * pass in more context - MSAA availability, actual GrContext to veto
against,
> > etc.
> > * expose cost metrics for debugging/tracing
> > * return a more sophisticated answer, not just veto bool:
> > USE_NATIVE/USE_MSAA/USE_SOFTWARE
> > * ?
> > 
> > It may also make sense to split the logic and the discussion between a) cost
> > heuristics/cost metrics computation and b) the actual veto heuristic
> (currently
> > numSlowPaths > 5).  I can't think of a good reason why the latter cannot
live
> in
> > Chrome - then maybe SkPictureGpuAnalyzer could evolve into a glorified cost
> > metrics accumulator and Chrome can look at the cost and decide what path to
> > take.
> 
> That would work too. So we'd call SkPictureAnalyzer for each of the pictures,
> and
> each of the clip paths (plus whatever else we think up), and it returns
> numSlowCommands() which we trigger off in cc. If we can keep and return a
count
> of num slow clip paths separately (something else I'd like for stats), that'd
be
> 
> gravy.

Yeah, we're sort of doing that already (feeding all SKPs to an analyzer, and if
crrev.com/2013723002 lands all clipPaths too).  I think exposing the actual
counters is reasonable - either for stats or for relocating the veto predicate
to Chrome - but we should run it by Mike/Brian and see what they think.