Various improvements to CodecSrc testing in dm

dm/DM.cpp

Index: dm/DM.cpp
diff --git a/dm/DM.cpp b/dm/DM.cpp
index f5df692ca66cfce0c700bc02d8afa432f8da083a..47f3650a87aa3e3b8519468f5c45135c7bfcaf25 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -205,6 +205,12 @@ static void push_src(const char* tag, const char* options, Src* s) {
     }
 }
 
+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) {
@@ -218,7 +224,6 @@ static void push_codec_srcs(Path path) {
     }
 
     // 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
@@ -234,30 +239,50 @@ static void push_codec_srcs(Path path) {
             // 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", get_output_dir("codec_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kCodec_Mode,
                         CodecSrc::kIndex8_Always_DstColorType, scale));
-                push_src("image", "scanline_kIndex8", new CodecSrc(path, CodecSrc::kScanline_Mode,
+                push_src("image", get_output_dir("scanline_kIndex8", scaleStr),
+                        new CodecSrc(path, CodecSrc::kScanline_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_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:
@@ -266,18 +291,26 @@ static void push_codec_srcs(Path path) {
         }
 
         // 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", get_output_dir("codec", scaleStr),
+                new CodecSrc(path, CodecSrc::kCodec_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
-        push_src("image", "scanline", new CodecSrc(path, CodecSrc::kScanline_Mode,
+        push_src("image", get_output_dir("scanline", scaleStr),
+                new CodecSrc(path, CodecSrc::kScanline_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
-        push_src("image", "scanline_subset", new CodecSrc(path, CodecSrc::kScanline_Subset_Mode,
+        push_src("image", get_output_dir("scanline_subset", scaleStr),
+                new CodecSrc(path, CodecSrc::kScanline_Subset_Mode,
                 CodecSrc::kGetFromCanvas_DstColorType, scale));
-        push_src("image", "stripe", new CodecSrc(path, CodecSrc::kStripe_Mode,
+        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));
     }
 }