Modify skpdiff to write diffs directly to provided directories

tools/skpdiff/SkDiffContext.cpp

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmap.h"
 #include "SkImageDecoder.h"
 #include "SkOSFile.h"
 #include "SkRunnable.h"
 #include "SkSize.h"
 #include "SkStream.h"
 #include "SkTDict.h"
 #include "SkThreadPool.h"
 
 #include "SkDiffContext.h"
 #include "SkImageDiffer.h"
 #include "skpdiff_util.h"
 
+#include <string>
+#include <algorithm>
+
 SkDiffContext::SkDiffContext() {
     fDiffers = NULL;
     fDifferCount = 0;
     fThreadCount = SkThreadPool::kThreadPerCore;
 }
 
 SkDiffContext::~SkDiffContext() {
     if (NULL != fDiffers) {
         SkDELETE_ARRAY(fDiffers);
     }
@@ skipping 10 matching lines @@
         fRgbDiffDir = path;
     }
 }
 
 void SkDiffContext::setWhiteDiffDir(const SkString& path) {
     if (!path.isEmpty() && sk_mkdir(path.c_str())) {
         fWhiteDiffDir = path;
     }
 }
 
+void SkDiffContext::setLongNames(const bool useLongNames) {
+    longNames = useLongNames;
+}
+
 void SkDiffContext::setDiffers(const SkTDArray<SkImageDiffer*>& differs) {
     // Delete whatever the last array of differs was
     if (NULL != fDiffers) {
         SkDELETE_ARRAY(fDiffers);
         fDiffers = NULL;
         fDifferCount = 0;
     }
 
     // Copy over the new differs
     fDifferCount = differs.count();
@@ skipping 11 matching lines @@
             return result;
         }
     }
     if (a.size() > b.size()) {
         return b;
     } else {
         return a;
     }
 }
 
+static SkString sanitize_name(const SkString& str) {
+    std::string result = std::string(str.c_str());
+    std::replace(result.begin(), result.end(), '.', '_');
+
+    return SkString(result.c_str());
+}
+
+static SkString get_combined_name(const SkString& a, const SkString& b) {
+    SkString result = a;
+    result.append("-vs-");

[epoger, 2014/08/11 19:33:28]

Looking at _get_difference_locator() in imagediffdb.py, I see:

"We must keep this function in sync with getImageDiffRelativeUrl() in
static/loader.js"

I suspect that would be the case here too, right?

I worry that this change makes our code yet more fragile... now this naming
convention must be kept in sync between loader.js and this native skpdiff code
(which is even further away from loader.js than imagediffdb.py is)

Could we instead continue to construct the filename in imagediffdb.py, and pass
the fully constructed filename into skpdiff?  I think that would be more
robust...

[stephana, 2014/08/11 20:03:57]

Yes, I just realized that it breaks the front-end code. 
Ok, I'll construct the filename in imagediffdb.py and pass it to skpdiff. 

But I would also advocate to pass the relative URL of the diffs from the backend
to the frontend, instead of generating the name on the frontend. Getting rid of
that fragility. 

On 2014/08/11 19:33:28, epoger wrote:

+    result.append(b);
+    return sanitize_name(result);
+}

[stephana, 2014/08/11 14:58:31]

This uses the <string> and <algorithm> headers included above. I didn't see a
lot of use of the C++ standard library in the Skia source - is that something to
be avoided ? 

[epoger, 2014/08/11 19:33:28]

A fine question... in my experience, within the Skia code we have always favored
our homebrew library (SkString and such) rather than bringing in the C++
standard library.

In a tool like skpdiff (as opposed to the core API), it probably doesn't matter
either way... but I guess for the core API we don't want to depend on the C++
std library.

I recommend you ask Mike Reed for guidance on this.

In this particular case, it seems like SkString should offer a replace() method.
 Searching through our code (git grep replace | grep cpp), I find several
implementations:

src/utils/SkRTConf.cpp : static inline void str_replace(char *s, char search,
char replace)
tools/picture_utils.cpp : void replace_char(SkString* str, const char oldChar,
const char newChar)
tools/skimage_main.cpp : static void replace_char(SkString* str, const char
oldChar, const char newChar)

[stephana, 2014/08/11 20:03:57]

Make sense, I'll consult with Mike about this. 

On 2014/08/11 19:33:28, epoger wrote:

+
 void SkDiffContext::addDiff(const char* baselinePath, const char* testPath) {
     // Load the images at the paths
     SkBitmap baselineBitmap;
     SkBitmap testBitmap;
     if (!SkImageDecoder::DecodeFile(baselinePath, &baselineBitmap)) {
         SkDebugf("Failed to load bitmap \"%s\"\n", baselinePath);
         return;
     }
     if (!SkImageDecoder::DecodeFile(testPath, &testBitmap)) {
         SkDebugf("Failed to load bitmap \"%s\"\n", testPath);
         return;
     }
 
     // Setup a record for this diff
     fRecordMutex.acquire();
     DiffRecord* newRecord = fRecords.addToHead(DiffRecord());
     fRecordMutex.release();
 
     // compute the common name
     SkString baseName = SkOSPath::Basename(baselinePath);
     SkString testName = SkOSPath::Basename(testPath);
-    newRecord->fCommonName = get_common_prefix(baseName, testName);
+
+    if (longNames) {
+        newRecord->fCommonName = get_combined_name(baseName, testName);
+    } else {
+        newRecord->fCommonName = get_common_prefix(baseName, testName);
+    }
+    newRecord->fCommonName.append(".png");
 
     newRecord->fBaselinePath = baselinePath;
     newRecord->fTestPath = testPath;
     newRecord->fSize = SkISize::Make(baselineBitmap.width(), baselineBitmap.height());
 
     // only generate diff images if we have a place to store them
     SkImageDiffer::BitmapsToCreate bitmapsToCreate;
     bitmapsToCreate.alphaMask = !fAlphaMaskDir.isEmpty();
     bitmapsToCreate.rgbDiff = !fRgbDiffDir.isEmpty();
     bitmapsToCreate.whiteDiff = !fWhiteDiffDir.isEmpty();
@@ skipping 319 matching lines @@
         for (int i = 0; i < cntColumns; i++) {
             SkString str;
             str.printf(", %f", values[i]);
             stream.writeText(str.c_str());
         }
         stream.writeText("\n");
 
         currentRecord = iter2.next();
     }
 }