Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1591073002:
Add ability to fuzz images and skps to fuzz binary (Closed)
DescriptionSeperating our fuzzing binary from DM produces a 50x speed increase for decoding images and a 10x speed increase in decoding/rendering Skps.
This also lets us differentiate between the decoding of Skps and the rendering of them, the latter of which may be more interesting for bugs.
BUG=skia:4800
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1591073002
Committed: https://skia.googlesource.com/skia/+/dba57344090631bba798e64e78f776bf6afba89c
Patch Set 1 #Patch Set 2 : Fixed linking issue #Patch Set 3 : Moved all fuzzes into one #
Total comments: 13
Patch Set 4 : Address comments and change to SkCodec #Patch Set 5 : Fix imports #
Total comments: 28
Patch Set 6 : Address second round of comments #Patch Set 7 : Add dumpPng() #
Total comments: 12
Patch Set 8 : Fix names, static and simplify #
Total comments: 12
Patch Set 9 : More cleanup and docs #Messages
Total messages: 25 (9 generated)
Description was changed from ========== Add fuzzImage binary for maximum fuzzing speed BUG=skia:4800 ========== to ========== Add fuzzImage binary for maximum fuzzing speed BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
kjlubick@google.com changed reviewers: + mtklein@google.com
Description was changed from ========== Add fuzzImage binary for maximum fuzzing speed BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Seperating our fuzzing binary from DM produces a 50x increase for decoding images and a 10x increase in decoding/rendering Skps. This also lets us differentiate between the decoding of Skps and the rendering of them, the latter of which may be more interesting for bugs. BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Seperating our fuzzing binary from DM produces a 50x increase for decoding images and a 10x increase in decoding/rendering Skps. This also lets us differentiate between the decoding of Skps and the rendering of them, the latter of which may be more interesting for bugs. BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Seperating our fuzzing binary from DM produces a 50x speed increase for decoding images and a 10x speed increase in decoding/rendering Skps. This also lets us differentiate between the decoding of Skps and the rendering of them, the latter of which may be more interesting for bugs. BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
scroggo@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode14 fuzz/fuzz.cpp:14: #include "SkImageDecoder.h" We're actually interested in testing SkCodec, not SkImageDecoder.
https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode28 fuzz/fuzz.cpp:28: DEFINE_string2(file, f, "", "The path to a binary file."); What's the difference between -b and -f? Let's pick one. https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode29 fuzz/fuzz.cpp:29: DEFINE_string(mode, "", "The mode in which to treat the binary file. Can be 'image', 'skp'"); DEFINE_string(mode, "api", "How to interpret --bytes, either 'image', 'skp', or 'api'."); https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode30 fuzz/fuzz.cpp:30: DEFINE_bool(debug, false, "If the output of any renderings should be dumped as a png."); DEFINE_string(dump, "", "If not empty, dump 'image' or 'skp' modes as a PNG with this name."); https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode75 fuzz/fuzz.cpp:75: SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(FLAGS_file[0])); It looks like you're copying and pasting. Let's share the logic between API, image, and SKP fuzzes. They all want to read a fuzzed file. I think you'll find this easier to do if you keep everything inside main(). https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode85 fuzz/fuzz.cpp:85: encoded.reset((SkData*)nullptr); // Might as well drop this when we're done with it. Actually, here in this program we probably don't need to bother, and might even consider going the other way, leaking this instead of ever unreffing it. In DM, it's important to free memory as soon as you can (so other threads can use it). It's not important here, as there are no other threads. https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode117 fuzz/fuzz.cpp:117: bitmap.allocN32Pixels(4000, 4000); Probably better to allocate pixels to fit pic->cullRect().roundOut().
I switched to SkCodec, but my test png is not decoding correctly. I'll attach the before and after in a moment. https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode28 fuzz/fuzz.cpp:28: DEFINE_string2(file, f, "", "The path to a binary file."); On 2016/01/19 at 16:17:25, mtklein wrote: > What's the difference between -b and -f? Let's pick one. Done https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode29 fuzz/fuzz.cpp:29: DEFINE_string(mode, "", "The mode in which to treat the binary file. Can be 'image', 'skp'"); On 2016/01/19 at 16:17:25, mtklein wrote: > DEFINE_string(mode, "api", "How to interpret --bytes, either 'image', 'skp', or 'api'."); Done https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode30 fuzz/fuzz.cpp:30: DEFINE_bool(debug, false, "If the output of any renderings should be dumped as a png."); On 2016/01/19 at 16:17:25, mtklein wrote: > DEFINE_string(dump, "", "If not empty, dump 'image' or 'skp' modes as a PNG with this name."); Done. https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode75 fuzz/fuzz.cpp:75: SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(FLAGS_file[0])); On 2016/01/19 at 16:17:25, mtklein wrote: > It looks like you're copying and pasting. Let's share the logic between API, image, and SKP fuzzes. They all want to read a fuzzed file. > > I think you'll find this easier to do if you keep everything inside main(). I'm going to keep the functions here for easier future addition of things and structure. I will group up duplicated effort. https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode85 fuzz/fuzz.cpp:85: encoded.reset((SkData*)nullptr); // Might as well drop this when we're done with it. On 2016/01/19 at 16:17:25, mtklein wrote: > Actually, here in this program we probably don't need to bother, and might even consider going the other way, leaking this instead of ever unreffing it. In DM, it's important to free memory as soon as you can (so other threads can use it). It's not important here, as there are no other threads. Done. https://codereview.chromium.org/1591073002/diff/40001/fuzz/fuzz.cpp#newcode117 fuzz/fuzz.cpp:117: bitmap.allocN32Pixels(4000, 4000); On 2016/01/19 at 16:17:25, mtklein wrote: > Probably better to allocate pixels to fit pic->cullRect().roundOut(). Done.
https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (left): https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#oldcode19 fuzz/fuzz.cpp:19: if (FLAGS_bytes.isEmpty()) { I think you need to rebase your patch. Are you intentionally removing the --bytes == this binary default? https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#oldcode33 fuzz/fuzz.cpp:33: return 0; 1? https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode23 fuzz/fuzz.cpp:23: DEFINE_string2(bytes, b, "", "A path to a file."); ? Do they not contain fuzzed bytes any more? https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode26 fuzz/fuzz.cpp:26: DEFINE_string(mode, "api", "How to interpret --bytes, either 'image', 'skp', or 'api'."); We may want to make this type, not mode, so that we can give it a short version too (-t)? https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode29 fuzz/fuzz.cpp:29: void printUsage(char*); It is a good habit to get into make functions (really, all globals) static when possible. As you've written it here, you're giving off an implicit signal that these functions are defined in another file. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode29 fuzz/fuzz.cpp:29: void printUsage(char*); It's another good habit to take your arguments with as little power as needed. Here, you can pass argv[0] as a const char*. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode30 fuzz/fuzz.cpp:30: int runSingleTest(SkData*); Let's make these more symmetric: static int fuzz_api(SkData*); static int fuzz_img(SkData*); static int fuzz_skp(SkData*); You can interpret an SkData as an SkStream using SkMemoryStream. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode38 fuzz/fuzz.cpp:38: printUsage(argv[0]); You might consider writing this as: static int usage(const char* argv0) { SkDebugf("...", argv0); return 1; } so you can just write return usage(); https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode51 fuzz/fuzz.cpp:51: if (0 == strcmp(FLAGS_mode[0], "api")) { This is fine, but we probably don't need to be this precise. E.g. switch (FLAGS_mode[0][0]) { case 'a': return fuzz_api(bytes); case 'c': case 'i': return fuzz_img(bytes); case 's': return fuzz_skp(bytes); } return usage(); https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode61 fuzz/fuzz.cpp:61: SkDebugf("Usage: %s --mode <mode> -b <path/to/file> [-m pattern]\n", name); It seems like this one would be more useful if defined at the same time it's declared, up a the top of the file near the flags, or even defined inside main(): SkCommandLineFlags::Parse(argc, argv); auto usage = [&] { SkDebugf("...", argv[0]); return 1; }; then later, return usage(); https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode78 fuzz/fuzz.cpp:78: SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(bytes)); This can't really be the easiest way to use SkCodec, right? Perhaps we should leave these as just static int decode_img(SkData* data) { // TODO(scroggo) return 1; } static int decode_skp(SkData* data) { // TODO(mtklein) return 1; } ? https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode154 fuzz/fuzz.cpp:154: canvas.clipRect(kSKPViewport); Let's not do this? If we're allocating space for the whole .skp, let's draw the whole .skp. If not, let's minimize the allocation to fit the intersection of the clip and the .skp's dimensions. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode158 fuzz/fuzz.cpp:158: SkImageEncoder::EncodeFile(FLAGS_dump[0], bitmap, SkImageEncoder::kPNG_Type, 100); Seems like we've got a bunch of common logic for .skps and images. I wonder if we can make them yet more common using SkCodecImageGenerator::NewFromEncodedCodec() / SkImageGenerator::NewFromPicture()? (Could we get Codec in there a few more times? :P)
The CQ bit was checked by mtklein@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591073002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591073002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
> Add ability to fuzz images and skps to fuzz binary Since this is the subject line, but *not* in the description I *think* it will not end up in the commit message. On 2016/01/20 13:08:15, kjlubick wrote: > I switched to SkCodec, but my test png is not decoding correctly. I'll attach > the before and after in a moment. Hmm, I do not see anything obviously wrong in your code. May be a bug with SkCodec. It behaved correctly with SkImageDecoder? Filed skbug.com/4830 https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode38 fuzz/fuzz.cpp:38: printUsage(argv[0]); On 2016/01/20 14:49:06, mtklein wrote: > You might consider writing this as: > > static int usage(const char* argv0) { > SkDebugf("...", argv0); > return 1; > } > > so you can just write > return usage(); SkCommandLineFlags has a "usage" string, but it only gets printed if you run your program with "-h" or "--help". Maybe there should be an option to print it from code? https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode78 fuzz/fuzz.cpp:78: SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(bytes)); On 2016/01/20 14:49:06, mtklein wrote: > This can't really be the easiest way to use SkCodec, right? > > Perhaps we should leave these as just > > static int decode_img(SkData* data) { > // TODO(scroggo) > return 1; > } > > static int decode_skp(SkData* data) { > // TODO(mtklein) > return 1; > } > > ? This looks about right. It's complicated in part by having the option for an index8 bitmap and zeroing the memory.
https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (left): https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#oldcode19 fuzz/fuzz.cpp:19: if (FLAGS_bytes.isEmpty()) { On 2016/01/20 at 14:49:06, mtklein wrote: > I think you need to rebase your patch. > > Are you intentionally removing the --bytes == this binary default? Whoops, just needed a rebase. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#oldcode33 fuzz/fuzz.cpp:33: return 0; On 2016/01/20 at 14:49:06, mtklein wrote: > 1? Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode23 fuzz/fuzz.cpp:23: DEFINE_string2(bytes, b, "", "A path to a file."); On 2016/01/20 at 14:49:06, mtklein wrote: > ? Do they not contain fuzzed bytes any more? Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode26 fuzz/fuzz.cpp:26: DEFINE_string(mode, "api", "How to interpret --bytes, either 'image', 'skp', or 'api'."); On 2016/01/20 at 14:49:06, mtklein wrote: > We may want to make this type, not mode, so that we can give it a short version too (-t)? Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode29 fuzz/fuzz.cpp:29: void printUsage(char*); On 2016/01/20 at 14:49:06, mtklein wrote: > It's another good habit to take your arguments with as little power as needed. Here, you can pass argv[0] as a const char*. Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode30 fuzz/fuzz.cpp:30: int runSingleTest(SkData*); On 2016/01/20 at 14:49:06, mtklein wrote: > Let's make these more symmetric: > > static int fuzz_api(SkData*); > static int fuzz_img(SkData*); > static int fuzz_skp(SkData*); > > You can interpret an SkData as an SkStream using SkMemoryStream. Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode38 fuzz/fuzz.cpp:38: printUsage(argv[0]); On 2016/01/20 at 14:49:06, mtklein wrote: > You might consider writing this as: > > static int usage(const char* argv0) { > SkDebugf("...", argv0); > return 1; > } > > so you can just write > return usage(); Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode38 fuzz/fuzz.cpp:38: printUsage(argv[0]); On 2016/01/20 at 18:46:19, scroggo wrote: > On 2016/01/20 14:49:06, mtklein wrote: > > You might consider writing this as: > > > > static int usage(const char* argv0) { > > SkDebugf("...", argv0); > > return 1; > > } > > > > so you can just write > > return usage(); > > SkCommandLineFlags has a "usage" string, but it only gets printed if you run your program with "-h" or "--help". Maybe there should be an option to print it from code? That would be nice. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode51 fuzz/fuzz.cpp:51: if (0 == strcmp(FLAGS_mode[0], "api")) { On 2016/01/20 at 14:49:06, mtklein wrote: > This is fine, but we probably don't need to be this precise. E.g. > > switch (FLAGS_mode[0][0]) { > case 'a': return fuzz_api(bytes); > case 'c': > case 'i': return fuzz_img(bytes); > case 's': return fuzz_skp(bytes); > } > > return usage(); Done, although I'm not sure what the c is for. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode61 fuzz/fuzz.cpp:61: SkDebugf("Usage: %s --mode <mode> -b <path/to/file> [-m pattern]\n", name); On 2016/01/20 at 14:49:07, mtklein wrote: > It seems like this one would be more useful if defined at the same time it's declared, up a the top of the file near the flags, or even defined inside main(): > > SkCommandLineFlags::Parse(argc, argv); > auto usage = [&] { > SkDebugf("...", argv[0]); > return 1; > }; > > then later, > return usage(); Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode78 fuzz/fuzz.cpp:78: SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(bytes)); On 2016/01/20 at 14:49:06, mtklein wrote: > This can't really be the easiest way to use SkCodec, right? > > Perhaps we should leave these as just > > static int decode_img(SkData* data) { > // TODO(scroggo) > return 1; > } > > static int decode_skp(SkData* data) { > // TODO(mtklein) > return 1; > } > > ? I went over it with scroggo@ yesterday, and this is what I understood to be the proper way. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode154 fuzz/fuzz.cpp:154: canvas.clipRect(kSKPViewport); On 2016/01/20 at 14:49:06, mtklein wrote: > Let's not do this? If we're allocating space for the whole .skp, let's draw the whole .skp. If not, let's minimize the allocation to fit the intersection of the clip and the .skp's dimensions. Done. https://codereview.chromium.org/1591073002/diff/80001/fuzz/fuzz.cpp#newcode158 fuzz/fuzz.cpp:158: SkImageEncoder::EncodeFile(FLAGS_dump[0], bitmap, SkImageEncoder::kPNG_Type, 100); On 2016/01/20 at 14:49:06, mtklein wrote: > Seems like we've got a bunch of common logic for .skps and images. I wonder if we can make them yet more common using SkCodecImageGenerator::NewFromEncodedCodec() / SkImageGenerator::NewFromPicture()? > > (Could we get Codec in there a few more times? :P) Deduped.
https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode30 fuzz/fuzz.cpp:30: DEFINE_string(dump, "", "If not empty, dump 'image' or 'skp' types as a PNG with this name."); This can probably be string2(dump, d, ...) https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode32 fuzz/fuzz.cpp:32: int printUsage(const char* name) { static int print_usage(const char* name) https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode44 fuzz/fuzz.cpp:44: if (FLAGS_bytes.isEmpty() || FLAGS_type.isEmpty()) { if (FLAGS_bytes.isEmpty() || FLAGS_type.isEmpty()) { return printUsage(argv[0]); } const char* path = FLAGS_bytes.isEmpty() ? argv[0] : FLAGS_bytes[0]; This makes no sense. https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode65 fuzz/fuzz.cpp:65: if (!SkCommandLineFlags::ShouldSkip(FLAGS_match, fuzzable.name)) { If we're going to run one, we'd better switch this to exact match. It doesn't make much sense to run just the first that matches when the user doesn't control the order we iterate. I'd suggest DEFINE_string2(name, n, "", "If --type is 'api', run the DEF_FUZZ with this name."); ... if (0 != strcmp(FLAGS_name[0], fuzzable.name)) { ... return 0; } SkDebugf("%s not found", FLAGS_name[0]); return 1; Let's take care to not call these tests. That's what we call unit tests. I'm open to suggestions. https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode76 fuzz/fuzz.cpp:76: void dumpPng(SkBitmap bitmap) { static void dump_png(...) https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode136 fuzz/fuzz.cpp:136: SkAutoTDelete<SkMemoryStream> stream(new SkMemoryStream(bytes)); SkMemoryStream stream(bytes); if (!stream.isValid()) { ... } SkDebugf("Decoding\n"); SkAutoTUnref<SkPicture> pic(SkPicture::CreateFromStream(&stream)); ...
https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode30 fuzz/fuzz.cpp:30: DEFINE_string(dump, "", "If not empty, dump 'image' or 'skp' types as a PNG with this name."); On 2016/01/20 at 20:15:21, mtklein wrote: > This can probably be string2(dump, d, ...) Done. https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode32 fuzz/fuzz.cpp:32: int printUsage(const char* name) { On 2016/01/20 at 20:15:21, mtklein wrote: > static int print_usage(const char* name) Done. https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode44 fuzz/fuzz.cpp:44: if (FLAGS_bytes.isEmpty() || FLAGS_type.isEmpty()) { On 2016/01/20 at 20:15:21, mtklein wrote: > if (FLAGS_bytes.isEmpty() || FLAGS_type.isEmpty()) { > return printUsage(argv[0]); > } > const char* path = FLAGS_bytes.isEmpty() ? argv[0] : FLAGS_bytes[0]; > > This makes no sense. https://goto.google.com/zvalt https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode65 fuzz/fuzz.cpp:65: if (!SkCommandLineFlags::ShouldSkip(FLAGS_match, fuzzable.name)) { On 2016/01/20 at 20:15:21, mtklein wrote: > If we're going to run one, we'd better switch this to exact match. It doesn't make much sense to run just the first that matches when the user doesn't control the order we iterate. > > I'd suggest > > DEFINE_string2(name, n, "", "If --type is 'api', run the DEF_FUZZ with this name."); > ... > if (0 != strcmp(FLAGS_name[0], fuzzable.name)) { > ... > return 0; > } > SkDebugf("%s not found", FLAGS_name[0]); > return 1; > > Let's take care to not call these tests. That's what we call unit tests. I'm open to suggestions. How about just API Fuzz? https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode76 fuzz/fuzz.cpp:76: void dumpPng(SkBitmap bitmap) { On 2016/01/20 at 20:15:21, mtklein wrote: > static void dump_png(...) Done. https://codereview.chromium.org/1591073002/diff/120001/fuzz/fuzz.cpp#newcode136 fuzz/fuzz.cpp:136: SkAutoTDelete<SkMemoryStream> stream(new SkMemoryStream(bytes)); On 2016/01/20 at 20:15:21, mtklein wrote: > SkMemoryStream stream(bytes); > if (!stream.isValid()) { > ... > } > > SkDebugf("Decoding\n"); > SkAutoTUnref<SkPicture> pic(SkPicture::CreateFromStream(&stream)); > ... There is no isValid on SkMemoryStream. I guess if the data is already in memory, no need to check further...
https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode22 fuzz/fuzz.cpp:22: #include <cmath> What's this needed for? https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode24 fuzz/fuzz.cpp:24: __SK_FORCE_IMAGE_DECODER_LINKING; I don't think this is needed for Codec. Try removing? (And SkForceLinking.h above?) https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode27 fuzz/fuzz.cpp:27: DEFINE_string2(name, n, "", "If --type is 'api', run the DEF_FUZZ with this name."); let's write API fuzz here too. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode33 fuzz/fuzz.cpp:33: SkDebugf("Usage: %s -t <type> -b <path/to/file> [-m pattern]\n", name); update? https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode44 fuzz/fuzz.cpp:44: if (FLAGS_type.isEmpty()) { No longer needed? If it's empty or doesn't match one of our expected types, we'll fall through to the printUsage() at the bottom. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode76 fuzz/fuzz.cpp:76: static void dumpPng(SkBitmap bitmap) { static void dump_png(SkBitmap bitmap)
https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode22 fuzz/fuzz.cpp:22: #include <cmath> On 2016/01/20 at 20:39:29, mtklein wrote: > What's this needed for? Deleted. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode24 fuzz/fuzz.cpp:24: __SK_FORCE_IMAGE_DECODER_LINKING; On 2016/01/20 at 20:39:29, mtklein wrote: > I don't think this is needed for Codec. Try removing? (And SkForceLinking.h above?) When dump is specified, if I don't include this linking, the png won't be produced. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode27 fuzz/fuzz.cpp:27: DEFINE_string2(name, n, "", "If --type is 'api', run the DEF_FUZZ with this name."); On 2016/01/20 at 20:39:29, mtklein wrote: > let's write API fuzz here too. Done. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode33 fuzz/fuzz.cpp:33: SkDebugf("Usage: %s -t <type> -b <path/to/file> [-m pattern]\n", name); On 2016/01/20 at 20:39:29, mtklein wrote: > update? Done. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode44 fuzz/fuzz.cpp:44: if (FLAGS_type.isEmpty()) { On 2016/01/20 at 20:39:29, mtklein wrote: > No longer needed? If it's empty or doesn't match one of our expected types, we'll fall through to the printUsage() at the bottom. Good catch. https://codereview.chromium.org/1591073002/diff/140001/fuzz/fuzz.cpp#newcode76 fuzz/fuzz.cpp:76: static void dumpPng(SkBitmap bitmap) { On 2016/01/20 at 20:39:29, mtklein wrote: > static void dump_png(SkBitmap bitmap) Done.
lgtm
The CQ bit was checked by kjlubick@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1591073002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1591073002/160001
Message was sent while issue was closed.
Description was changed from ========== Seperating our fuzzing binary from DM produces a 50x speed increase for decoding images and a 10x speed increase in decoding/rendering Skps. This also lets us differentiate between the decoding of Skps and the rendering of them, the latter of which may be more interesting for bugs. BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Seperating our fuzzing binary from DM produces a 50x speed increase for decoding images and a 10x speed increase in decoding/rendering Skps. This also lets us differentiate between the decoding of Skps and the rendering of them, the latter of which may be more interesting for bugs. BUG=skia:4800 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/dba57344090631bba798e64e78f776bf6afba89c ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/dba57344090631bba798e64e78f776bf6afba89c |
