SkPictureGpuAnalyzer

SkPictureGpuAnalyzer

Stateful helper for gathering multi-picture GPU stats.

Exposes the existing SkPicture GPU veto semantics, while preserving
the SKP impl (which has some nice properties: lazy, hierarchical,
cached per pic).

R=reed@google.com,bsalomon@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1974833003

Committed: https://skia.googlesource.com/skia/+/796e365999220f53acc58ba91ed55bb35900c8bc

[f(malita), 2016-05-12 16:38:23]

Description was changed from

==========
SkPictureAnalyzer

Stateful helper for gathering multi-picture statistics.

Exposes the existing SkPicture stats, while preserving the SKP impl
(which has some nice properties: lazy, hierarchical, cached per pic).

R=reed@google.com,bsalomon@google.com
==========

to

==========
SkPictureAnalyzer

Stateful helper for gathering multi-picture statistics.

Exposes the existing SkPicture stats, while preserving the SKP impl
(which has some nice properties: lazy, hierarchical, cached per pic).

R=reed@google.com,bsalomon@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[f(malita), 2016-05-12 19:54:30]

Description was changed from

==========
SkPictureAnalyzer

Stateful helper for gathering multi-picture statistics.

Exposes the existing SkPicture stats, while preserving the SKP impl
(which has some nice properties: lazy, hierarchical, cached per pic).

R=reed@google.com,bsalomon@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPictureGpuAnalyzer

Stateful helper for gathering multi-picture GPU stats.

Exposes the existing SkPicture GPU veto semantics, while preserving
the SKP impl (which has some nice properties: lazy, hierarchical,
cached per pic).

R=reed@google.com,bsalomon@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[f(malita), 2016-05-12 19:57:43]

For now this is just a stateful wrapper for existing SkPicture's numSlowPaths
and heuristics.

[f(malita), 2016-05-12 19:57:51]

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

[commit-bot: I haz the power, 2016-05-12 19:57:59]

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

[f(malita), 2016-05-12 19:59:09]

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

[commit-bot: I haz the power, 2016-05-12 19:59:19]

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

[commit-bot: I haz the power, 2016-05-12 20:25:49]

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

[commit-bot: I haz the power, 2016-05-12 20:25:50]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-05-12 20:59:02]

https://codereview.chromium.org/1974833003/diff/60001/include/core/SkPictureA...
File include/core/SkPictureAnalyzer.h (right):

https://codereview.chromium.org/1974833003/diff/60001/include/core/SkPictureA...
include/core/SkPictureAnalyzer.h:39: bool
suitableForGpuRasterization(GrContext*, const char** whyNot = nullptr) const;
Should we take the GrContext* in the constructor in case in the future we want
to decide what needs to be tracked based on its properties?

Also, I think we should take a GrContextThreadSafeProxy instead. This would make
it safe to run the analyzer on a different thread than the one used to draw the
pictures. Maybe its not needed today, but could be in the future.

[f(malita), 2016-05-12 20:59:04]

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

[commit-bot: I haz the power, 2016-05-12 20:59:06]

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

[f(malita), 2016-05-12 21:09:18]

fmalita@chromium.org changed reviewers:
+ chrishtr@chromium.org

[f(malita), 2016-05-12 21:15:19]

https://codereview.chromium.org/1974833003/diff/60001/include/core/SkPictureA...
File include/core/SkPictureAnalyzer.h (right):

https://codereview.chromium.org/1974833003/diff/60001/include/core/SkPictureA...
include/core/SkPictureAnalyzer.h:39: bool
suitableForGpuRasterization(GrContext*, const char** whyNot = nullptr) const;
On 2016/05/12 20:59:02, bsalomon wrote:
> Should we take the GrContext* in the constructor in case in the future we want
> to decide what needs to be tracked based on its properties?
> 
> Also, I think we should take a GrContextThreadSafeProxy instead. This would
make
> it safe to run the analyzer on a different thread than the one used to draw
the
> pictures. Maybe its not needed today, but could be in the future.

Done.

[chrishtr, 2016-05-12 21:41:03]

This approach looks good to me for the GPU veto use case in Chrome.

[bsalomon, 2016-05-13 16:14:27]

https://codereview.chromium.org/1974833003/diff/120001/include/core/SkPicture...
File include/core/SkPictureAnalyzer.h (right):

https://codereview.chromium.org/1974833003/diff/120001/include/core/SkPicture...
include/core/SkPictureAnalyzer.h:23: explicit
SkPictureGpuAnalyzer(GrContextThreadSafeProxy* = nullptr);
sk_sp<GrContextThreadSafeProxy>?

[f(malita), 2016-05-13 16:59:16]

https://codereview.chromium.org/1974833003/diff/120001/include/core/SkPicture...
File include/core/SkPictureAnalyzer.h (right):

https://codereview.chromium.org/1974833003/diff/120001/include/core/SkPicture...
include/core/SkPictureAnalyzer.h:23: explicit
SkPictureGpuAnalyzer(GrContextThreadSafeProxy* = nullptr);
On 2016/05/13 16:14:27, bsalomon wrote:
> sk_sp<GrContextThreadSafeProxy>?

Done.  This also forced me to think about non-SK_SUPPORT_GPU builds: since this
helper is GPU-specific, I've added guards to hide it in that case.

[f(malita), 2016-05-13 16:59:21]

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

[commit-bot: I haz the power, 2016-05-13 16:59:28]

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

[commit-bot: I haz the power, 2016-05-13 17:22:43]

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

[commit-bot: I haz the power, 2016-05-13 17:22:44]

Dry run: This issue passed the CQ dry run.

[bsalomon, 2016-05-13 17:43:20]

lgtm

[f(malita), 2016-05-13 18:39:05]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2016-05-13 18:39:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974833003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974833003/140001

[commit-bot: I haz the power, 2016-05-13 18:40:10]

Description was changed from

==========
SkPictureGpuAnalyzer

Stateful helper for gathering multi-picture GPU stats.

Exposes the existing SkPicture GPU veto semantics, while preserving
the SKP impl (which has some nice properties: lazy, hierarchical,
cached per pic).

R=reed@google.com,bsalomon@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPictureGpuAnalyzer

Stateful helper for gathering multi-picture GPU stats.

Exposes the existing SkPicture GPU veto semantics, while preserving
the SKP impl (which has some nice properties: lazy, hierarchical,
cached per pic).

R=reed@google.com,bsalomon@google.com
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/796e365999220f53acc58ba91ed55bb35900c8bc
==========

[commit-bot: I haz the power, 2016-05-13 18:40:11]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/796e365999220f53acc58ba91ed55bb35900c8bc

[reed1, 2016-05-13 19:35:47]

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

https://codereview.chromium.org/1974833003/diff/140001/include/core/SkPicture...
include/core/SkPictureAnalyzer.h:25: explicit
SkPictureGpuAnalyzer(sk_sp<GrContextThreadSafeProxy> = nullptr);
Are we really ref-ing the ctx? Do we need the analyzer to outlive the caller's
grcontext?

[f(malita), 2016-05-13 19:46:45]

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

https://codereview.chromium.org/1974833003/diff/140001/include/core/SkPicture...
include/core/SkPictureAnalyzer.h:25: explicit
SkPictureGpuAnalyzer(sk_sp<GrContextThreadSafeProxy> = nullptr);
On 2016/05/13 19:35:47, reed1 wrote:
> Are we really ref-ing the ctx? Do we need the analyzer to outlive the caller's
> grcontext?

Hmm, good question: the current use pattern doesn't need a ref (short
lived/one-shot), but maybe going forward we'll want to cache these analyzers -
then holding a ref seems less fragile.

I don't have a strong opinion and at this point a ref doesn't have any
advantage; we can go whichever way you and Brian decide.