update skpdiff visualization (image magnification with alpha mask)

update skpdiff visualization (image magnification with alpha mask)

R=epoger@google.com

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

[epoger, 2013-10-21 15:02:52]

Thanks for taking this on.  I will be happy to discuss any of this in-person if
you like...

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

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:84: // compute the common name
What does "common name" mean exactly?  Does it mean the name of the test with
the config stripped off?  Or just "whatever characters baselinePath and testPath
have in common"?

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:88: if (baseName[x] != testName[x]) {
Is there the possibility for out-of-bounds reference if testName is shorter than
baseName?

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:89: baseName.insertUnichar(x, '\n');
Why do you insert a newline rather than just truncating it?

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:114: && 1.0f != diffData.fResult
Is 1.0 a "magic number" that fResult is assigned in a particular case?

If so... please use a constant

If not... are you concerned about float comparisons not matching up exactly?

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:117: newRecord->fCommonName.c_str());
Is fCommonName an SkString or a const char *? 

Line 93: newRecord->fCommonName = baseName.c_str();
Line 117: reference to newRecord->fCommonName.c_str()

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:238:
stream.writeText(currentRecord->fCommonName.c_str());
ditto (const char * vs SkString)

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDifferentPixe...
File tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp (right):

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDifferentPixe...
tools/skpdiff/SkDifferentPixelsMetric_cpu.cpp:49: if (fPOIAlphaMask) {
Where is fPOIAlphaMask declared?  I don't see it in SkDiffContext.h

Oh, I see, it's in the SkImageDiffer class declaration, not the
SkDifferentPixelsMetric class declaration (and SkDifferentPixelsMetric extends
SkImageDiffer).  Since it's only used in SkDifferentPixelsMetric, wouldn't it be
better to declare it as part of that class?

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

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkImageDiffer.h...
tools/skpdiff/SkImageDiffer.h:42: virtual void enablePOIAlphaMask() {
fPOIAlphaMask = true; }
Related to my comments above about fPOIAlphaMask: maybe instead, match
getPointsOfInterestAlphaMask below:

virtual void enablePOIAlphaMask() { /* do nothing */ }

and override enablePOIAlphaMask() in SkDifferentPixelsMetric class

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js
File tools/skpdiff/diff_viewer.js (right):

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:2: var MAGNIFIER_WIDTH = 200;
Is the web UI available somewhere for me to view easily?  (i.e. without patching
this CL into a local workspace and preparing my own JSON file to feed it)

As much as I love reading Javascript, my feedback will be more helpful if I can
play around with the UI

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:11: // Custom directive for showing an image that
gets swapped my mouseover.
my -> by?

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:22: var magnifiyContent = false;
magnifiy -> magnify

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:35: image.src = scope.record.testPath;
inconsistent indentation in here

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:48: while ((image.width / divisor) >
MAX_SWAP_IMG_SIZE) {
The iterative implementation is _ok_, but couldn't you compute this in one shot
using log()?

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:124: scope.MagnifyDraw = function(event,
startMagnify) {
please document the function parameters and behavior

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:126: scope.setMagnifierState(true);
inconsistent indentation again

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:158: $scope.imgScaleFactor = 1.0;
given that the image is always REDUCED, not expanded, maybe it would be better
to store it as the divisor (and thus avoid floating-point resolution issues)?

[djsollen, 2013-11-06 16:02:29]

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

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:84: // compute the common name
On 2013/10/21 15:02:53, epoger wrote:
> What does "common name" mean exactly?  Does it mean the name of the test with
> the config stripped off?  Or just "whatever characters baselinePath and
testPath
> have in common"?

Since the input can be two arbitrary files it means whatever characters they
have in common (prefix only).  In practice when used with GM output this is the
test name with the config stripped off.

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:88: if (baseName[x] != testName[x]) {
On 2013/10/21 15:02:53, epoger wrote:
> Is there the possibility for out-of-bounds reference if testName is shorter
than
> baseName?

Done.

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:89: baseName.insertUnichar(x, '\n');
On 2013/10/21 15:02:53, epoger wrote:
> Why do you insert a newline rather than just truncating it?

SkString doesn't support realloc so if we were to try to truncate it we would
need to alloc new memory, do a copy, and free the old memory just to save a few
bytes.  That seemed like more overhead than it was worth.

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:114: && 1.0f != diffData.fResult
On 2013/10/21 15:02:53, epoger wrote:
> Is 1.0 a "magic number" that fResult is assigned in a particular case?
> 
> If so... please use a constant
> 
> If not... are you concerned about float comparisons not matching up exactly?

Done.

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:117: newRecord->fCommonName.c_str());
On 2013/10/21 15:02:53, epoger wrote:
> Is fCommonName an SkString or a const char *? 
> 
> Line 93: newRecord->fCommonName = baseName.c_str();
> Line 117: reference to newRecord->fCommonName.c_str()

It is an SkString.  I updated line 93 for clarity.

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

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkImageDiffer.h...
tools/skpdiff/SkImageDiffer.h:42: virtual void enablePOIAlphaMask() {
fPOIAlphaMask = true; }
On 2013/10/21 15:02:53, epoger wrote:
> Related to my comments above about fPOIAlphaMask: maybe instead, match
> getPointsOfInterestAlphaMask below:
> 
> virtual void enablePOIAlphaMask() { /* do nothing */ }
> 
> and override enablePOIAlphaMask() in SkDifferentPixelsMetric class

Done.

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js
File tools/skpdiff/diff_viewer.js (right):

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:11: // Custom directive for showing an image that
gets swapped my mouseover.
On 2013/10/21 15:02:53, epoger wrote:
> my -> by?

Done.

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/diff_viewer.js#...
tools/skpdiff/diff_viewer.js:22: var magnifiyContent = false;
On 2013/10/21 15:02:53, epoger wrote:
> magnifiy -> magnify

Done.

[djsollen, 2013-11-06 16:25:12]

I think this is ready for review now.

[epoger, 2013-11-06 18:35:13]

Looks pretty good, just a couple of quibbles remaining.  Ping me for live
discussion if you like.

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

https://codereview.chromium.org/29103005/diff/1/tools/skpdiff/SkDiffContext.c...
tools/skpdiff/SkDiffContext.cpp:89: baseName.insertUnichar(x, '\n');
On 2013/11/06 16:02:30, djsollen wrote:
> On 2013/10/21 15:02:53, epoger wrote:
> > Why do you insert a newline rather than just truncating it?
> 
> SkString doesn't support realloc so if we were to try to truncate it we would
> need to alloc new memory, do a copy, and free the old memory just to save a
few
> bytes.  That seemed like more overhead than it was worth.  

I don't understand how inserting a newline actually truncates the string.  Won't
someone who prints the string get the original string with a carriage return in
the middle?

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

https://codereview.chromium.org/29103005/diff/110001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:87: const size_t maxNameLength =
SkTMin(baseName.size(), testName.size());
Please encapsulate this within a new method, something like this:

SkString getCommonPrefix(const char *s1, const char *s2)

[djsollen, 2013-11-07 19:04:57]

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

https://codereview.chromium.org/29103005/diff/110001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:87: const size_t maxNameLength =
SkTMin(baseName.size(), testName.size());
On 2013/11/06 18:35:14, epoger wrote:
> Please encapsulate this within a new method, something like this:
> 
> SkString getCommonPrefix(const char *s1, const char *s2)

Done.

[epoger, 2013-11-07 19:21:02]

LGTM with or without these final adjustments

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

https://codereview.chromium.org/29103005/diff/280001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:65: const size_t maxNameLength =
SkTMin(a.size(), b.size());
Instead of maxNameLength, maybe maxPrefixLength?

https://codereview.chromium.org/29103005/diff/280001/tools/skpdiff/SkDiffCont...
tools/skpdiff/SkDiffContext.cpp:70: result.printf("%.*s", (int)x, a.c_str());
Maybe instead of SkString.printf(), do this:

result.set(a.c_str(), x);

[djsollen, 2013-11-07 19:24:16]

Committed patchset #6 manually as r12174 (presubmit successful).