Fix for nexus 5 crashing in GL benches

Fix for nexus 5 crashing in GL benches

GLBenches do not expect gl state  to change between onPerCanvasPreDraw and *PostDraw, but we do a clear and sometimes we clear as draw.  This causes us to bind vertex objects / programs / etc.

This change creates two new virtual methods which are called right before and immediately after timing.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/8a6697af95b340aad6dee7e6228048fa305c1e59

[joshualitt, 2015-09-30 17:08:12]

joshualitt@google.com changed reviewers:
+ mtklein@google.com

[joshualitt, 2015-09-30 17:09:05]

joshualitt@google.com changed reviewers:
+ joshualitt@google.com

[joshualitt, 2015-09-30 17:09:06]

ptal, a straightforward change.  Eventually we will move to visual bench
anyways, but these benches should crash even if run in nanobench(though we can
add asserts eventually, or fission VisualBenchmark / Benchmark)

[mtklein_C, 2015-09-30 17:13:23]

On 2015/09/30 17:09:06, joshualitt wrote:
> anyways, but these benches should crash even if run in nanobench(though we can

should *not* crash, right?

[joshualitt, 2015-09-30 17:21:04]

On 2015/09/30 17:13:23, mtklein_C wrote:
> On 2015/09/30 17:09:06, joshualitt wrote:
> > anyways, but these benches should crash even if run in nanobench(though we
can
> 
> should *not* crash, right?

lol, should *not* crash

[mtklein_C, 2015-09-30 17:26:29]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2015-09-30 17:26:29]

https://codereview.chromium.org/1379853003/diff/20001/bench/Benchmark.h
File bench/Benchmark.h (right):

https://codereview.chromium.org/1379853003/diff/20001/bench/Benchmark.h#newco...
bench/Benchmark.h:78: void perCanvasLastCallBeforeDraw(SkCanvas*);
Having names this similar to the perCanvas-flavored calls seems error prone,
especially if we document that we call them once but actually call them for each
sample.

I'm inclined to write it like this:
  // Called just before and after each call to draw().  Not timed.
  void preDraw(SkCanvas*)
  void postDraw(SkCanvas*)

Now, this does have a slight problem, which is that we've already (mis-) used
the name "preDraw()".  So I propose this sequence of global edits to your patch
here:
   1)  s/preDraw/delayedSetup/g
   2) s/onPreDraw/onDelayedSetup/g
   3) s/perCanvasLastCallBeforeDraw/preDraw/g
   4) s/onPerCanvasLastCallBeforeDraw/onPreDraw/g
   5) s/perCanvasFirstCallAfterDraw/postDraw/g
   6) s/onPerCanvasFirstCallAfterDraw/onPostDraw/g
   7) Separate preDraw / postDraw from perCanvasPreDraw and perCanvasPostDraw,
and add comments.

I'd also be fine if you made these directly virtual rather than jumping through
the foo / onFoo hoop.  There's no need to do that for internal tools.  It's also
fine by me if you want to leave it.

[joshualitt, 2015-09-30 17:57:17]

feedback incorporated

https://codereview.chromium.org/1379853003/diff/20001/bench/Benchmark.h
File bench/Benchmark.h (right):

https://codereview.chromium.org/1379853003/diff/20001/bench/Benchmark.h#newco...
bench/Benchmark.h:78: void perCanvasLastCallBeforeDraw(SkCanvas*);
On 2015/09/30 17:26:29, mtklein_C wrote:
> Having names this similar to the perCanvas-flavored calls seems error prone,
> especially if we document that we call them once but actually call them for
each
> sample.
> 
> I'm inclined to write it like this:
>   // Called just before and after each call to draw().  Not timed.
>   void preDraw(SkCanvas*)
>   void postDraw(SkCanvas*)
> 
> Now, this does have a slight problem, which is that we've already (mis-) used
> the name "preDraw()".  So I propose this sequence of global edits to your
patch
> here:
>    1)  s/preDraw/delayedSetup/g
>    2) s/onPreDraw/onDelayedSetup/g
>    3) s/perCanvasLastCallBeforeDraw/preDraw/g
>    4) s/onPerCanvasLastCallBeforeDraw/onPreDraw/g
>    5) s/perCanvasFirstCallAfterDraw/postDraw/g
>    6) s/onPerCanvasFirstCallAfterDraw/onPostDraw/g
>    7) Separate preDraw / postDraw from perCanvasPreDraw and perCanvasPostDraw,
> and add comments.
done
> 
> I'd also be fine if you made these directly virtual rather than jumping
through
> the foo / onFoo hoop.  There's no need to do that for internal tools.  It's
also
> fine by me if you want to leave it.

Meh, I agree with you, but lets just keep it consistent.

[mtklein, 2015-09-30 18:01:46]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-09-30 18:01:46]

lgtm

[commit-bot: I haz the power, 2015-09-30 18:02:05]

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

[commit-bot: I haz the power, 2015-09-30 18:02:58]

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

[commit-bot: I haz the power, 2015-09-30 18:02:58]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[joshualitt, 2015-09-30 18:42:58]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-09-30 18:42:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1379853003/#ps80001
(title: "tweaks")

[commit-bot: I haz the power, 2015-09-30 18:43:25]

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

[commit-bot: I haz the power, 2015-09-30 18:44:27]

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

[commit-bot: I haz the power, 2015-09-30 18:44:27]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[joshualitt, 2015-09-30 19:03:26]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-09-30 19:03:26]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1379853003/#ps100001
(title: "tweaks")

[commit-bot: I haz the power, 2015-09-30 19:03:49]

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

[commit-bot: I haz the power, 2015-09-30 19:11:11]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/8a6697af95b340aad6dee7e6228048fa305c1e59