From 99589af4e333422639d7e873207dd323fdd6e808 Mon Sep 17 00:00:00 2001 From: "commit-bot@chromium.org" Date: Tue, 10 Dec 2013 14:53:16 +0000 Subject: [PATCH] Add support for reading a directory of images with --expectations (-r). DM writes out its images in a hierarchy that's a little different than GM, so this can't read GM's output. But it can read its own, written with -w. Example usage: $ out/Release/dm -w /tmp/baseline $ out/Release/dm -r /tmp/baseline -w /tmp/new (and optionally) $ mkdir /tmp/diff; out/Release/skdiff /tmp/baseline /tmp/new /tmp/diff GM's IndividualImageExpectationsSource and Expectations are a little too eager about decoding and hashing the expected images, so I took the opportunity to add DM::Expectations that mostly replaces skiagm::ExpectationsSource and skiagm::Expectations in DM. It mainly exists to move the image decoding and comparison off the main thread, which would otherwise be a major speed bottleneck. I tried to use skiagm code where possible. One notable place where I differed is in this new feature. When -r is a directory of images, DM does no hashing. It considerably faster to read the expected file into an SkBitmap and do a byte-for-byte comparison than to hash the two bitmaps and check those. The example usage above isn't quite working 100% yet. Expectations on some GMs fail, even with no binary change. I haven't pinned down whether this is due to - a bug in DM - flaky GMs - unthreadsafe GMs - flaky image decoding - unthreadsafe image decoding - something else but I intend to. Leon, Derek and I have suspected PNG decoding isn't threadsafe, but are as yet unable to prove it. I also seem to be able to cause malloc to fail on my laptop if I run too many configs at once, though I never seem to be using more than ~1G of RAM. Will track that down too. BUG= R=reed@google.com, bsalomon@google.com Author: mtklein@google.com Review URL: https://codereview.chromium.org/108963002 git-svn-id: http://skia.googlecode.com/svn/trunk@12596 2bbb7eff-a529-9590-31e7-b0007b416f81 --- dm/DM.cpp | 26 ++++--- dm/DMChecksumTask.cpp | 26 ------- dm/DMCpuTask.cpp | 8 +-- dm/DMCpuTask.h | 8 +-- dm/DMExpectations.h | 46 +++++++++++++ dm/DMExpectationsTask.cpp | 21 ++++++ dm/{DMChecksumTask.h => DMExpectationsTask.h} | 16 ++--- dm/DMGpuTask.cpp | 8 +-- dm/DMGpuTask.h | 6 +- dm/DMUtil.cpp | 5 -- dm/DMUtil.h | 3 - dm/DMWriteTask.cpp | 68 ++++++++++++++++--- dm/DMWriteTask.h | 12 ++++ gyp/dm.gyp | 2 +- 14 files changed, 173 insertions(+), 82 deletions(-) delete mode 100644 dm/DMChecksumTask.cpp create mode 100644 dm/DMExpectations.h create mode 100644 dm/DMExpectationsTask.cpp rename dm/{DMChecksumTask.h => DMExpectationsTask.h} (57%) diff --git a/dm/DM.cpp b/dm/DM.cpp index 274ec2a815..65fccc7611 100644 --- a/dm/DM.cpp +++ b/dm/DM.cpp @@ -14,18 +14,18 @@ #include "DMTaskRunner.h" #include "DMCpuTask.h" #include "DMGpuTask.h" +#include "DMWriteTask.h" #include using skiagm::GM; using skiagm::GMRegistry; -using skiagm::Expectations; -using skiagm::ExpectationsSource; -using skiagm::JsonExpectationsSource; DEFINE_int32(cpuThreads, -1, "Threads for CPU work. Default NUM_CPUS."); DEFINE_int32(gpuThreads, 1, "Threads for GPU work."); -DEFINE_string(expectations, "", "Compare generated images against JSON expectations at this path."); +DEFINE_string2(expectations, r, "", + "If a directory, compare generated images against images under this path. " + "If a file, compare generated images against JSON expectations at this path."); DEFINE_string(resources, "resources", "Path to resources directory."); DEFINE_string(match, "", "[~][^]substring[$] [...] of GM name to run.\n" "Multiple matches may be separated by spaces.\n" @@ -50,7 +50,7 @@ static SkString lowercase(SkString s) { static void kick_off_tasks(const SkTDArray& gms, const SkTArray& configs, - const ExpectationsSource& expectations, + const DM::Expectations& expectations, DM::Reporter* reporter, DM::TaskRunner* tasks) { const SkBitmap::Config _565 = SkBitmap::kRGB_565_Config; @@ -107,13 +107,6 @@ static void report_failures(const DM::Reporter& reporter) { } } -class NoExpectations : public ExpectationsSource { -public: - Expectations get(const char* /*testName*/) const SK_OVERRIDE { - return Expectations(); - } -}; - int tool_main(int argc, char** argv); int tool_main(int argc, char** argv) { SkGraphics::Init(); @@ -134,9 +127,14 @@ int tool_main(int argc, char** argv) { } SkDebugf("%d GMs x %d configs\n", gms.count(), configs.count()); - SkAutoTUnref expectations(SkNEW(NoExpectations)); + SkAutoTDelete expectations(SkNEW(DM::NoExpectations)); if (FLAGS_expectations.count() > 0) { - expectations.reset(SkNEW_ARGS(JsonExpectationsSource, (FLAGS_expectations[0]))); + const char* path = FLAGS_expectations[0]; + if (sk_isdir(path)) { + expectations.reset(SkNEW_ARGS(DM::WriteTask::Expectations, (path))); + } else { + expectations.reset(SkNEW_ARGS(DM::JsonExpectations, (path))); + } } DM::Reporter reporter; diff --git a/dm/DMChecksumTask.cpp b/dm/DMChecksumTask.cpp deleted file mode 100644 index 0d9dce7fd1..0000000000 --- a/dm/DMChecksumTask.cpp +++ /dev/null @@ -1,26 +0,0 @@ -#include "DMChecksumTask.h" -#include "DMUtil.h" - -namespace DM { - -ChecksumTask::ChecksumTask(const Task& parent, - skiagm::Expectations expectations, - SkBitmap bitmap) - : Task(parent) - , fName(parent.name()) // Masquerade as parent so failures are attributed to it. - , fExpectations(expectations) - , fBitmap(bitmap) - {} - -void ChecksumTask::draw() { - if (fExpectations.ignoreFailure() || fExpectations.empty()) { - return; - } - - const skiagm::GmResultDigest digest(fBitmap); - if (!fExpectations.match(digest)) { - this->fail(); - } -} - -} // namespace DM diff --git a/dm/DMCpuTask.cpp b/dm/DMCpuTask.cpp index 3f51c8a318..c538f0a6e4 100644 --- a/dm/DMCpuTask.cpp +++ b/dm/DMCpuTask.cpp @@ -1,5 +1,5 @@ #include "DMCpuTask.h" -#include "DMChecksumTask.h" +#include "DMExpectationsTask.h" #include "DMPipeTask.h" #include "DMReplayTask.h" #include "DMSerializeTask.h" @@ -12,14 +12,14 @@ namespace DM { CpuTask::CpuTask(const char* name, Reporter* reporter, TaskRunner* taskRunner, - const skiagm::ExpectationsSource& expectations, + const Expectations& expectations, skiagm::GMRegistry::Factory gmFactory, SkBitmap::Config config) : Task(reporter, taskRunner) , fGMFactory(gmFactory) , fGM(fGMFactory(NULL)) , fName(UnderJoin(fGM->shortName(), name)) - , fExpectations(expectations.get(Png(fName).c_str())) + , fExpectations(expectations) , fConfig(config) {} @@ -33,7 +33,7 @@ void CpuTask::draw() { canvas.flush(); #define SPAWN(ChildTask, ...) this->spawnChild(SkNEW_ARGS(ChildTask, (*this, __VA_ARGS__))) - SPAWN(ChecksumTask, fExpectations, bitmap); + SPAWN(ExpectationsTask, fExpectations, bitmap); SPAWN(PipeTask, fGMFactory(NULL), bitmap, false, false); SPAWN(PipeTask, fGMFactory(NULL), bitmap, true, false); diff --git a/dm/DMCpuTask.h b/dm/DMCpuTask.h index 998ed7ba4d..c1ee7152c9 100644 --- a/dm/DMCpuTask.h +++ b/dm/DMCpuTask.h @@ -1,6 +1,7 @@ #ifndef DMCpuTask_DEFINED #define DMCpuTask_DEFINED +#include "DMExpectations.h" #include "DMReporter.h" #include "DMTask.h" #include "DMTaskRunner.h" @@ -8,12 +9,9 @@ #include "SkString.h" #include "SkTemplates.h" #include "gm.h" -#include "gm_expectations.h" // This is the main entry point for drawing GMs with the CPU. Commandline // flags control whether this kicks off various comparison tasks when done. -// Currently: -// --replay: spawn a DMReplayTask to record into a picture, draw the picture, and compare. namespace DM { @@ -22,7 +20,7 @@ public: CpuTask(const char* name, Reporter*, TaskRunner*, - const skiagm::ExpectationsSource&, + const Expectations&, skiagm::GMRegistry::Factory, SkBitmap::Config); @@ -35,7 +33,7 @@ private: skiagm::GMRegistry::Factory fGMFactory; SkAutoTDelete fGM; const SkString fName; - const skiagm::Expectations fExpectations; + const Expectations& fExpectations; const SkBitmap::Config fConfig; }; diff --git a/dm/DMExpectations.h b/dm/DMExpectations.h new file mode 100644 index 0000000000..238d1c5bea --- /dev/null +++ b/dm/DMExpectations.h @@ -0,0 +1,46 @@ +#ifndef DMExpectations_DEFINED +#define DMExpectations_DEFINED + +#include "DMTask.h" +#include "gm_expectations.h" + +namespace DM { + +struct Expectations { + virtual ~Expectations() {} + + // Return true if bitmap is the correct output for task, else false. + virtual bool check(const Task& task, SkBitmap bitmap) const = 0; +}; + +class NoExpectations : public Expectations { +public: + NoExpectations() {} + bool check(const Task&, SkBitmap) const SK_OVERRIDE { return true; } +}; + +class JsonExpectations : public Expectations { +public: + explicit JsonExpectations(const char* path) : fGMExpectations(path) {} + + bool check(const Task& task, SkBitmap bitmap) const SK_OVERRIDE { + SkString filename = task.name(); + filename.append(".png"); + const skiagm::Expectations expectations = fGMExpectations.get(filename.c_str()); + + if (expectations.ignoreFailure() || expectations.empty()) { + return true; + } + + // Delay this calculation as long as possible. It's expensive. + const skiagm::GmResultDigest digest(bitmap); + return expectations.match(digest); + } + +private: + skiagm::JsonExpectationsSource fGMExpectations; +}; + +} // namespace DM + +#endif // DMExpectations_DEFINED diff --git a/dm/DMExpectationsTask.cpp b/dm/DMExpectationsTask.cpp new file mode 100644 index 0000000000..cb92486269 --- /dev/null +++ b/dm/DMExpectationsTask.cpp @@ -0,0 +1,21 @@ +#include "DMExpectationsTask.h" +#include "DMUtil.h" + +namespace DM { + +ExpectationsTask::ExpectationsTask(const Task& parent, + const Expectations& expectations, + SkBitmap bitmap) + : Task(parent) + , fName(parent.name()) // Masquerade as parent so failures are attributed to it. + , fExpectations(expectations) + , fBitmap(bitmap) + {} + +void ExpectationsTask::draw() { + if (!fExpectations.check(*this, fBitmap)) { + this->fail(); + } +} + +} // namespace DM diff --git a/dm/DMChecksumTask.h b/dm/DMExpectationsTask.h similarity index 57% rename from dm/DMChecksumTask.h rename to dm/DMExpectationsTask.h index b0afac9467..cf76fc8bdf 100644 --- a/dm/DMChecksumTask.h +++ b/dm/DMExpectationsTask.h @@ -1,18 +1,18 @@ -#ifndef DMChecksumTask_DEFINED -#define DMChecksumTask_DEFINED +#ifndef DMExpectationsTask_DEFINED +#define DMExpectationsTask_DEFINED +#include "DMExpectations.h" #include "DMTask.h" #include "SkBitmap.h" #include "SkString.h" -#include "gm_expectations.h" namespace DM { -// ChecksumTask compares an SkBitmap against some Expectations. +// ExpectationsTask compares an SkBitmap against some Expectations. // Moving this off the GPU threadpool is a nice (~30%) runtime win. -class ChecksumTask : public Task { +class ExpectationsTask : public Task { public: - ChecksumTask(const Task& parent, skiagm::Expectations, SkBitmap); + ExpectationsTask(const Task& parent, const Expectations&, SkBitmap); virtual void draw() SK_OVERRIDE; virtual bool usesGpu() const SK_OVERRIDE { return false; } @@ -21,10 +21,10 @@ public: private: const SkString fName; - const skiagm::Expectations fExpectations; + const Expectations& fExpectations; const SkBitmap fBitmap; }; } // namespace DM -#endif // DMChecksumTask_DEFINED +#endif // DMExpectationsTask_DEFINED diff --git a/dm/DMGpuTask.cpp b/dm/DMGpuTask.cpp index d4c4254c62..c0502eeed5 100644 --- a/dm/DMGpuTask.cpp +++ b/dm/DMGpuTask.cpp @@ -1,6 +1,6 @@ #include "DMGpuTask.h" -#include "DMChecksumTask.h" +#include "DMExpectationsTask.h" #include "DMUtil.h" #include "DMWriteTask.h" #include "SkCommandLineFlags.h" @@ -12,7 +12,7 @@ namespace DM { GpuTask::GpuTask(const char* name, Reporter* reporter, TaskRunner* taskRunner, - const skiagm::ExpectationsSource& expectations, + const Expectations& expectations, skiagm::GMRegistry::Factory gmFactory, SkBitmap::Config config, GrContextFactory::GLContextType contextType, @@ -20,7 +20,7 @@ GpuTask::GpuTask(const char* name, : Task(reporter, taskRunner) , fGM(gmFactory(NULL)) , fName(UnderJoin(fGM->shortName(), name)) - , fExpectations(expectations.get(Png(fName).c_str())) + , fExpectations(expectations) , fConfig(config) , fContextType(contextType) , fSampleCount(sampleCount) @@ -60,7 +60,7 @@ void GpuTask::draw() { gr->printCacheStats(); #endif - this->spawnChild(SkNEW_ARGS(ChecksumTask, (*this, fExpectations, bitmap))); + this->spawnChild(SkNEW_ARGS(ExpectationsTask, (*this, fExpectations, bitmap))); this->spawnChild(SkNEW_ARGS(WriteTask, (*this, bitmap))); } diff --git a/dm/DMGpuTask.h b/dm/DMGpuTask.h index 87c530b825..aa350c9d99 100644 --- a/dm/DMGpuTask.h +++ b/dm/DMGpuTask.h @@ -1,6 +1,7 @@ #ifndef DMGpuTask_DEFINED #define DMGpuTask_DEFINED +#include "DMExpectations.h" #include "DMReporter.h" #include "DMTask.h" #include "DMTaskRunner.h" @@ -9,7 +10,6 @@ #include "SkString.h" #include "SkTemplates.h" #include "gm.h" -#include "gm_expectations.h" // This is the main entry point for drawing GMs with the GPU. @@ -20,7 +20,7 @@ public: GpuTask(const char* name, Reporter*, TaskRunner*, - const skiagm::ExpectationsSource&, + const Expectations&, skiagm::GMRegistry::Factory, SkBitmap::Config, GrContextFactory::GLContextType, @@ -34,7 +34,7 @@ public: private: SkAutoTDelete fGM; const SkString fName; - const skiagm::Expectations fExpectations; + const Expectations& fExpectations; const SkBitmap::Config fConfig; const GrContextFactory::GLContextType fContextType; const int fSampleCount; diff --git a/dm/DMUtil.cpp b/dm/DMUtil.cpp index 6cf6c22e2a..52efda3d32 100644 --- a/dm/DMUtil.cpp +++ b/dm/DMUtil.cpp @@ -10,11 +10,6 @@ SkString UnderJoin(const char* a, const char* b) { return s; } -SkString Png(SkString s) { - s.appendf(".png"); - return s; -} - void RecordPicture(skiagm::GM* gm, SkPicture* picture, uint32_t recordFlags) { const SkISize size = gm->getISize(); SkCanvas* canvas = picture->beginRecording(size.width(), size.height(), recordFlags); diff --git a/dm/DMUtil.h b/dm/DMUtil.h index 5f22df0878..b887d828d4 100644 --- a/dm/DMUtil.h +++ b/dm/DMUtil.h @@ -12,9 +12,6 @@ namespace DM { // UnderJoin("a", "b") -> "a_b" SkString UnderJoin(const char* a, const char* b); -// Png("a") -> "a.png" -SkString Png(SkString s); - // Draw gm to picture. Passes recordFlags to SkPicture::beginRecording(). void RecordPicture(skiagm::GM* gm, SkPicture* picture, uint32_t recordFlags = 0); diff --git a/dm/DMWriteTask.cpp b/dm/DMWriteTask.cpp index c86381f78a..11b9fd3470 100644 --- a/dm/DMWriteTask.cpp +++ b/dm/DMWriteTask.cpp @@ -2,6 +2,7 @@ #include "DMUtil.h" #include "SkCommandLineFlags.h" +#include "SkImageDecoder.h" #include "SkImageEncoder.h" #include "SkString.h" @@ -9,18 +10,25 @@ DEFINE_string2(writePath, w, "", "If set, write GMs here as .pngs."); namespace DM { -WriteTask::WriteTask(const Task& parent, SkBitmap bitmap) : Task(parent), fBitmap(bitmap) { - const int suffixes = parent.depth() + 1; - const char* name = parent.name().c_str(); +// Splits off the last N suffixes of name (splitting on _) and appends them to out. +// Returns the total number of characters consumed. +static int split_suffixes(int N, const char* name, SkTArray* out) { SkTArray split; SkStrSplit(name, "_", &split); - int totalSuffixLength = 0; - for (int i = 0; i < suffixes; i++) { + int consumed = 0; + for (int i = 0; i < N; i++) { // We're splitting off suffixes from the back to front. - fSuffixes.push_back(split[split.count()-i-1]); - totalSuffixLength += fSuffixes.back().size() + 1; + out->push_back(split[split.count()-i-1]); + consumed += out->back().size() + 1; // Add one for the _. } - fGmName.set(name, strlen(name)-totalSuffixLength); + return consumed; +} + +WriteTask::WriteTask(const Task& parent, SkBitmap bitmap) : Task(parent), fBitmap(bitmap) { + const int suffixes = parent.depth() + 1; + const SkString& name = parent.name(); + const int totalSuffixLength = split_suffixes(suffixes, name.c_str(), &fSuffixes); + fGmName.set(name.c_str(), name.size()-totalSuffixLength); } void WriteTask::makeDirOrFail(SkString dir) { @@ -36,7 +44,9 @@ void WriteTask::draw() { dir = SkOSPath::SkPathJoin(dir.c_str(), fSuffixes[i].c_str()); this->makeDirOrFail(dir); } - if (!SkImageEncoder::EncodeFile(Png(SkOSPath::SkPathJoin(dir.c_str(), fGmName.c_str())).c_str(), + SkString path = SkOSPath::SkPathJoin(dir.c_str(), fGmName.c_str()); + path.append(".png"); + if (!SkImageEncoder::EncodeFile(path.c_str(), fBitmap, SkImageEncoder::kPNG_Type, 100/*quality*/)) { @@ -57,4 +67,44 @@ bool WriteTask::shouldSkip() const { return FLAGS_writePath.isEmpty(); } +static SkString path_to_expected_image(const char* root, const Task& task) { + SkString filename = task.name(); + + // We know that all names passed in here belong to top-level Tasks, which have a single suffix + // (8888, 565, gpu, etc.) indicating what subdirectory to look in. + SkTArray suffixes; + const int suffixLength = split_suffixes(1, filename.c_str(), &suffixes); + SkASSERT(1 == suffixes.count()); + + // We'll look in root/suffix for images. + const SkString dir = SkOSPath::SkPathJoin(root, suffixes[0].c_str()); + + // Remove the suffix and tack on a .png. + filename.remove(filename.size() - suffixLength, suffixLength); + filename.append(".png"); + + //SkDebugf("dir %s, filename %s\n", dir.c_str(), filename.c_str()); + + return SkOSPath::SkPathJoin(dir.c_str(), filename.c_str()); +} + +bool WriteTask::Expectations::check(const Task& task, SkBitmap bitmap) const { + const SkString path = path_to_expected_image(fRoot, task); + + SkBitmap expected; + if (SkImageDecoder::DecodeFile(path.c_str(), &expected)) { + if (expected.config() != bitmap.config()) { + SkBitmap converted; + SkAssertResult(expected.copyTo(&converted, bitmap.config())); + expected.swap(converted); + } + SkASSERT(expected.config() == bitmap.config()); + return BitmapsEqual(expected, bitmap); + } + + // Couldn't read the file, etc. + SkDebugf("Problem decoding %s to SkBitmap.\n", path.c_str()); + return false; +} + } // namespace DM diff --git a/dm/DMWriteTask.h b/dm/DMWriteTask.h index 82a26bc928..49a5c746a6 100644 --- a/dm/DMWriteTask.h +++ b/dm/DMWriteTask.h @@ -1,11 +1,13 @@ #ifndef DMWriteTask_DEFINED #define DMWriteTask_DEFINED +#include "DMExpectations.h" #include "DMTask.h" #include "SkBitmap.h" #include "SkString.h" #include "SkTArray.h" + // Writes a bitmap to a file. namespace DM { @@ -21,6 +23,16 @@ public: virtual bool shouldSkip() const SK_OVERRIDE; virtual SkString name() const SK_OVERRIDE; + // Reads image files WriteTask wrote under root and compares them with bitmap. + class Expectations : public DM::Expectations { + public: + explicit Expectations(const char* root) : fRoot(root) {} + + bool check(const Task& task, SkBitmap bitmap) const SK_OVERRIDE; + private: + const char* fRoot; + }; + private: SkTArray fSuffixes; SkString fGmName; diff --git a/gyp/dm.gyp b/gyp/dm.gyp index 8fa59c545a..e56cf16d65 100644 --- a/gyp/dm.gyp +++ b/gyp/dm.gyp @@ -20,8 +20,8 @@ 'includes': [ 'gmslides.gypi' ], 'sources': [ '../dm/DM.cpp', - '../dm/DMChecksumTask.cpp', '../dm/DMCpuTask.cpp', + '../dm/DMExpectationsTask.cpp', '../dm/DMGpuTask.cpp', '../dm/DMPipeTask.cpp', '../dm/DMReplayTask.cpp', -- 2.34.1