add skpdiff tool to compare bitmaps

add skpdiff tool to compare bitmaps

- start framework for pluggable algorithms
- implement simple number of pixels different OpenCL algo

Committed: http://code.google.com/p/skia/source/detail?r=9616

[Zach Reizner, 2013-06-11 22:23:44]

This is definitely a work in progress, but I want some input on the direction
I'm going. Specifically, I'm wondering if my API around ImageDiffer will crash
and burn as I implement more subclasses.

[djsollen, 2013-06-12 13:32:04]

If you want to chat about any of my comments let me know and we can walk through
them.

https://codereview.chromium.org/16284007/diff/1/Makefile
File Makefile (right):

https://codereview.chromium.org/16284007/diff/1/Makefile#newcode51
Makefile:51: skpdiff
remove this.  For experimental code you'll need to run...

1) ./gyp_skia [path_to_your_gyp]
2) make skpdiff

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
File experimental/skpdiff/SkImageDiffer.cpp (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.cpp:11: : fIsGood(true) {
why do you assume good here and bad in the CLImageDiffer?

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.cpp:23: std::cerr << "Failed to load bitmap
\"" << baseline << "\"" << std::endl;
As a general rule we don't use stl.  Use SkDebugf() instead.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
File experimental/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:1: #ifndef SkImageDiffer_DEFINED
add the standard google copyright at the top of the file

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:11: struct RunProfile {
remove this

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:15: struct QueuedDiff {
Does this need to exist or can it be folded into LocalQueueDiff?

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:29: int queueDiffOfFile(const char
baseline[], const char test[]);
to be consistent wit the rest of skia use const char*

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:31: virtual int queueDiff(SkBitmap *
baseline, SkBitmap * test) = 0;
no space -- SkBitmap*

also add comment about what the function does.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:55: class CLImageDiffer : public
ImageDiffer {
So at first glance I would move this and DifferentPixelsImageDiffer into
SkCLImageDiffer.h so that this file doesn't have a dependency on OpenCL

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp
File experimental/skpdiff/main.cpp (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp...
experimental/skpdiff/main.cpp:23: std::vector<cl::Platform> platformList;
looks like you can also use cl::vector< cl::Platform > platformList;

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp...
experimental/skpdiff/main.cpp:98: if (!differ->init(device(), context())) {
why do you need an init method if you can just pass it to the constructor?

https://codereview.chromium.org/16284007/diff/1/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/16284007/diff/1/gyp/tools.gyp#newcode60
gyp/tools.gyp:60: 'target_name': 'skpdiff',
we need to figure out a better place to put this. Use the pdfviewer.gyp as your
reference, but replace that target with this one and call it skpdiff.gyp and put
it into your experimental directory.

NOTE: I'm not sure if GYP will let you put it there, so let's talk if that
doesn't work.

https://codereview.chromium.org/16284007/diff/1/gyp/tools.gyp#newcode68
gyp/tools.gyp:68: 'skia_lib.gyp:skia_lib',
you will need to update this line to be ../../gyp/skia_lib.gyp:skia_lib

[bsalomon, 2013-06-12 13:55:11]

I see nothing particularly problematic with the overall design. I don't know CL
at all, though.

In the future it will be nice if one or both of the images came in the form of a
texture ID rather than a bitmap. Actually SkBitmap can be backed by a texture,
so the interface need not change. I don't think this is necessary at all for a
first version, though.

I've got some minor style stuff inline.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
File experimental/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:20: class ImageDiffer {
Comments describing these classes would be really helpful.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:20: class ImageDiffer {
Should probably be SkImageDiffer

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:29: int queueDiffOfFile(const char
baseline[], const char test[]);
On 2013/06/12 13:32:04, djsollen wrote:
> to be consistent wit the rest of skia use const char*

Is our rule const char* for null term strings? We use [] often in other
contexts. Perhaps we need to update the style guide.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:72: private:
skia style is to have a \n before public, private, protected.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:94: {
{ goes on prev line

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp
File experimental/skpdiff/main.cpp (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp...
experimental/skpdiff/main.cpp:97: CLImageDiffer* differ = new
DifferentPixelsImageDiffer();
SkNEW(DifferentPixelsImageDiffer)

[Zach Reizner, 2013-06-12 21:57:47]

https://codereview.chromium.org/16284007/diff/1/Makefile
File Makefile (right):

https://codereview.chromium.org/16284007/diff/1/Makefile#newcode51
Makefile:51: skpdiff
On 2013/06/12 13:32:04, djsollen wrote:
> remove this.  For experimental code you'll need to run...
> 
> 1) ./gyp_skia [path_to_your_gyp]
> 2) make skpdiff

pdfviewer has to insert itself here as well. I can't build without it here.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
File experimental/skpdiff/SkImageDiffer.cpp (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.cpp:11: : fIsGood(true) {
On 2013/06/12 13:32:04, djsollen wrote:
> why do you assume good here and bad in the CLImageDiffer?

By default, differs are good. CLImageDiffer is not good because it is not
initialized on construction.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.cpp:23: std::cerr << "Failed to load bitmap
\"" << baseline << "\"" << std::endl;
On 2013/06/12 13:32:04, djsollen wrote:
> As a general rule we don't use stl.  Use SkDebugf() instead.

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
File experimental/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:1: #ifndef SkImageDiffer_DEFINED
On 2013/06/12 13:32:04, djsollen wrote:
> add the standard google copyright at the top of the file

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:11: struct RunProfile {
On 2013/06/12 13:32:04, djsollen wrote:
> remove this

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:15: struct QueuedDiff {
On 2013/06/12 13:32:04, djsollen wrote:
> Does this need to exist or can it be folded into LocalQueueDiff?

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:20: class ImageDiffer {
On 2013/06/12 13:55:11, bsalomon wrote:
> Comments describing these classes would be really helpful.

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:29: int queueDiffOfFile(const char
baseline[], const char test[]);
On 2013/06/12 13:55:11, bsalomon wrote:
> On 2013/06/12 13:32:04, djsollen wrote:
> > to be consistent wit the rest of skia use const char*
> 
> Is our rule const char* for null term strings? We use [] often in other
> contexts. Perhaps we need to update the style guide.

I saw const char[] in the sources I was copying style from. Perhaps I was just
looking at something that was not conforming to the style guide.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:31: virtual int queueDiff(SkBitmap *
baseline, SkBitmap * test) = 0;
On 2013/06/12 13:32:04, djsollen wrote:
> no space -- SkBitmap*
> 
> also add comment about what the function does.

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:55: class CLImageDiffer : public
ImageDiffer {
On 2013/06/12 13:32:04, djsollen wrote:
> So at first glance I would move this and DifferentPixelsImageDiffer into
> SkCLImageDiffer.h so that this file doesn't have a dependency on OpenCL

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:72: private:
On 2013/06/12 13:55:11, bsalomon wrote:
> skia style is to have a \n before public, private, protected.

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/SkImageD...
experimental/skpdiff/SkImageDiffer.h:94: {
On 2013/06/12 13:55:11, bsalomon wrote:
> { goes on prev line

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp
File experimental/skpdiff/main.cpp (right):

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp...
experimental/skpdiff/main.cpp:23: std::vector<cl::Platform> platformList;
On 2013/06/12 13:32:04, djsollen wrote:
> looks like you can also use cl::vector< cl::Platform > platformList;

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp...
experimental/skpdiff/main.cpp:97: CLImageDiffer* differ = new
DifferentPixelsImageDiffer();
On 2013/06/12 13:55:11, bsalomon wrote:
> SkNEW(DifferentPixelsImageDiffer)

Done.

https://codereview.chromium.org/16284007/diff/1/experimental/skpdiff/main.cpp...
experimental/skpdiff/main.cpp:98: if (!differ->init(device(), context())) {
On 2013/06/12 13:32:04, djsollen wrote:
> why do you need an init method if you can just pass it to the constructor?

I plan to make it so that all differs can be constructed into some registry so
that the command line help can enumerate all Differs. I don't necessarily want
them all to be initialized at this point.

https://codereview.chromium.org/16284007/diff/1/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/16284007/diff/1/gyp/tools.gyp#newcode60
gyp/tools.gyp:60: 'target_name': 'skpdiff',
On 2013/06/12 13:32:04, djsollen wrote:
> we need to figure out a better place to put this. Use the pdfviewer.gyp as
your
> reference, but replace that target with this one and call it skpdiff.gyp and
put
> it into your experimental directory.
> 
> NOTE: I'm not sure if GYP will let you put it there, so let's talk if that
> doesn't work.

Done.

https://codereview.chromium.org/16284007/diff/1/gyp/tools.gyp#newcode68
gyp/tools.gyp:68: 'skia_lib.gyp:skia_lib',
On 2013/06/12 13:32:04, djsollen wrote:
> you will need to update this line to be ../../gyp/skia_lib.gyp:skia_lib

Done.

[djsollen, 2013-06-13 13:33:02]

looking good.

https://codereview.chromium.org/16284007/diff/19001/Makefile
File Makefile (right):

https://codereview.chromium.org/16284007/diff/19001/Makefile#newcode52
Makefile:52: skpdiff
alphabetical ordering

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/SkCL...
File experimental/skpdiff/SkCLImageDiffer.cpp (right):

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/SkCL...
experimental/skpdiff/SkCLImageDiffer.cpp:34: bool initResult = this->onInit();
fIsGood = this->onInit();
return fIsGood;

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/SkCL...
experimental/skpdiff/SkCLImageDiffer.cpp:151: return -1;
not required but you may want to consider returning QUEUE_ERROR

#define QUEUE_ERROR -1

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/SkCL...
experimental/skpdiff/SkCLImageDiffer.cpp:154: diff->result = 1.0 /
((double)baseline->width() * baseline->height());
what is this number used for?

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/SkCL...
File experimental/skpdiff/SkCLImageDiffer.h (right):

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/SkCL...
experimental/skpdiff/SkCLImageDiffer.h:31: virtual bool init(cl_device_id
device, cl_context context) SK_OVERRIDE;
this doesn't appear to be an override.

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/main...
File experimental/skpdiff/main.cpp (right):

https://codereview.chromium.org/16284007/diff/19001/experimental/skpdiff/main...
experimental/skpdiff/main.cpp:110:
diff_directories("/usr/local/google/home/zachr/Downloads/diffs/baseline",
let's take these as argv[1] and argv[2] for now

[djsollen, 2013-06-14 17:07:02]

lgtm

[commit-bot: I haz the power, 2013-06-14 17:12:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zachr@google.com/16284007/25001

[commit-bot: I haz the power, 2013-06-14 17:27:00]

Change committed as 9616