Use separate subset directories in skimage.

tools/skimage_main.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "gm_expectations.h"
 #include "SkBitmap.h"
 #include "SkBitmapHasher.h"
@@ skipping 52 matching lines @@
 
 static SkImageDecoder::Format guess_format_from_suffix(const char suffix[]) {
     for (size_t i = 0; i < SK_ARRAY_COUNT(gFormats); i++) {
         if (strcmp(suffix, gFormats[i].fSuffix) == 0) {
             return gFormats[i].fFormat;
         }
     }
     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, '.');
         if (dot != NULL) {
             int32_t index = SkToS32(dot - cstyleDst);
             dst->remove(index, dst->size() - index);
         }
         dst->append(suffix);
     }
 }
@@ skipping 82 matching lines @@
 static void write_expectations(const SkBitmap& bitmap, const char* filename) {
     if (!FLAGS_createExpectationsPath.isEmpty()) {
         // Creates an Expectations object, and add it to the list to write.
         skiagm::Expectations expectation(bitmap);
         Json::Value value = expectation.asJsonValue();
         gExpectationsToWrite[filename] = value;
     }
 }
 
 /**
- *  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.
  *  @return bool True in any of these cases:
  *                  - the bitmap matches the expectation.
  *                  - there is no expectations file.
  *               False in any of these cases:
  *                  - there is an expectations file, but no expectation for this bitmap.
  *                  - there is an expectation for this bitmap, but it did not match.
  *                  - expectation could not be computed from the bitmap.
@@ skipping 27 matching lines @@
     }
 
     if (failureArray != NULL) {
         failureArray->push_back().printf("decoded %s, but the result does not match "
                                          "expectations.",
                                          filename);
     }
     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);
     if (!stream.isValid()) {
         gInvalidStreams.push_back().set(srcPath);
         return;
     }
 
     SkImageDecoder* codec = SkImageDecoder::Factory(&stream);
     if (NULL == codec) {
@@ skipping 39 matching lines @@
                 if (codec->decodeSubset(&bitmapFromDecodeSubset, rect, SkBitmap::kNo_Config)) {
                     SkString subsetName = SkStringPrintf("%s_%s", filename, subsetDim.c_str());
                     if (compare_to_expectations_if_necessary(bitmapFromDecodeSubset,
                                                              subsetName.c_str(),
                                                              &gFailedSubsetDecodes)) {
                         gSuccessfulSubsetDecodes.push_back().printf("Decoded subset %s from %s",
                                                               subsetDim.c_str(), srcPath);
                     }
 
                     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.
         SkImageDecoder::Format format = codec->getFormat();
         if (SkImageDecoder::kUnknown_Format == format) {
             if (stream.rewind()) {
                 format = SkImageDecoder::GetStreamFormat(&stream);
             }
             if (SkImageDecoder::kUnknown_Format == format) {
                 const char* dot = strrchr(srcPath, '.');
@@ skipping 169 matching lines @@
     }
 
     return failed ? -1 : 0;
 }
 
 #if !defined SK_BUILD_FOR_IOS
 int main(int argc, char * const argv[]) {
     return tool_main(argc, (char**) argv);
 }
 #endif