Various improvements to CodecSrc testing in dm

dm/DM.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 "CrashHandler.h"
 #include "DMJsonWriter.h"
 #include "DMSrcSink.h"
@@ skipping 187 matching lines @@
     if (in_shard() &&
         FLAGS_src.contains(tag) &&
         !SkCommandLineFlags::ShouldSkip(FLAGS_match, src->name().c_str())) {
         TaggedSrc& s = gSrcs.push_back();
         s.reset(src.detach());
         s.tag = tag;
         s.options = options;
     }
 }
 
+static const char* get_output_dir(const char* mode, const SkString& scale) {
+    SkString* str = new SkString(mode);

[scroggo, 2015/08/31 19:35:24]

This string is leaked.

[msarett, 2015/08/31 21:05:28]

Acknowledged.

+    str->append(scale);
+    return str->c_str();

[scroggo, 2015/08/31 19:35:24]

This only works because you leaked str. c_str() returns a pointer to the
internal data. If you had done the following:

SkString str(mode);
str.append(scale);
return str.c_str();

the returned pointer would point to the location in memory where the internal
data used to be.

[msarett, 2015/08/31 21:05:28]

Acknowledged.

+}
+
 static void push_codec_srcs(Path path) {
     SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(path.c_str()));
     if (!encoded) {
         SkDebugf("Couldn't read %s.", path.c_str());
         return;
     }
     SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(encoded));
     if (nullptr == codec.get()) {
         SkDebugf("Couldn't create codec for %s.", path.c_str());
         return;
     }
 
     // Choose scales for scaling tests.
-    // TODO (msarett): Add more scaling tests as we implement more flexible scaling.
     // TODO (msarett): Implement scaling tests for SkImageDecoder in order to compare with these
     //                 tests.  SkImageDecoder supports downscales by integer factors.
     // SkJpegCodec natively supports scaling to: 0.125, 0.25, 0.375, 0.5, 0.625, 0.75, 0.875
     // 0.1, 0.16, 0.2 etc allow us to test SkScaledCodec with sampleSize 10, 6, 5, etc
     // 0.4, 0.7 etc allow to test what happens when the client requests a scale that
     // does not exactly match a sampleSize or native scaling capability
     const float scales[] = { 0.1f, 0.125f, 0.166f, 0.2f, 0.25f, 0.333f, 0.375f, 0.4f, 0.5f, 0.6f,
                              0.625f, 0.750f, 0.8f, 0.875f, 1.0f };
 
     for (float scale : scales) {
         if (scale != 1.0f && (path.endsWith(".webp") || path.endsWith(".WEBP"))) {
             // FIXME: skbug.com/4038 Scaling webp seems to leave some pixels uninitialized/
             // compute their colors based on uninitialized values.
             continue;
         }
+
+        // We will categorize test outputs by scale, mode, and color type.  Start by converting
+        // the scale to a string.
+        SkString scaleStr;
+        if (1.0f != scale) {
+            scaleStr.appendf("_%.3f", scale);
+        }
+
         // Build additional test cases for images that decode natively to non-canvas types
         switch(codec->getInfo().colorType()) {
             case kGray_8_SkColorType:
-                push_src("image", "codec_kGray8", new CodecSrc(path, CodecSrc::kNormal_Mode,
+                push_src("image", get_output_dir("scaled_codec_kGray8", scaleStr),

[scroggo, 2015/08/31 19:35:24]

Why not pas scale directly to get_output_dir? It can then decide whether to
append based on whether it is 1.0f.

[msarett, 2015/08/31 21:05:28]

Acknowledged.

+                        new CodecSrc(path, CodecSrc::kScaledCodec_Mode,
                         CodecSrc::kGrayscale_Always_DstColorType, scale));
-                push_src("image", "scanline_kGray8", new CodecSrc(path, CodecSrc::kScanline_Mode,
+                push_src("image", get_output_dir("codec_kGray8", scaleStr),

[scroggo, 2015/08/31 19:35:24]

This is getting messy. I think we can clean this up with a different helper
function:

push_codec_src(Path path, CodecSrc::Mode mode, CodecSrc::DstColorType dstCT,
               float scale)
{
  SkString folder;
  switch (mode) {
   case CodecSrc::kCodec_Mode:
     folder.append("codec");
     break;
   ...
  }

  switch (dstCT) {
   case CodecSrc::kGrayScale_Always_DstColorType:
     folder.append("_kGray8");
     break;
   ...
  }

  if (scale != 1.0f) {
    folder.append("_%.3f", scale);
  }

  CodecSrc* src = new CodecSrc(path, mode, dstCT, scale);
  push_src("image", folder.c_str(), src);
}

Then this will look something like:

push_codec_src(path, CodecSrc::kScaledCodec_Mode,
               CodecSrc::kGrayscale_Always_DstColorType, scale);
...

Maybe you could put all this repetitive code in a for loop, hitting all the
Modes...

Thoughts? Does that make this cleaner?

[msarett, 2015/08/31 21:05:28]

Yes this is far better!

+                        new CodecSrc(path, CodecSrc::kCodec_Mode,
                         CodecSrc::kGrayscale_Always_DstColorType, scale));
-                push_src("image", "scanline_subset_kGray8", new CodecSrc(path,
-                        CodecSrc::kScanline_Subset_Mode, CodecSrc::kGrayscale_Always_DstColorType,
-                        scale));
-                push_src("image", "stripe_kGray8", new CodecSrc(path, CodecSrc::kStripe_Mode,
+                push_src("image", get_output_dir("scanline_kGray8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kScanline_Mode,
+                        CodecSrc::kGrayscale_Always_DstColorType, scale));
+                push_src("image", get_output_dir("scanline_subset_kGray8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kScanline_Subset_Mode,
+                        CodecSrc::kGrayscale_Always_DstColorType, scale));
+                push_src("image", get_output_dir("stripe_kGray8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kStripe_Mode,
                         CodecSrc::kGrayscale_Always_DstColorType, scale));
                 // Intentional fall through
                 // FIXME: Is this a long term solution for testing wbmps decodes to kIndex8?
                 // Further discussion on this topic is at skbug.com/3683
             case kIndex_8_SkColorType:
-                push_src("image", "codec_kIndex8", new CodecSrc(path, CodecSrc::kNormal_Mode,
+                push_src("image", get_output_dir("scaled_codec_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kScaledCodec_Mode,
                         CodecSrc::kIndex8_Always_DstColorType, scale));
-                push_src("image", "scanline_kIndex8", new CodecSrc(path, CodecSrc::kScanline_Mode,
+                push_src("image", get_output_dir("codec_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kCodec_Mode,
                         CodecSrc::kIndex8_Always_DstColorType, scale));
-                push_src("image", "scanline_subset_kIndex8", new CodecSrc(path,
-                        CodecSrc::kScanline_Subset_Mode, CodecSrc::kIndex8_Always_DstColorType,
-                        scale));
-                push_src("image", "stripe_kIndex8", new CodecSrc(path, CodecSrc::kStripe_Mode,
+                push_src("image", get_output_dir("scanline_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kScanline_Mode,
+                        CodecSrc::kIndex8_Always_DstColorType, scale));
+                push_src("image", get_output_dir("scanline_subset_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kScanline_Subset_Mode,
+                        CodecSrc::kIndex8_Always_DstColorType, scale));
+                push_src("image", get_output_dir("stripe_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kStripe_Mode,
                         CodecSrc::kIndex8_Always_DstColorType, scale));
                 break;
             default:
                 // Do nothing
                 break;
         }
 
         // Decode all images to the canvas color type
-        push_src("image", "codec", new CodecSrc(path, CodecSrc::kNormal_Mode,
+        push_src("image", get_output_dir("scaled_codec", scaleStr),
+                new CodecSrc(path, CodecSrc::kScaledCodec_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
-        push_src("image", "scanline", new CodecSrc(path, CodecSrc::kScanline_Mode,
+        push_src("image", get_output_dir("codec", scaleStr),
+                new CodecSrc(path, CodecSrc::kCodec_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
-        push_src("image", "scanline_subset", new CodecSrc(path, CodecSrc::kScanline_Subset_Mode,
+        push_src("image", get_output_dir("scanline", scaleStr),
+                new CodecSrc(path, CodecSrc::kScanline_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
-        push_src("image", "stripe", new CodecSrc(path, CodecSrc::kStripe_Mode,
+        push_src("image", get_output_dir("scanline_subset", scaleStr),
+                new CodecSrc(path, CodecSrc::kScanline_Subset_Mode,
+                CodecSrc::kGetFromCanvas_DstColorType, scale));
+        push_src("image", get_output_dir("stripe", scaleStr),
+                new CodecSrc(path, CodecSrc::kStripe_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
         // Note: The only codec which supports subsets natively is SkWebpCodec, which will never
         // report kIndex_8 or kGray_8, so there is no need to test kSubset_mode with those color
         // types specifically requested.
-        push_src("image", "codec_subset", new CodecSrc(path, CodecSrc::kSubset_Mode,
+        push_src("image", get_output_dir("codec_subset", scaleStr),
+                new CodecSrc(path, CodecSrc::kSubset_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
     }
 }
 
 static bool codec_supported(const char* ext) {
     // FIXME: Once other versions of SkCodec are available, we can add them to this
     // list (and eventually we can remove this check once they are all supported).
     static const char* const exts[] = {
         "bmp", "gif", "jpg", "jpeg", "png", "ico", "wbmp", "webp",
         "BMP", "GIF", "JPG", "JPEG", "PNG", "ICO", "WBMP", "WEBP",
@@ skipping 622 matching lines @@
     }
     return 0;
 }
 
 #if !defined(SK_BUILD_FOR_IOS)
 int main(int argc, char** argv) {
     SkCommandLineFlags::Parse(argc, argv);
     return dm_main();
 }
 #endif