Treat bad values passed to --images as a fatal error
authorscroggo <scroggo@google.com>
Thu, 28 Jan 2016 16:41:10 +0000 (08:41 -0800)
committerCommit bot <commit-bot@chromium.org>
Thu, 28 Jan 2016 16:41:10 +0000 (08:41 -0800)
If an option is passed to --images that is either a non-existent path or
a folder with no images matching the supported types, assume this is
an error and exit, so they can supply a valid path instead.

Share code between DM and nanobench in SkCommonFlags.

nanobench now behaves more like DM - it will check a directory for
images that match the supported extensions.

Only consider image paths ending in RAW suffixes as images if
SK_CODE_DECODES_RAW is defined. This prevents us from seeing failure
to decode errors on platforms that cannot decode it.

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1611323004

Review URL: https://codereview.chromium.org/1611323004

bench/nanobench.cpp
dm/DM.cpp
gyp/codec.gyp
gyp/common_conditions.gypi
gyp/common_variables.gypi
tools/flags/SkCommonFlags.cpp
tools/flags/SkCommonFlags.h

index 909ed0d..c07bfa0 100644 (file)
@@ -582,19 +582,8 @@ public:
         fUseMPDs.push_back() = false;
 
         // Prepare the images for decoding
-        for (int i = 0; i < FLAGS_images.count(); i++) {
-            const char* flag = FLAGS_images[i];
-            if (sk_isdir(flag)) {
-                // If the value passed in is a directory, add all the images
-                SkOSFile::Iter it(flag);
-                SkString file;
-                while (it.next(&file)) {
-                    fImages.push_back() = SkOSPath::Join(flag, file.c_str());
-                }
-            } else if (sk_exists(flag)) {
-                // Also add the value if it is a single image
-                fImages.push_back() = flag;
-            }
+        if (!CollectImages(&fImages)) {
+            exit(1);
         }
 
         // Choose the candidate color types for image decoding
index d53402e..715c4ca 100644 (file)
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -519,7 +519,7 @@ static bool brd_supported(const char* ext) {
     return false;
 }
 
-static void gather_srcs() {
+static bool gather_srcs() {
     for (const skiagm::GMRegistry* r = skiagm::GMRegistry::Head(); r; r = r->next()) {
         push_src("gm", "", new GMSrc(r->factory()));
     }
@@ -534,31 +534,25 @@ static void gather_srcs() {
             push_src("skp", "", new SKPSrc(path));
         }
     }
-    static const char* const exts[] = {
-        "bmp", "gif", "jpg", "jpeg", "png", "webp", "ktx", "astc", "wbmp", "ico",
-        "BMP", "GIF", "JPG", "JPEG", "PNG", "WEBP", "KTX", "ASTC", "WBMP", "ICO",
-        "arw", "cr2", "dng", "nef", "nrw", "orf", "raf", "rw2", "pef", "srw",
-        "ARW", "CR2", "DNG", "NEF", "NRW", "ORF", "RAF", "RW2", "PEF", "SRW",
-    };
-    for (int i = 0; i < FLAGS_images.count(); i++) {
-        const char* flag = FLAGS_images[i];
-        if (sk_isdir(flag)) {
-            for (size_t j = 0; j < SK_ARRAY_COUNT(exts); j++) {
-                SkOSFile::Iter it(flag, exts[j]);
-                for (SkString file; it.next(&file); ) {
-                    SkString path = SkOSPath::Join(flag, file.c_str());
-                    push_codec_srcs(path);
-                    if (brd_supported(exts[j])) {
-                        push_brd_srcs(path);
-                    }
-                }
-            }
-        } else if (sk_exists(flag)) {
-            // assume that FLAGS_images[i] is a valid image if it is a file.
-            push_codec_srcs(flag);
-            push_brd_srcs(flag);
+
+    SkTArray<SkString> images;
+    if (!CollectImages(&images)) {
+        return false;
+    }
+
+    for (auto image : images) {
+        push_codec_srcs(image);
+        const char* ext = "";
+        int index = image.findLastOf('.');
+        if (index >= 0 && (size_t) ++index < image.size()) {
+            ext = &image.c_str()[index];
+        }
+        if (brd_supported(ext)) {
+            push_brd_srcs(image);
         }
     }
+
+    return true;
 }
 
 static void push_sink(const SkCommandLineConfig& config, Sink* s) {
@@ -1106,7 +1100,9 @@ int dm_main() {
     gather_gold();
     gather_uninteresting_hashes();
 
-    gather_srcs();
+    if (!gather_srcs()) {
+        return 1;
+    }
     gather_sinks();
     gather_tests();
 
index 596270a..2043b35 100644 (file)
@@ -68,8 +68,7 @@
         'TURBO_HAS_SKIP',
       ],
       'conditions': [
-        # FIXME: fix the support for ChromeOS [DNG SDK issue with clock_gettime()].
-        ['skia_codec_decodes_raw and skia_os != "chromeos"', {
+        ['skia_codec_decodes_raw', {
           'dependencies': [
             'raw_codec',
           ],
@@ -77,7 +76,7 @@
       ],
     }, {
       # RAW codec needs exceptions. Due to that, it is a separate target. Its usage can be
-      # controlled by SK_CODEC_DECODES_RAW flag.
+      # controlled by skia_codec_decodes_raw flag.
       'target_name': 'raw_codec',
       'product_name': 'raw_codec',
       'type': 'static_library',
         'include_dirs': [
           '../include/codec',
         ],
-        'defines': [
-          'SK_CODEC_DECODES_RAW',
-        ],
       },
-      'defines': [
-        'SK_CODEC_DECODES_RAW',
-      ],
       'conditions': [
         ['skia_arch_type == "x86" or skia_arch_type == "arm"', {
           'defines': [
index c7b19de..11f98bd 100644 (file)
     'SK_FORCE_DISTANCE_FIELD_TEXT=<(skia_force_distance_field_text)',
   ],
   'conditions' : [
+    [ 'skia_codec_decodes_raw', {
+      'defines': [
+        'SK_CODEC_DECODES_RAW',
+      ],
+    }],
     ['skia_pic', {
      'cflags': [
        '-fPIC',
index abbe729..17c5364 100644 (file)
   'variables': {  # level 1
     'angle_path%': '../',
 
-    # RAW codec needs exceptions. Due to that, it is a separate target. Its usage can be controlled
-    # by this variable.
-    'skia_codec_decodes_raw%': 1,
-
     'variables': {  # level 2
 
       # Variables needed by conditions list within the level-2 variables dict.
           }, {
             'skia_arch_type%': 'x86',
           }],
+          # RAW codec needs exceptions. Due to that, it is a separate target. Its usage can be
+          # controlled by skia_codec_decodes_raw.
+          ['skia_os == "chromeos"', {
+            # FIXME: fix the support for ChromeOS [DNG SDK issue with clock_gettime()].
+            'skia_codec_decodes_raw%' : 0,
+          }, {
+            'skia_codec_decodes_raw%' : 1,
+          }],
         ],
         'arm_version%': 0,
         'arm_neon%': 0,
@@ -72,6 +76,7 @@
       # so that siblings of the level-2 'variables' dict can see them.
       # (skia_os will depend on skia_android_framework.)
       'skia_android_framework%': '<(skia_android_framework)',
+      'skia_codec_decodes_raw%': '<(skia_codec_decodes_raw)',
       'skia_arch_type%': '<(skia_arch_type)',
       'arm_version%': '<(arm_version)',
       'arm_neon%': '<(arm_neon)',
     'skia_gpu_extra_tests_path%': '<(skia_gpu_extra_tests_path)',
     'skia_stroke_path_rendering%': '<(skia_stroke_path_rendering)',
     'skia_android_framework%': '<(skia_android_framework)',
+    'skia_codec_decodes_raw%': '<(skia_codec_decodes_raw)',
     'skia_use_android_framework_defines%': '<(skia_use_android_framework_defines)',
     'skia_use_system_json%': '<(skia_use_system_json)',
     'skia_android_path_rendering%': '<(skia_android_path_rendering)',
index 2d5a855..e6d269b 100644 (file)
@@ -6,6 +6,7 @@
  */
 
 #include "SkCommonFlags.h"
+#include "SkOSFile.h"
 
 DEFINE_bool(cpu, true, "master switch for running CPU-bound work.");
 
@@ -14,7 +15,8 @@ DEFINE_bool(dryRun, false,
 
 DEFINE_bool(gpu, true, "master switch for running GPU-bound work.");
 
-DEFINE_string(images, "", "Directory of images to decode.");
+DEFINE_string(images, "", "List of images and/or directories to decode. A directory with no images"
+                          " is treated as a fatal error.");
 
 DEFINE_string2(match, m, nullptr,
                "[~][^]substring[$] [...] of GM name to run.\n"
@@ -47,5 +49,46 @@ DEFINE_string(key, "",
               "Space-separated key/value pairs to add to JSON identifying this builder.");
 DEFINE_string(properties, "",
               "Space-separated key/value pairs to add to JSON identifying this run.");
-
 DEFINE_bool2(pre_log, p, false, "Log before running each test. May be incomprehensible when threading");
+
+bool CollectImages(SkTArray<SkString>* output) {
+    SkASSERT(output);
+
+    static const char* const exts[] = {
+        "bmp", "gif", "jpg", "jpeg", "png", "webp", "ktx", "astc", "wbmp", "ico",
+        "BMP", "GIF", "JPG", "JPEG", "PNG", "WEBP", "KTX", "ASTC", "WBMP", "ICO",
+#ifdef SK_CODEC_DECODES_RAW
+        "arw", "cr2", "dng", "nef", "nrw", "orf", "raf", "rw2", "pef", "srw",
+        "ARW", "CR2", "DNG", "NEF", "NRW", "ORF", "RAF", "RW2", "PEF", "SRW",
+#endif
+    };
+
+    for (int i = 0; i < FLAGS_images.count(); ++i) {
+        const char* flag = FLAGS_images[i];
+        if (!sk_exists(flag)) {
+            SkDebugf("%s does not exist!\n", flag);
+            return false;
+        }
+
+        if (sk_isdir(flag)) {
+            // If the value passed in is a directory, add all the images
+            bool foundAnImage = false;
+            for (const char* ext : exts) {
+                SkOSFile::Iter it(flag, ext);
+                SkString file;
+                while (it.next(&file)) {
+                    foundAnImage = true;
+                    output->push_back() = SkOSPath::Join(flag, file.c_str());
+                }
+            }
+            if (!foundAnImage) {
+                SkDebugf("No supported images found in %s!\n", flag);
+                return false;
+            }
+        } else {
+            // Also add the value if it is a single image
+            output->push_back() = flag;
+        }
+    }
+    return true;
+}
index 127d373..8e00e75 100644 (file)
@@ -9,6 +9,8 @@
 #define SK_COMMON_FLAGS_H
 
 #include "SkCommandLineFlags.h"
+#include "SkTArray.h"
+#include "SkString.h"
 
 DECLARE_bool(cpu);
 DECLARE_bool(dryRun);
@@ -30,4 +32,18 @@ DECLARE_bool(pre_log);
 DECLARE_string(key);
 DECLARE_string(properties);
 
+/**
+ *  Helper to assist in collecting image paths from --images.
+ *
+ *  Populates an array of strings with paths to images to test.
+ *
+ *  Returns true if each argument to --images is meaningful:
+ *  - If the file/directory does not exist, return false.
+ *  - If a directory passed to --images does not have any supported images (based on file
+ *  type), return false.
+ *  - If a file is passed to --images, assume the user is deliberately testing this image,
+ *  regardless of file type.
+ */
+bool CollectImages(SkTArray<SkString>*);
+
 #endif