Added a few bench for some image filters

Added a few bench for some image filters

Committed: https://code.google.com/p/skia/source/detail?r=8824

[sugoi1, 2013-04-12 20:21:28]

I made a few tests to see if these benchmarks are properly made.

I am fully aware that the gyp file isn't in this cl, I just want to have
comments on the benches themselves before I do more.

[Stephen White, 2013-04-15 15:34:21]

https://codereview.chromium.org/14234019/diff/1/bench/BlendBench.cpp
File bench/BlendBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/BlendBench.cpp#newcode36
bench/BlendBench.cpp:36: (SkBlendImageFilter::kNormal_Mode,
background)))->unref();
I think it would be better to break out each of these into separate benchmark
test cases.

https://codereview.chromium.org/14234019/diff/1/bench/ColorFilterBench.cpp
File bench/ColorFilterBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/ColorFilterBench.cpp#ne...
bench/ColorFilterBench.cpp:58: canvas->drawRect(r, paint);
We probably want to break these out into separate benches as well, since they
exercise the color filter matrix collapse optimization.

We should probably have a test case for a single SkColorFilter as well.

We also probably should have a test that exercises SkTableColorFilter.

https://codereview.chromium.org/14234019/diff/1/bench/DisplacementBench.cpp
File bench/DisplacementBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/DisplacementBench.cpp#n...
bench/DisplacementBench.cpp:43: // 2 ) Displacement, with 1 alpha component
(which isn't pre-multiplied)
Should probably split these up into separate bench test cases, and ensure that
they cover all the getValue() template specializations in
SkDisplacementMapEffect, as well permutations of the shader generation (if they
have any perf impact).

https://codereview.chromium.org/14234019/diff/1/bench/DisplacementBench.cpp#n...
bench/DisplacementBench.cpp:48: // 2 ) Displacement, with 2 non-alpha components
Nit:  that's 2 2)'s.  :)

https://codereview.chromium.org/14234019/diff/1/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/MagnifierBench.cpp#newc...
bench/MagnifierBench.cpp:33: const char* str = "The quick brown fox jumped over
the lazy dog.";
We should probably rewrite this one (as well as the GM) to draw a smaller image.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp
File bench/MergeBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode19
bench/MergeBench.cpp:19: class MergeBench : public SkBenchmark {
This bench seems to test a lot more than just Merge.  Perhaps we should break
out the large-DAG tests into their own bench, and maybe use something a bit more
real-world, like a skia version of the example here: 
http://www.w3.org/TR/SVG/filters.html#AnExample.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode41
bench/MergeBench.cpp:41: for (size_t i = 0; i < SK_ARRAY_COUNT(gFilterProc);
++i) {
Again, should be separate bench test cases.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode74
bench/MergeBench.cpp:74: return new SkMergeImageFilter(first, second);
I think it'd be better to have something that isolates the performance of Merge
alone; perhaps using SkBitmapSource as inputs?

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode79
bench/MergeBench.cpp:79: SkImageFilter* inner = new
SkDownSampleImageFilter(SK_Scalar1 / 5);
I'd test SkDownSampleImageFilter separately in its own test case, and try to
isolate the performance of SkComposeImageFilter here (maybe also in its own
file, rather than MergeBench?)

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode96
bench/MergeBench.cpp:96: SkImageFilter* inner = new SkBlurImageFilter(8, 0);
Blur will probably dominate the performance time here, so we should probably use
something simpler (perhaps just SkBitmapSource) to isolate the performance of
SkComposeImageFilter (or eliminate this one in favour of a reduced version of
the one above).

[sugoi1, 2013-04-19 18:09:42]

https://codereview.chromium.org/14234019/diff/1/bench/BlendBench.cpp
File bench/BlendBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/BlendBench.cpp#newcode36
bench/BlendBench.cpp:36: (SkBlendImageFilter::kNormal_Mode,
background)))->unref();
On 2013/04/15 15:34:21, Stephen White wrote:
> I think it would be better to break out each of these into separate benchmark
> test cases.

Done.

https://codereview.chromium.org/14234019/diff/1/bench/ColorFilterBench.cpp
File bench/ColorFilterBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/ColorFilterBench.cpp#ne...
bench/ColorFilterBench.cpp:58: canvas->drawRect(r, paint);
On 2013/04/15 15:34:21, Stephen White wrote:
> We probably want to break these out into separate benches as well, since they
> exercise the color filter matrix collapse optimization.
> 
> We should probably have a test case for a single SkColorFilter as well.
> 
> We also probably should have a test that exercises SkTableColorFilter.

Done.

https://codereview.chromium.org/14234019/diff/1/bench/DisplacementBench.cpp
File bench/DisplacementBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/DisplacementBench.cpp#n...
bench/DisplacementBench.cpp:43: // 2 ) Displacement, with 1 alpha component
(which isn't pre-multiplied)
On 2013/04/15 15:34:21, Stephen White wrote:
> Should probably split these up into separate bench test cases, and ensure that
> they cover all the getValue() template specializations in
> SkDisplacementMapEffect, as well permutations of the shader generation (if
they
> have any perf impact).

Done.

https://codereview.chromium.org/14234019/diff/1/bench/DisplacementBench.cpp#n...
bench/DisplacementBench.cpp:48: // 2 ) Displacement, with 2 non-alpha components
On 2013/04/15 15:34:21, Stephen White wrote:
> Nit:  that's 2 2)'s.  :)
> 

Done.

https://codereview.chromium.org/14234019/diff/1/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/MagnifierBench.cpp#newc...
bench/MagnifierBench.cpp:33: const char* str = "The quick brown fox jumped over
the lazy dog.";
On 2013/04/15 15:34:21, Stephen White wrote:
> We should probably rewrite this one (as well as the GM) to draw a smaller
image.

Done. I made it twice as small in each direction, so 4 times smaller, but I
haven't changed the GM.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp
File bench/MergeBench.cpp (right):

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode41
bench/MergeBench.cpp:41: for (size_t i = 0; i < SK_ARRAY_COUNT(gFilterProc);
++i) {
On 2013/04/15 15:34:21, Stephen White wrote:
> Again, should be separate bench test cases.

I only kept 1, only with bitmaps, to get a better bench of the merge filter
only.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode74
bench/MergeBench.cpp:74: return new SkMergeImageFilter(first, second);
On 2013/04/15 15:34:21, Stephen White wrote:
> I think it'd be better to have something that isolates the performance of
Merge
> alone; perhaps using SkBitmapSource as inputs?

Removed.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode79
bench/MergeBench.cpp:79: SkImageFilter* inner = new
SkDownSampleImageFilter(SK_Scalar1 / 5);
On 2013/04/15 15:34:21, Stephen White wrote:
> I'd test SkDownSampleImageFilter separately in its own test case, and try to
> isolate the performance of SkComposeImageFilter here (maybe also in its own
> file, rather than MergeBench?)

Removed.

https://codereview.chromium.org/14234019/diff/1/bench/MergeBench.cpp#newcode96
bench/MergeBench.cpp:96: SkImageFilter* inner = new SkBlurImageFilter(8, 0);
On 2013/04/15 15:34:21, Stephen White wrote:
> Blur will probably dominate the performance time here, so we should probably
use
> something simpler (perhaps just SkBitmapSource) to isolate the performance of
> SkComposeImageFilter (or eliminate this one in favour of a reduced version of
> the one above).

Removed.

[Stephen White, 2013-04-19 18:39:01]

https://codereview.chromium.org/14234019/diff/6001/bench/BlendBench.cpp
File bench/BlendBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/BlendBench.cpp#newco...
bench/BlendBench.cpp:36: fBitmap.setConfig(SkBitmap::kARGB_8888_Config, 80, 80);
As discussed, we should probably use a larger bitmap, just to stress fill rate a
bit more.  Something that doesn't fit in L1 cache.  :)  Maybe one small like
this, one large, say 512x512 (if it's not too slow).

https://codereview.chromium.org/14234019/diff/6001/bench/ColorFilterBench.cpp
File bench/ColorFilterBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/ColorFilterBench.cpp...
bench/ColorFilterBench.cpp:13: #define FILTER_WIDTH    SkIntToScalar(30)
As above, maybe one 512x512, one 32x32.

https://codereview.chromium.org/14234019/diff/6001/bench/DisplacementBench.cpp
File bench/DisplacementBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/DisplacementBench.cp...
bench/DisplacementBench.cpp:28: fBitmap.setConfig(SkBitmap::kARGB_8888_Config,
80, 80);
Same here, I'd say 32x32 and 512x512.

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:11: #define WIDTH 250
As above, perhaps 32x32 and 512x512.

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:41: canvas->drawText(str, strlen(str),
SkIntToScalar(x),
You should probably preinitialize the input bitmap here, to take text drawing
speed out of the equation.  And I'd take out the 25x loop, since the bench
infrastructure should take care of iterations (I think).

https://codereview.chromium.org/14234019/diff/6001/bench/MergeBench.cpp
File bench/MergeBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/MergeBench.cpp#newco...
bench/MergeBench.cpp:14: #define FILTER_WIDTH    SkIntToScalar(150)
As above, two sizes.

[jvanverth1, 2013-04-19 19:26:31]

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:35: srand(1234);
Is there a reason you're using rand()? Can you try SkMWCRandom instead?

[sugoi1, 2013-04-19 20:47:27]

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:35: srand(1234);
On 2013/04/19 19:26:31, JimVV wrote:
> Is there a reason you're using rand()? Can you try SkMWCRandom instead?

The quality of the randomness isn't really important here. I could also hardcode
some values. I'll change it to SkMWCRandom, it's doesn't make a difference to
me. Note that this part was copied from the GM, so you may want to change the GM
if rand() should not be used in GMs.

[sugoi1, 2013-04-22 15:49:47]

https://codereview.chromium.org/14234019/diff/6001/bench/BlendBench.cpp
File bench/BlendBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/BlendBench.cpp#newco...
bench/BlendBench.cpp:36: fBitmap.setConfig(SkBitmap::kARGB_8888_Config, 80, 80);
On 2013/04/19 18:39:01, Stephen White wrote:
> As discussed, we should probably use a larger bitmap, just to stress fill rate
a
> bit more.  Something that doesn't fit in L1 cache.  :)  Maybe one small like
> this, one large, say 512x512 (if it's not too slow).

Bumped it only to 256x256 so that it's not too slow (GPU is very fast, but CPU
is a bit slow)

https://codereview.chromium.org/14234019/diff/6001/bench/ColorFilterBench.cpp
File bench/ColorFilterBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/ColorFilterBench.cpp...
bench/ColorFilterBench.cpp:13: #define FILTER_WIDTH    SkIntToScalar(30)
On 2013/04/19 18:39:01, Stephen White wrote:
> As above, maybe one 512x512, one 32x32.

Again, here, replaced 512x512 by 256x256. It's still fairly slow in some cases.

https://codereview.chromium.org/14234019/diff/6001/bench/DisplacementBench.cpp
File bench/DisplacementBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/DisplacementBench.cp...
bench/DisplacementBench.cpp:28: fBitmap.setConfig(SkBitmap::kARGB_8888_Config,
80, 80);
On 2013/04/19 18:39:01, Stephen White wrote:
> Same here, I'd say 32x32 and 512x512.

Done.

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:11: #define WIDTH 250
On 2013/04/19 18:39:01, Stephen White wrote:
> As above, perhaps 32x32 and 512x512.

Done.

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:35: srand(1234);
Well, I ended up removing all random number generation from the bench...

https://codereview.chromium.org/14234019/diff/6001/bench/MagnifierBench.cpp#n...
bench/MagnifierBench.cpp:41: canvas->drawText(str, strlen(str),
SkIntToScalar(x),
On 2013/04/19 18:39:01, Stephen White wrote:
> You should probably preinitialize the input bitmap here, to take text drawing
> speed out of the equation.  And I'd take out the 25x loop, since the bench
> infrastructure should take care of iterations (I think).

Done.

https://codereview.chromium.org/14234019/diff/6001/bench/MergeBench.cpp
File bench/MergeBench.cpp (right):

https://codereview.chromium.org/14234019/diff/6001/bench/MergeBench.cpp#newco...
bench/MergeBench.cpp:14: #define FILTER_WIDTH    SkIntToScalar(150)
On 2013/04/19 18:39:01, Stephen White wrote:
> As above, two sizes.

Done.

[Stephen White, 2013-04-22 21:36:11]

Sorry to be annoying, but let's make all the sizes line up, so we can get a
rough idea of the relative cost if each.

https://codereview.chromium.org/14234019/diff/15001/bench/DisplacementBench.cpp
File bench/DisplacementBench.cpp (right):

https://codereview.chromium.org/14234019/diff/15001/bench/DisplacementBench.c...
bench/DisplacementBench.cpp:15: #define FILTER_WIDTH_LARGE  SkIntToScalar(512)
Since you've made the others 256x256, let's do the same here.

https://codereview.chromium.org/14234019/diff/15001/bench/MagnifierBench.cpp
File bench/MagnifierBench.cpp (right):

https://codereview.chromium.org/14234019/diff/15001/bench/MagnifierBench.cpp#...
bench/MagnifierBench.cpp:15: #define FILTER_WIDTH_LARGE  SkIntToScalar(512)
Same here.

[sugoi1, 2013-04-23 14:08:51]

Switched size to 256x256 in all cases (for large size)

[Stephen White, 2013-04-23 14:19:12]

LGTM

[sugoi1, 2013-04-23 14:20:51]

Committed patchset #4 manually as r8824 (presubmit successful).