More work to integrate skimage with rebaseline tools.

More work to integrate skimage with rebaseline tools.

tools/skimage_main.cpp:
Add the ability to write the results to checksum based filenames,
much like GM uses. This will allow using the skpdiff server to
rebaseline images.
Write the keys in the JSON file as <original image>_<pref config>.png,
so it matches gm_json.IMAGE_FILENAME_PATTERN. Also replace '_' with
'-' in the original file name, to avoid confusing the pattern matcher.
The '_' to '-' replacement also happens on the output filename.
Read the keys in a similar manner.
In make_outname, no longer remove a suffix. This fixes a bug where
subset decoding writes multiple subsets to the same file.

tools/rebaseline.py:
Since the filenames written to json files now match
gm_json.IMAGE_FILENAME_PATTERN, enable the option to match based
on configs/tests when rebaselining skimage.

test json files:
Update to match the new format of output.

gm/gm_expectations:
Add a constructor that takes a BitmapAndDigest as input.

tools/tests/skimage_self_test.py:
Test that reading the expectations file just created by skimage with
the same file actually compares to the original file (rather than just
succeeding because expectations were missing).

Change the expectations files to match the new format.

Will require a buildbot change to use the new flag: https://codereview.chromium.org/27389002/

BUG=1466
R=epoger@google.com

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

[scroggo, 2013-10-16 23:10:51]

I missed a bug when I first sent you this review. That bug is now fixed, and
ready for you to review.

https://codereview.chromium.org/26297004/diff/15001/gm/gm_expectations.h
File gm/gm_expectations.h (right):

https://codereview.chromium.org/26297004/diff/15001/gm/gm_expectations.h#newc...
gm/gm_expectations.h:124: explicit Expectations(const BitmapAndDigest&
bitmapAndDigest);
For my use case, this could just take a digest. Either way is fine with me.

I also did not add ignoreFailure, since I do not need it, but I'm fine with
adding it for consistency.

https://codereview.chromium.org/26297004/diff/15001/tools/tests/skimage_self_...
File tools/tests/skimage_self_test.py (right):

https://codereview.chromium.org/26297004/diff/15001/tools/tests/skimage_self_...
tools/tests/skimage_self_test.py:155: if "Missing" in output:
These tests catch the error that patchset 5 missed: The format of reading the
expectations file did not match the format of writing it.

[epoger, 2013-10-17 15:54:12]

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:91: // Only remove if this is a 4-5 character suffix
(including '.').
Why this "magic" constraint on the length of the extension to remove?

If we're going to have this constraint, please make it configurable using a
constant (or even better, a flag)

maybe something like this at the top...
DEFINE_int32(maxExtensionLength, 4);

and then...
if (amountToRemove <= (FLAGS_maxExtensionLength+1)) {
  dst->remove(index, amountToRemove);
}

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:140: *  FLAGS_writeChecksumBasedFilenames. If true, the
final image will be
Maybe make writeChecksumBasedFilenames a parameter, for greater
flexibility/reusability?

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:235: *  Return a string which includes the name of the
file and the pref config,
what does "pref" mean?  ("prefix"?  "preference"?  If "preference", a preference
among which choices?)

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:249: *         expectations object, for future writing to
a file, which will later be
I don't think this needs to be fixed within this CL... but it bothers me that we
are using globals.  I think we're better off using member variables in C++
objects.

(Right now, when I see that this method is static, I think "oh goody, it doesn't
depend on any object state".  But it does, really.)

I suggest you add a TODO about this.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:253: *  The two purposes share a function so they can
share filenames and expectations.
IMHO it would be clearer for this to remain as two separate methods.

It seems to me that, if both --createExpectationsPath and --readExpectationsPath
have been set, the only overlap between the two methods would be calling
create_json_key(filename), which doesn't seem that bad.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:254: *  @param bitmapAndDigest BitmapAndDigest, computed
from the decoded bitmap, to both
add newline before first param

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:330: *  FLAGS_writeChecksumBasedFilenames. If true:
Again, rather than a static method depending on global state, I think it would
be preferable to pass in this parameter (or make it a non-static method that
depends on an object variable).

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:339: *  @param writePath Parent directory to hold the
folders for the PNG files to write. Must
newline before first param (just to make it easier to read)

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:473: static void replace_char(SkString* str, const char
oldChar, const char newChar) {
I don't think this needs to happen within this CL, but I think we'd be better
off in the long run if this method lived somewhere (like SkString.h) where it
would be:

1. available for general use
2. unit-tested

I suggest a TODO

https://codereview.chromium.org/26297004/diff/29001/tools/tests/skimage/input...
File tools/tests/skimage/input/bad-images/ignore-results.json (right):

https://codereview.chromium.org/26297004/diff/29001/tools/tests/skimage/input...
tools/tests/skimage/input/bad-images/ignore-results.json:9:
"invalid.png_None.png" : {
is this the expected filename format?  What's with the _None ?

[scroggo, 2013-10-18 21:28:03]

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:91: // Only remove if this is a 4-5 character suffix
(including '.').
On 2013/10/17 15:54:13, epoger wrote:
> Why this "magic" constraint on the length of the extension to remove?
> 
> If we're going to have this constraint, please make it configurable using a
> constant (or even better, a flag)
> 
> maybe something like this at the top...
> DEFINE_int32(maxExtensionLength, 4);
> 
> and then...
> if (amountToRemove <= (FLAGS_maxExtensionLength+1)) {
>   dst->remove(index, amountToRemove);
> }

Here is the motivation: It is used to create the filename of an image which was
sometimes created by taking a subset. The name that gets passed to this function
is something like <original image.png>-[subset dimensions]. In the case of the
subset, I don't want to remove ".png-[subset dimensions]".

That said, you've forced me to rethink this a bit. As currently written,
decoding the file "image.webp" will create a few files. It will create
"image.png", which is the same image decoded to Skia and then reencoded to png.
It will also create various subsets, with names like "image.webp-[L,T,R,B].png".
The second name retains information about the original image (it was a webp
image) and also ensures that if I ran the test on both "image.webp" and
"image.jpg" the output from both images survives. The first name does neither of
these.

So I've updated the code to never remove any suffix, but to instead change the
name of the file to contain the original suffix (after a '-' instead of a '.').
Now "image.webp" begets "image-webp.png" and "image-webp-[L,T,R,B].png". This
also means that "image.jpg" can also create an output without one of them being
overwritten by the other.

It also means that I have removed the magic.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:235: *  Return a string which includes the name of the
file and the pref config,
On 2013/10/17 15:54:13, epoger wrote:
> what does "pref" mean?  ("prefix"?  "preference"?  If "preference", a
preference
> among which choices?)

I've clarified the comment.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:249: *         expectations object, for future writing to
a file, which will later be
On 2013/10/17 15:54:13, epoger wrote:
> I don't think this needs to be fixed within this CL... but it bothers me that
we
> are using globals.  I think we're better off using member variables in C++
> objects.
> 
> (Right now, when I see that this method is static, I think "oh goody, it
doesn't
> depend on any object state".  But it does, really.)
> 
> I suggest you add a TODO about this.

I actually like using globals in this case. If I were to remove the dependence
on global variables, decodeFileAndWrite would basically need to take all of the
flags as parameters, which would be very messy. Alternatively, we could create
an object which holds all the flags, but that still seems like a convoluted way
to do what globals do very easily in this case.

I agree in general that we should avoid global variables, but since this a tool
which does not need to be reused by any other code, the convenience of global
variables outweigh the costs.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:253: *  The two purposes share a function so they can
share filenames and expectations.
On 2013/10/17 15:54:13, epoger wrote:
> IMHO it would be clearer for this to remain as two separate methods.
> 
> It seems to me that, if both --createExpectationsPath and
--readExpectationsPath
> have been set, the only overlap between the two methods would be calling
> create_json_key(filename), which doesn't seem that bad.

Done.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:254: *  @param bitmapAndDigest BitmapAndDigest, computed
from the decoded bitmap, to both
On 2013/10/17 15:54:13, epoger wrote:
> add newline before first param

Done.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:339: *  @param writePath Parent directory to hold the
folders for the PNG files to write. Must
On 2013/10/17 15:54:13, epoger wrote:
> newline before first param (just to make it easier to read)

Done.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:473: static void replace_char(SkString* str, const char
oldChar, const char newChar) {
On 2013/10/17 15:54:13, epoger wrote:
> I don't think this needs to happen within this CL, but I think we'd be better
> off in the long run if this method lived somewhere (like SkString.h) where it
> would be:
> 
> 1. available for general use
> 2. unit-tested
> 
> I suggest a TODO

Done.

https://codereview.chromium.org/26297004/diff/29001/tools/tests/skimage/input...
File tools/tests/skimage/input/bad-images/ignore-results.json (right):

https://codereview.chromium.org/26297004/diff/29001/tools/tests/skimage/input...
tools/tests/skimage/input/bad-images/ignore-results.json:9:
"invalid.png_None.png" : {
On 2013/10/17 15:54:13, epoger wrote:
> is this the expected filename format?  What's with the _None ?

That comes from the "--config" flag, which I've run with the default.

[epoger, 2013-10-21 15:13:39]

LGTM with or without a couple of small modifications

Thanks for the explanations!

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:91: // Only remove if this is a 4-5 character suffix
(including '.').
On 2013/10/18 21:28:03, scroggo wrote:
> It also means that I have removed the magic.

Thank you for:
1. removing the magic
2. the cogent explanation
3. using the word "beget"

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:140: *  FLAGS_writeChecksumBasedFilenames. If true, the
final image will be
On 2013/10/17 15:54:13, epoger wrote:
> Maybe make writeChecksumBasedFilenames a parameter, for greater
> flexibility/reusability?

I still think it would be better for this to be a parameter, but I won't close
the government over it or anything.

https://codereview.chromium.org/26297004/diff/29001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:249: *         expectations object, for future writing to
a file, which will later be
On 2013/10/18 21:28:03, scroggo wrote:
> I actually like using globals in this case. If I were to remove the dependence
> on global variables, decodeFileAndWrite would basically need to take all of
the
> flags as parameters, which would be very messy. Alternatively, we could create
> an object which holds all the flags, but that still seems like a convoluted
way
> to do what globals do very easily in this case.
> 
> I agree in general that we should avoid global variables, but since this a
tool
> which does not need to be reused by any other code, the convenience of global
> variables outweigh the costs.

Fair enough.  Personally, I would prefer member variables on an object, but I
suppose it's a matter of taste.

https://codereview.chromium.org/26297004/diff/36001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/26297004/diff/36001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:242: // of the file and the pref config, matching the
pattern of gm_json.py's
Again, I pref using the whole word, to aid readability.

Also, I think you should move this comment into the header above the function
declaration.  It's definitely of interest to callers.

[scroggo, 2013-10-21 16:18:04]

https://codereview.chromium.org/26297004/diff/36001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/26297004/diff/36001/tools/skimage_main.cpp#ne...
tools/skimage_main.cpp:242: // of the file and the pref config, matching the
pattern of gm_json.py's
On 2013/10/21 15:13:39, epoger wrote:
> Again, I pref using the whole word, to aid readability.
> 
> Also, I think you should move this comment into the header above the function
> declaration.  It's definitely of interest to callers.

Done.

[epoger, 2013-10-21 16:21:57]

lgtm

[scroggo, 2013-10-22 00:43:24]

Committed patchset #11 manually as r11902 (presubmit successful).