fix multithread related crashes in skpdiff

fix multithread related crashes in skpdiff

BUG=skia:1798
R=mtklein@google.com, scroggo@google.com

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

[scroggo, 2013-11-05 21:08:51]

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.cpp
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:181: SkAutoMutexAcquire
recordsLock(fRecordMutex);
I don't know this whole program, but I assume that addDiffs needs to be
protected by a mutex because it is called from multiple threads simultaneously.

Will addDiffs be called while outputRecords is running? It seems like if it is,
some records may not be output. If not, do we still need the mutex?

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:289: SkAutoMutexAcquire
recordsLock(fRecordMutex);
Again, do we need the mutex here?

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.h
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.h...
tools/skpdiff/SkDiffContext.h:120: SkMutex fImageMutex;
Could you create a bug add a link to it here?

[djsollen, 2013-11-05 21:24:30]

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.cpp
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:181: SkAutoMutexAcquire
recordsLock(fRecordMutex);
On 2013/11/05 21:08:52, scroggo wrote:
> I don't know this whole program, but I assume that addDiffs needs to be
> protected by a mutex because it is called from multiple threads
simultaneously.
> 
> Will addDiffs be called while outputRecords is running? It seems like if it
is,
> some records may not be output. If not, do we still need the mutex?

Your right I don't need to try to make the whole class thread safe, just
addDiffs.  As a result I was able to remove the locks from the output methods.

https://codereview.chromium.org/60833002/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:289: SkAutoMutexAcquire
recordsLock(fRecordMutex);
On 2013/11/05 21:08:52, scroggo wrote:
> Again, do we need the mutex here?

Done.

[mtklein, 2013-11-06 14:24:36]

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.cpp:53: SkAutoMutexAcquire imageLock(fImageMutex);
Put these in a {} block or just call .lock()?  It's sort of suggesting the wrong
thing to use an SkAutoMutexAcquire then unconditionally call release() later.

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.cpp:54: if
(!SkImageDecoder::DecodeFile(baselinePath, &baselineBitmap)) {
Does this bottleneck you now?  Is there a way to instantiate your own image
decoders instead of using the globals?

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.h:125: SkMutex fRecordMutex;
You don't seem to grab this mutex when iterating over fRecords.  Is that only
done from one thread?

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDifferent...
File tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDifferent...
tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp:77: fIsGood = false;
I doubt it's going to cause much trouble, but fIsGood is also not protected by
any synchronization.

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
tools/skpdiff/SkImageDiffer.h:17: class SkImageDiffer {
This doesn't seem like a particularly good interface.  Why not just expose the
underlying reality that we're doing this synchronously?  Then a lot of this
stuff becomes less bothersome threading-wise.  All the state would stay on the
stack; no mutexes needed at all.

const char* getName() const;
bool requiresOpenCL() const;
bool diffFile(const char[], const char[], DiffResult*) const;
bool diff(const SkBitmap&, const SkBitmap&, DiffResult*) const;

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
tools/skpdiff/SkImageDiffer.h:96: SkMutex fQueueMutex;
Pretty weird to have this on the base class.

[djsollen, 2013-11-06 15:01:58]

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.cpp:53: SkAutoMutexAcquire imageLock(fImageMutex);
On 2013/11/06 14:24:36, mtklein wrote:
> Put these in a {} block or just call .lock()?  It's sort of suggesting the
wrong
> thing to use an SkAutoMutexAcquire then unconditionally call release() later.

The auto acquire is nice as it handles the cleanup in the event that we hit one
of the return statements.

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.cpp:54: if
(!SkImageDecoder::DecodeFile(baselinePath, &baselineBitmap)) {
On 2013/11/06 14:24:36, mtklein wrote:
> Does this bottleneck you now?  Is there a way to instantiate your own image
> decoders instead of using the globals?

I'm sure it bottlenecks the process a little, but I don't know how much faster
it could be because it crashes when decoding in parallel.  I could spend the
time to estimate what that cost is but I didn't want to add that to this CL,
which is why I filed the other bug.

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.h:125: SkMutex fRecordMutex;
On 2013/11/06 14:24:36, mtklein wrote:
> You don't seem to grab this mutex when iterating over fRecords.  Is that only
> done from one thread?

I just forgot to clean up that comment after updating the behavior at Leon's
suggestion.

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDifferent...
File tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDifferent...
tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp:77: fIsGood = false;
On 2013/11/06 14:24:36, mtklein wrote:
> I doubt it's going to cause much trouble, but fIsGood is also not protected by
> any synchronization.

Yes, it currently isn't read by anything so I was going to refactor it or remove
it in a future CL.

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
tools/skpdiff/SkImageDiffer.h:17: class SkImageDiffer {
On 2013/11/06 14:24:36, mtklein wrote:
> This doesn't seem like a particularly good interface.  Why not just expose the
> underlying reality that we're doing this synchronously?  Then a lot of this
> stuff becomes less bothersome threading-wise.  All the state would stay on the
> stack; no mutexes needed at all.
> 
> const char* getName() const;
> bool requiresOpenCL() const;
> bool diffFile(const char[], const char[], DiffResult*) const;
> bool diff(const SkBitmap&, const SkBitmap&, DiffResult*) const;

Agreed. Is it Ok if I do this refactor in a second CL or do you prefer to have
it done now?

[mtklein, 2013-11-06 15:09:41]

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
tools/skpdiff/SkDiffContext.cpp:53: SkAutoMutexAcquire imageLock(fImageMutex);
On 2013/11/06 15:01:58, djsollen wrote:
> On 2013/11/06 14:24:36, mtklein wrote:
> > Put these in a {} block or just call .lock()?  It's sort of suggesting the
> wrong
> > thing to use an SkAutoMutexAcquire then unconditionally call release()
later.
> 
> The auto acquire is nice as it handles the cleanup in the event that we hit
one
> of the return statements.

Ah.  Brain fart.  LGTM

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
tools/skpdiff/SkImageDiffer.h:17: class SkImageDiffer {
On 2013/11/06 15:01:58, djsollen wrote:
> On 2013/11/06 14:24:36, mtklein wrote:
> > This doesn't seem like a particularly good interface.  Why not just expose
the
> > underlying reality that we're doing this synchronously?  Then a lot of this
> > stuff becomes less bothersome threading-wise.  All the state would stay on
the
> > stack; no mutexes needed at all.
> > 
> > const char* getName() const;
> > bool requiresOpenCL() const;
> > bool diffFile(const char[], const char[], DiffResult*) const;
> > bool diff(const SkBitmap&, const SkBitmap&, DiffResult*) const;
> 
> Agreed. Is it Ok if I do this refactor in a second CL or do you prefer to have
> it done now?

Given how much of this CL becomes moot, I'd personally do it now.

[mtklein, 2013-11-06 15:10:14]

On 2013/11/06 15:09:41, mtklein wrote:
>
https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
> File tools/skpdiff/SkDiffContext.cpp (right):
> 
>
https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkDiffConte...
> tools/skpdiff/SkDiffContext.cpp:53: SkAutoMutexAcquire imageLock(fImageMutex);
> On 2013/11/06 15:01:58, djsollen wrote:
> > On 2013/11/06 14:24:36, mtklein wrote:
> > > Put these in a {} block or just call .lock()?  It's sort of suggesting the
> > wrong
> > > thing to use an SkAutoMutexAcquire then unconditionally call release()
> later.
> > 
> > The auto acquire is nice as it handles the cleanup in the event that we hit
> one
> > of the return statements.
> 
> Ah.  Brain fart.  LGTM
> 
>
https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
> File tools/skpdiff/SkImageDiffer.h (right):
> 
>
https://codereview.chromium.org/60833002/diff/70001/tools/skpdiff/SkImageDiff...
> tools/skpdiff/SkImageDiffer.h:17: class SkImageDiffer {
> On 2013/11/06 15:01:58, djsollen wrote:
> > On 2013/11/06 14:24:36, mtklein wrote:
> > > This doesn't seem like a particularly good interface.  Why not just expose
> the
> > > underlying reality that we're doing this synchronously?  Then a lot of
this
> > > stuff becomes less bothersome threading-wise.  All the state would stay on
> the
> > > stack; no mutexes needed at all.
> > > 
> > > const char* getName() const;
> > > bool requiresOpenCL() const;
> > > bool diffFile(const char[], const char[], DiffResult*) const;
> > > bool diff(const SkBitmap&, const SkBitmap&, DiffResult*) const;
> > 
> > Agreed. Is it Ok if I do this refactor in a second CL or do you prefer to
have
> > it done now?
> 
> Given how much of this CL becomes moot, I'd personally do it now.

Ha.  Had no idea an LGTM in a comment would LGTM the CL.

[djsollen, 2013-11-06 15:53:07]

I have another CL that add support for producing alpha masks waiting in the
wings and it would be easier for me to do the refactor after that CL lands.

[djsollen, 2013-11-07 21:17:44]

still need to do the openCL version, but this has the update ImageDiff API

[scroggo, 2013-11-07 21:43:23]

lgtm

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:96: newRecord->fCommonName =
get_common_prefix(baseName, testName);
Is it okay to have a common name with size 0? Do we ever get to that case in
get_common_prefix?

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:119: newRecord->fDifferencePath =
SkOSPath::SkPathJoin(fDifferenceDir.c_str(),
So if fDifferenceDir is size 0, newRecord->fDifferencePath will be

"/<common_name>"

Is that the intent? (Maybe SkPathJoin should handle an empty  rootPath better.)

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDifferen...
File tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp (right):

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDifferen...
tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp:58: // to protect the createion
of the ID and queuedDiff object.
creation*

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkPMetric.cpp
File tools/skpdiff/SkPMetric.cpp (right):

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkPMetric....
tools/skpdiff/SkPMetric.cpp:261: static double pmetric(const ImageLAB*
baselineLAB, const ImageLAB* testLAB, int* poiCount) {
Can we assert that the parameters are not NULL?

[djsollen, 2013-11-07 21:53:56]

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:96: newRecord->fCommonName =
get_common_prefix(baseName, testName);
On 2013/11/07 21:43:23, scroggo wrote:
> Is it okay to have a common name with size 0? Do we ever get to that case in
> get_common_prefix?

Yes, it is ok as the json will just have an empty string (e.g. we don't crash).

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:119: newRecord->fDifferencePath =
SkOSPath::SkPathJoin(fDifferenceDir.c_str(),
On 2013/11/07 21:43:23, scroggo wrote:
> So if fDifferenceDir is size 0, newRecord->fDifferencePath will be
> 
> "/<common_name>"
> 
> Is that the intent? (Maybe SkPathJoin should handle an empty  rootPath
better.)

that shouldn't be possible as we only set alphaMaskPending to true if the
fDifferenceDir is not empty. We should add a check here that common name is not
empty as well.

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkPMetric.cpp
File tools/skpdiff/SkPMetric.cpp (right):

https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkPMetric....
tools/skpdiff/SkPMetric.cpp:261: static double pmetric(const ImageLAB*
baselineLAB, const ImageLAB* testLAB, int* poiCount) {
On 2013/11/07 21:43:23, scroggo wrote:
> Can we assert that the parameters are not NULL?

Done.

[scroggo, 2013-11-07 22:00:02]

On 2013/11/07 21:53:56, djsollen wrote:
>
https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
> File tools/skpdiff/SkDiffContext.cpp (right):
> 
>
https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
> tools/skpdiff/SkDiffContext.cpp:96: newRecord->fCommonName =
> get_common_prefix(baseName, testName);
> On 2013/11/07 21:43:23, scroggo wrote:
> > Is it okay to have a common name with size 0? Do we ever get to that case in
> > get_common_prefix?
> 
> Yes, it is ok as the json will just have an empty string (e.g. we don't
crash).
> 
>
https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkDiffCont...
> tools/skpdiff/SkDiffContext.cpp:119: newRecord->fDifferencePath =
> SkOSPath::SkPathJoin(fDifferenceDir.c_str(),
> On 2013/11/07 21:43:23, scroggo wrote:
> > So if fDifferenceDir is size 0, newRecord->fDifferencePath will be
> > 
> > "/<common_name>"
> > 
> > Is that the intent? (Maybe SkPathJoin should handle an empty  rootPath
> better.)
> 
> that shouldn't be possible as we only set alphaMaskPending to true if the
> fDifferenceDir is not empty. We should add a check here that common name is
not
> empty as well.
> 
>
https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkPMetric.cpp
> File tools/skpdiff/SkPMetric.cpp (right):
> 
>
https://codereview.chromium.org/60833002/diff/210001/tools/skpdiff/SkPMetric....
> tools/skpdiff/SkPMetric.cpp:261: static double pmetric(const ImageLAB*
> baselineLAB, const ImageLAB* testLAB, int* poiCount) {
> On 2013/11/07 21:43:23, scroggo wrote:
> > Can we assert that the parameters are not NULL?
> 
> Done.

lgtm, but I can't comment on the OpenCL code.

[mtklein, 2013-11-08 15:06:12]

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:88: DiffRecord* newRecord =
fRecords.addToHead(DiffRecord());
If you're feeling precise, I think the release can go right after this line.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:106: alphaMaskPending =
differ->enablePOIAlphaMask();
Can you eliminate enablePOIAlphaMask?  Make it part of diff?  As written, not
threadsafe.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.h:89: *             "baselinePath": "queue.png",
maybe add a commonName to the example?

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDifferen...
File tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDifferen...
tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp:13: static const char
kDifferentPixelsKernelSource[] =
Out of curiosity, do you see us realistically using the opencl version of this? 
Seems neat, but might be easier to just blow it away if we're only really going
to use the _cpu path.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:35: virtual const char* getName() = 0;
Make this const?

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:41: bool isGood() { return fIsGood; }
Can go away now?

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:46: virtual bool requiresOpenCL() { return false;
}
const?

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:52: virtual bool enablePOIAlphaMask() { return
false; }
Param to diff?

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:60: virtual bool diff(SkBitmap* baseline,
SkBitmap* test, Result* result) = 0;
can this be const?  if not, it's a fairly good sign it's not yet threadsafe.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:60: virtual bool diff(SkBitmap* baseline,
SkBitmap* test, Result* result) = 0;
do these bitmaps need to be SkBitmap*?  const SkBitmap*?  const SkBitmap&?

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:63: bool fIsGood;
Can die?

[djsollen, 2013-11-12 16:40:43]

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.cpp (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:88: DiffRecord* newRecord =
fRecords.addToHead(DiffRecord());
On 2013/11/08 15:06:13, mtklein wrote:
> If you're feeling precise, I think the release can go right after this line.

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:106: alphaMaskPending =
differ->enablePOIAlphaMask();
On 2013/11/08 15:06:13, mtklein wrote:
> Can you eliminate enablePOIAlphaMask?  Make it part of diff?  As written, not
> threadsafe.

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
File tools/skpdiff/SkDiffContext.h (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.h:89: *             "baselinePath": "queue.png",
On 2013/11/08 15:06:13, mtklein wrote:
> maybe add a commonName to the example?

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDifferen...
File tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkDifferen...
tools/skpdiff/SkDifferentPixelsMetric_opencl.cpp:13: static const char
kDifferentPixelsKernelSource[] =
On 2013/11/08 15:06:13, mtklein wrote:
> Out of curiosity, do you see us realistically using the opencl version of
this? 
> Seems neat, but might be easier to just blow it away if we're only really
going
> to use the _cpu path.

I don't want to blow it away yet, until I can do some due diligence and know
that the perf benefit is not worth the code complexity.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:35: virtual const char* getName() = 0;
On 2013/11/08 15:06:13, mtklein wrote:
> Make this const?

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:41: bool isGood() { return fIsGood; }
On 2013/11/08 15:06:13, mtklein wrote:
> Can go away now?

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:46: virtual bool requiresOpenCL() { return false;
}
On 2013/11/08 15:06:13, mtklein wrote:
> const?

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:52: virtual bool enablePOIAlphaMask() { return
false; }
On 2013/11/08 15:06:13, mtklein wrote:
> Param to diff?

Done.

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:60: virtual bool diff(SkBitmap* baseline,
SkBitmap* test, Result* result) = 0;
On 2013/11/08 15:06:13, mtklein wrote:
> do these bitmaps need to be SkBitmap*?  const SkBitmap*?  const SkBitmap&?

the function is now const but the input params can't be const as we need to call
lockPixels on them.  I could require that the caller lock the pixels to get
around this.

[mtklein, 2013-11-12 17:00:19]

lgtm

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
File tools/skpdiff/SkImageDiffer.h (right):

https://codereview.chromium.org/60833002/diff/350001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.h:60: virtual bool diff(SkBitmap* baseline,
SkBitmap* test, Result* result) = 0;
On 2013/11/12 16:40:43, djsollen wrote:
> On 2013/11/08 15:06:13, mtklein wrote:
> > do these bitmaps need to be SkBitmap*?  const SkBitmap*?  const SkBitmap&?
> 
> the function is now const but the input params can't be const as we need to
call
> lockPixels on them.  I could require that the caller lock the pixels to get
> around this.

gotcha.  seems fine as-is.

https://codereview.chromium.org/60833002/diff/440001/tools/skpdiff/SkImageDif...
File tools/skpdiff/SkImageDiffer.cpp (right):

https://codereview.chromium.org/60833002/diff/440001/tools/skpdiff/SkImageDif...
tools/skpdiff/SkImageDiffer.cpp:15: const double SkImageDiffer::RESULT_INCORRECT
= 0.0f;
At this point you van probably just get rid of this file.

[djsollen, 2013-11-12 18:29:23]

Committed patchset #5 manually as r12252 (presubmit successful).