Use separate subset directories in skimage.

Use separate subset directories in skimage.

Create separate directories for the results of decodeSubset and
extractSubset, and use the same name for each version, so that
they can be compared easily, using skdiff or visual inspection.

R=epoger@google.com

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

[epoger, 2013-05-09 17:45:40]

Looks good on the whole, a couple of small questions...

https://codereview.chromium.org/14672017/diff/1/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14672017/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:305: if (sk_mkdir(dir.c_str())) {
What happens if the mkdir fails?  Does it pretend to have completed
successfully?

https://codereview.chromium.org/14672017/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:311: write_bitmap(outPath.c_str(),
&bitmapFromDecodeSubset);
Instead of the 3 lines, I think you can do it like this:

SkAssertResult(write_bitmap(outPath.c_str(), &bitmapFromDecodeSubset));

See
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTypes.h?r=8...

[scroggo, 2013-05-13 17:34:12]

https://codereview.chromium.org/14672017/diff/1/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14672017/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:305: if (sk_mkdir(dir.c_str())) {
On 2013/05/09 17:45:40, epoger wrote:
> What happens if the mkdir fails?  Does it pretend to have completed
> successfully?

Now it will provide an error.

https://codereview.chromium.org/14672017/diff/1/tools/skimage_main.cpp#newcod...
tools/skimage_main.cpp:311: write_bitmap(outPath.c_str(),
&bitmapFromDecodeSubset);
On 2013/05/09 17:45:40, epoger wrote:
> Instead of the 3 lines, I think you can do it like this:
> 
> SkAssertResult(write_bitmap(outPath.c_str(), &bitmapFromDecodeSubset));
> 
> See
>
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTypes.h?r=8...

Done.

[epoger, 2013-05-14 15:50:31]

https://codereview.chromium.org/14672017/diff/4001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14672017/diff/4001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:343:
gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from "
As we've discussed before, I think it's a lot cleaner to exit early in failure
cases like this:

1. The debug message is constructed much closer to the failed test (not 38 lines
later as in this case)
2. It saves you from multi-level indent hell

Maybe something like this:

/**
 * Copy of the method in gm/gm_expectations.cpp
 * TODO: move it into SkOSFile.h in a future CL
 */
SkString SkPathJoin(const char *rootPath, const char *relativePath) {
  blahblah;
}

bool write_subsets(const char *writePath, SkString *failureReason) {
  if (writePath == NULL) {
    return true;
  }

  SkString dir = SkPathJoin(writePath, "subsets");
  if (!sk_mkdir(dir.c_str())) {
    failureReason.printf("blah blah blah");
    return false;
  }

  blah blah;

  if (!bitmap.extractSubset(&extractedSubset, rect)) {
    failureReason.printf("blah blah blah");
    return false;
  }

  blah blah;
  return true;
}

[scroggo, 2013-05-14 20:13:13]

https://codereview.chromium.org/14672017/diff/4001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14672017/diff/4001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:343:
gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from "
On 2013/05/14 15:50:31, epoger wrote:
> As we've discussed before, I think it's a lot cleaner to exit early in failure
> cases like this:
> 
> 1. The debug message is constructed much closer to the failed test (not 38
lines
> later as in this case)
> 2. It saves you from multi-level indent hell
> 
> Maybe something like this:
> 
> /**
>  * Copy of the method in gm/gm_expectations.cpp
>  * TODO: move it into SkOSFile.h in a future CL
>  */
> SkString SkPathJoin(const char *rootPath, const char *relativePath) {
>   blahblah;
> }
> 
> bool write_subsets(const char *writePath, SkString *failureReason) {
>   if (writePath == NULL) {
>     return true;
>   }
> 
>   SkString dir = SkPathJoin(writePath, "subsets");
>   if (!sk_mkdir(dir.c_str())) {
>     failureReason.printf("blah blah blah");
>     return false;
>   }
> 
>   blah blah;
> 
>   if (!bitmap.extractSubset(&extractedSubset, rect)) {
>     failureReason.printf("blah blah blah");
>     return false;
>   }
> 
>   blah blah;
>   return true;
> }

Done.

[epoger, 2013-05-15 15:42:01]

Definitely easier to follow, thanks!

Going into the weeds, I have a few more comments.  Sorry to drag this out.

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:84: if (NULL == filename || ++filename == '\0') {
should be *++filename ?

When we move this into SkOSFile.h, we should add unittests to catch bugs like
that...

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:264: static void write_subset(const char* writePath,
const char* filename, const char* subsetDim,
Seems like this function may as well return a bool to indicate whether it was
successful... the caller can of course ignore it...

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:268: SkASSERT(writePath != NULL);
I like the precondition checks!

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:272: // Create a subdirectory to hold the results of
decodeSubset.
IMHO, newlines before each comment-delimited block of code would make this
easier to read.

[scroggo, 2013-05-15 18:08:24]

> Sorry to drag this out.

Np. I'd rather this take longer and result in better code!

New patch up, addressing latest comments.

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp
File tools/skimage_main.cpp (right):

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:84: if (NULL == filename || ++filename == '\0') {
On 2013/05/15 15:42:01, epoger wrote:
> should be *++filename ?
> 
> When we move this into SkOSFile.h, we should add unittests to catch bugs like
> that...

Done.

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:264: static void write_subset(const char* writePath,
const char* filename, const char* subsetDim,
On 2013/05/15 15:42:01, epoger wrote:
> Seems like this function may as well return a bool to indicate whether it was
> successful... the caller can of course ignore it...

Done.

https://codereview.chromium.org/14672017/diff/9001/tools/skimage_main.cpp#new...
tools/skimage_main.cpp:272: // Create a subdirectory to hold the results of
decodeSubset.
On 2013/05/15 15:42:01, epoger wrote:
> IMHO, newlines before each comment-delimited block of code would make this
> easier to read.

Done.

[epoger, 2013-05-15 18:09:27]

LGTM!

[scroggo, 2013-05-15 18:14:41]

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