Refactor BenchmarkingCanvas to avoid internal Skia dependencies

Refactor BenchmarkingCanvas to avoid internal Skia dependencies

Re-implement BenchmarkingCanvas functionality based on public Skia APIs
only.

Convert Skia types to base::Value() structured data hierarchies (which
are then pass through to the viewer).

BUG=457691
R=reed@google.com,robertphillips@google.com,nduca@chromium.org,pdr@chromium.org

Committed: https://crrev.com/f5190e0b6f8f56c5f715928b4c68d39564cb1988
Cr-Commit-Position: refs/heads/master@{#318236}

[f(malita), 2015-02-19 18:23:01]

I couldn't quite get myself to rip out the trace-viewer paint debugger, so
instead I took a stab at refactoring.

The CL is chunky, but most of it is just boring Skia types -> base::Value
conversion goo.

The skia_benchmarking_extension glue code actually got thinner/cleaner as we're
now just passing/converting base::Value structured data directly to the viewer.

Command param data is likely not as complete as Skia's stringified version at
this point, but on the upside it is structured! This opens some interesting
presentation possibilities in the viewer.

Trace-viewer update for displaying the new structs:
https://github.com/fmalita/trace-viewer/commit/b2ab3469b0f18ec06a832c622bac4f...

[pdr., 2015-02-20 04:48:00]

Is there a performance difference with this approach vs the original?

https://codereview.chromium.org/942533002/diff/20002/content/renderer/skia_be...
File content/renderer/skia_benchmarking_extension.cc (right):

https://codereview.chromium.org/942533002/diff/20002/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:182: // FIXME: no overdraw
support ATM.
Maybe we should just take this out? Overdraw seems useful but I've never
actually seen someone use it.

[f(malita), 2015-02-20 13:57:00]

On 2015/02/20 04:48:00, pdr wrote:
> Is there a performance difference with this approach vs the original?

At a high level we are more efficient now for a couple of reasons:

* Rasterize() only draws once, into one canvas (instead of twice: once in a
SkDebugCanvas then replay that up to the desired op index into a real bitmap).
That also means we never draw more than the requested number of ops - which can
potentially make a big difference for partial replays of large SKPs.

* BenchmarkingCanvas now only proxies to one real canvas (instead of two: one
SkDebugCanvas and one TimingCanvas which in turn forwards to another real
canvas). So GetOpTimings() should also be leaner.

OTOH it's hard to compare base::Value marshalling vs. the SkDebugCanvas'
stingification. I certainly like the former's pass-through nature better though.

Subjectively, I can't tell a difference: large frames that used to be slow are
still slow (some cnn.com skps take ~2s to process on my computer). But
instrumenting the methods does show the expected improvements - so the bulk of
cpu time must be spent somewhere else:

                    ToT     CL

#ops:              3742   3746
GetOpTimings(ms):    47     22
Rasterize(ms):       84     63

https://codereview.chromium.org/942533002/diff/20002/content/renderer/skia_be...
File content/renderer/skia_benchmarking_extension.cc (right):

https://codereview.chromium.org/942533002/diff/20002/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:182: // FIXME: no overdraw
support ATM.
On 2015/02/20 04:48:00, pdr wrote:
> Maybe we should just take this out? Overdraw seems useful but I've never
> actually seen someone use it.

Sounds reasonable. The only reason I'm hesitating is that I expect it to be
fairly easy to implement (just a custom color filter or xfermode to be slapped
onto each panint).

[f(malita), 2015-02-20 14:03:33]

On 2015/02/20 13:57:00, f(malita) wrote:
> On 2015/02/20 04:48:00, pdr wrote:
> > Is there a performance difference with this approach vs the original?
> 
> At a high level we are more efficient now

(by that I mean "with this CL" :)

[pdr., 2015-02-20 18:17:53]

LGTM from the peanut gallery that doesn't know much about skia/ or cc/. Robert
or Reed, could you please review this as well?

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
File skia/ext/benchmarking_canvas.cc (right):

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
skia/ext/benchmarking_canvas.cc:435: // FIXME: moar params
Oops, a fixme slipped in.

[f(malita), 2015-02-23 19:53:18]

Ping for Skia review.

[robertphillips, 2015-02-23 20:30:21]

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
File skia/ext/benchmarking_canvas.cc (right):

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
skia/ext/benchmarking_canvas.cc:180: 
ImageFilters?
ColorFilters?

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
skia/ext/benchmarking_canvas.cc:254: val->Set("bounds",
AsValue(path.getBounds()));
Dump out convexity, verbs & points?

[reed1, 2015-02-23 20:42:45]

lgtm

[f(malita), 2015-02-24 17:45:00]

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
File skia/ext/benchmarking_canvas.cc (right):

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
skia/ext/benchmarking_canvas.cc:180: 
On 2015/02/23 20:30:21, robertphillips wrote:
> ImageFilters?
> ColorFilters?

Done.

(for ImageFilters there's not much info to extract atm)

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
skia/ext/benchmarking_canvas.cc:254: val->Set("bounds",
AsValue(path.getBounds()));
On 2015/02/23 20:30:21, robertphillips wrote:
> Dump out convexity, verbs & points?

Done.

https://codereview.chromium.org/942533002/diff/20002/skia/ext/benchmarking_ca...
skia/ext/benchmarking_canvas.cc:435: // FIXME: moar params
On 2015/02/20 18:17:53, pdr wrote:
> Oops, a fixme slipped in.

Done.

[f(malita), 2015-02-24 18:19:53]

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

[f(malita), 2015-02-24 18:19:54]

+piman for the skia_benchmarking_extension.cc cleanup bits.

Nat, can you also take a quick look at the Picture API tweaks?

Thanks!

[piman, 2015-02-24 21:55:09]

LGTM for content/ but some style nits.

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
File content/renderer/skia_benchmarking_extension.cc (right):

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:65: , playback_count_(count) {
nit: chromium style please (, goes to previous line).
run git cl format?

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:69: return
canvas_.CommandCount() > playback_count_;
nit: indent should be +2

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:182: // FIXME: no overdraw
support ATM.
nit: // TODO(fmalita): no overdraw support at the moment.
Is there a bug filed for whatever needs to be done (if so, you can reference
here).

[f(malita), 2015-02-24 22:29:20]

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
File content/renderer/skia_benchmarking_extension.cc (right):

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:65: , playback_count_(count) {
On 2015/02/24 21:55:09, piman (Very slow to review) wrote:
> nit: chromium style please (, goes to previous line).
> run git cl format?

Done.

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:69: return
canvas_.CommandCount() > playback_count_;
On 2015/02/24 21:55:09, piman (Very slow to review) wrote:
> nit: indent should be +2

Done.

https://codereview.chromium.org/942533002/diff/60001/content/renderer/skia_be...
content/renderer/skia_benchmarking_extension.cc:182: // FIXME: no overdraw
support ATM.
On 2015/02/24 21:55:09, piman (Very slow to review) wrote:
> nit: // TODO(fmalita): no overdraw support at the moment.
> Is there a bug filed for whatever needs to be done (if so, you can reference
> here).

Good idea - I've opened http://crbug.com/461534 for tracking. Also, to keep the
bug/fix localized, I went ahead and plumbed the overdraw flag into
BenchmarkingCanvas (and relocated the TODO there).

[f(malita), 2015-02-25 22:44:58]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-02-25 22:44:59]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
reed@google.com, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/942533002/#ps80001 (title:
"review comments")

[commit-bot: I haz the power, 2015-02-25 22:46:53]

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

[commit-bot: I haz the power, 2015-02-26 02:36:19]

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

[commit-bot: I haz the power, 2015-02-26 02:36:21]

Failed to commit the patch.

[commit-bot: I haz the power, 2015-02-26 02:36:42]

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

[f(malita), 2015-02-26 15:39:04]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-02-26 15:39:32]

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

[commit-bot: I haz the power, 2015-02-26 15:49:03]

Committed patchset #6 (id:80001)

[commit-bot: I haz the power, 2015-02-26 15:49:54]

Patchset 6 (id:??) landed as
https://crrev.com/f5190e0b6f8f56c5f715928b4c68d39564cb1988
Cr-Commit-Position: refs/heads/master@{#318236}