Use separate subset directories in skimage.

tools/skimage_main.cpp

Index: tools/skimage_main.cpp
diff --git a/tools/skimage_main.cpp b/tools/skimage_main.cpp
index e434ff072d6c98d60b3bc7ada6ea61d4cec979af..9904b2e87bcf374c85277d8a416887aba0615d23 100644
--- a/tools/skimage_main.cpp
+++ b/tools/skimage_main.cpp
@@ -70,16 +70,27 @@ static SkImageDecoder::Format guess_format_from_suffix(const char suffix[]) {
     return SkImageDecoder::kUnknown_Format;
 }
 
+/**
+ *  Return the name of the file, ignoring the directory structure.
+ *  Does not create a new string.
+ *  @param fullPath Full path to the file.
+ *  @return string The basename of the file - anything beyond the final slash, or the full name
+ *      if there is no slash.
+ *  TODO: Might this be useful as a utility function in SkOSFile? Would it be more appropriate to
+ *  create a new string?
+ */
+static const char* SkBasename(const char* fullPath) {
+    const char* filename = strrchr(fullPath, SkPATH_SEPARATOR);
+    if (NULL == filename || ++filename == '\0') {

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

should be *++filename ?

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

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

Done.

+        filename = fullPath;
+    }
+    return filename;
+}
+
 static void make_outname(SkString* dst, const char outDir[], const char src[],
                          const char suffix[]) {
-    dst->set(outDir);
-    const char* start = strrchr(src, '/');
-    if (start) {
-        start += 1; // skip the actual last '/'
-    } else {
-        start = src;
-    }
-    dst->append(start);
+    const char* basename = SkBasename(src);
+    dst->set(skiagm::SkPathJoin(outDir, basename));
     if (!dst->endsWith(suffix)) {
         const char* cstyleDst = dst->c_str();
         const char* dot = strrchr(cstyleDst, '.');
@@ -182,23 +193,6 @@ static void write_expectations(const SkBitmap& bitmap, const char* filename) {
 }
 
 /**
- *  Return the name of the file, ignoring the directory structure.
- *  Does not create a new string.
- *  @param fullPath Full path to the file.
- *  @return string The basename of the file - anything beyond the final slash, or the full name
- *      if there is no slash.
- *  TODO: Might this be useful as a utility function in SkOSFile? Would it be more appropriate to
- *  create a new string?
- */
-static const char* SkBasename(const char* fullPath) {
-    const char* filename = strrchr(fullPath, SkPATH_SEPARATOR);
-    if (NULL == filename || ++filename == '\0') {
-        filename = fullPath;
-    }
-    return filename;
-}
-
-/**
  *  Compare against an expectation for this filename, if there is one.
  *  @param bitmap SkBitmap to compare to the expected value.
  *  @param filename String used to find the expected value.
@@ -246,6 +240,70 @@ static bool compare_to_expectations_if_necessary(const SkBitmap& bitmap, const c
     return false;
 }
 
+/**
+ *  Helper function to write a bitmap subset to a file. Only called if subsets were created
+ *  and a writePath was provided. Creates a subdirectory called 'subsets' and writes a PNG to
+ *  that directory. Also creates a subdirectory called 'extracted' and writes a bitmap created
+ *  using extractSubset to a PNG in that directory. Both files will represent the same
+ *  subrectangle and have the same name for comparison.
+ *  @param writePath Parent directory to hold the folders for the PNG files to write. Must
+ *      not be NULL.
+ *  @param filename Basename of the original file. Used to name the new files. Must not be
+ *      NULL.
+ *  @param subsetDim String representing the dimensions of the subset. Used to name the new
+ *      files. Must not be NULL.
+ *  @param bitmapFromDecodeSubset Pointer to SkBitmap created by SkImageDecoder::DecodeSubset,
+ *      using rect as the area to decode.
+ *  @param rect Rectangle of the area decoded into bitmapFromDecodeSubset. Used to call
+ *      extractSubset on originalBitmap to create a bitmap with the same dimensions/pixels as
+ *      bitmapFromDecodeSubset (assuming decodeSubset worked properly).
+ *  @param originalBitmap SkBitmap decoded from the same stream as bitmapFromDecodeSubset,
+ *      using SkImageDecoder::decode to get the entire image. Used to create a PNG file for
+ *      comparison to the PNG created by bitmapFromDecodeSubset.
+ */
+static void write_subset(const char* writePath, const char* filename, const char* subsetDim,

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

Seems like this function may as well return a bool to indicate whether it was
successful... the caller can of course ignore it...

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

Done.

+                          SkBitmap* bitmapFromDecodeSubset, SkIRect rect,
+                          const SkBitmap& originalBitmap) {
+    // All parameters must be valid.
+    SkASSERT(writePath != NULL);

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

I like the precondition checks!

+    SkASSERT(filename != NULL);
+    SkASSERT(subsetDim != NULL);
+    SkASSERT(bitmapFromDecodeSubset != NULL);
+    // Create a subdirectory to hold the results of decodeSubset.

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

IMHO, newlines before each comment-delimited block of code would make this
easier to read.

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

Done.

+    // TODO: Move SkPathJoin into SkOSFile.h
+    SkString dir = skiagm::SkPathJoin(writePath, "subsets");
+    if (!sk_mkdir(dir.c_str())) {
+        gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from %s, but failed to "
+                                                "create a directory to write to.", subsetDim,
+                                                 filename);
+        return;
+    }
+    // Write the subset to a file whose name includes the dimensions.
+    SkString suffix = SkStringPrintf("_%s.png", subsetDim);
+    SkString outPath;
+    make_outname(&outPath, dir.c_str(), filename, suffix.c_str());
+    SkAssertResult(write_bitmap(outPath.c_str(), bitmapFromDecodeSubset));
+    gSuccessfulSubsetDecodes.push_back().printf("\twrote %s", outPath.c_str());
+    // Also use extractSubset from the original for visual comparison.
+    // Write the result to a file in a separate subdirectory.
+    SkBitmap extractedSubset;
+    if (!originalBitmap.extractSubset(&extractedSubset, rect)) {
+        gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from %s, but failed to "
+                                                "extract a similar subset for comparison.",
+                                                subsetDim, filename);
+        return;
+    }
+    SkString dirExtracted = skiagm::SkPathJoin(writePath, "extracted");
+    if (!sk_mkdir(dirExtracted.c_str())) {
+        gFailedSubsetDecodes.push_back().printf("Successfully decoded %s from %s, but failed to "
+                                                "create a directory for extractSubset comparison.",
+                                                subsetDim, filename);
+        return;
+    }
+    make_outname(&outPath, dirExtracted.c_str(), filename, suffix.c_str());
+    SkAssertResult(write_bitmap(outPath.c_str(), &extractedSubset));
+}
+
 static void decodeFileAndWrite(const char srcPath[], const SkString* writePath) {
     SkBitmap bitmap;
     SkFILEStream stream(srcPath);
@@ -305,32 +363,18 @@ static void decodeFileAndWrite(const char srcPath[], const SkString* writePath)
                     }
 
                     write_expectations(bitmapFromDecodeSubset, subsetName.c_str());
-
                     if (writePath != NULL) {
-                        // Write the region to a file whose name includes the dimensions.
-                        SkString suffix = SkStringPrintf("_%s.png", subsetDim.c_str());
-                        SkString outPath;
-                        make_outname(&outPath, writePath->c_str(), srcPath, suffix.c_str());
-                        SkDEBUGCODE(bool success =)
-                        write_bitmap(outPath.c_str(), &bitmapFromDecodeSubset);
-                        SkASSERT(success);
-                        gSuccessfulSubsetDecodes.push_back().printf("\twrote %s", outPath.c_str());
-                        // Also use extractSubset from the original for visual comparison.
-                        SkBitmap extractedSubset;
-                        if (bitmap.extractSubset(&extractedSubset, rect)) {
-                            suffix.printf("_%s_extracted.png", subsetDim.c_str());
-                            make_outname(&outPath, writePath->c_str(), srcPath, suffix.c_str());
-                            SkDEBUGCODE(success =) write_bitmap(outPath.c_str(), &extractedSubset);
-                            SkASSERT(success);
-                        }
+                        write_subset(writePath->c_str(), filename, subsetDim.c_str(),
+                                     &bitmapFromDecodeSubset, rect, bitmap);
                     }
                 } else {
-                    gFailedSubsetDecodes.push_back().printf("Failed to decode region %s from %s\n",
+                    gFailedSubsetDecodes.push_back().printf("Failed to decode region %s from %s",
                                                             subsetDim.c_str(), srcPath);
                 }
             }
         }
     }
+
     if (FLAGS_reencode) {
         // Encode to the format the file was originally in, or PNG if the encoder for the same
         // format is unavailable.