Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1698963003:
Add ability to fuzz images using scaling and different modes (Closed)
DescriptionAdd ability to fuzz images using scaling and different modes
This also fixes the tryAllocPixels/SkColorTable mismatch which was causing the
"Image might be too large (32 x 32)" problems.
BUG=skia:4952
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1698963003
Committed: https://skia.googlesource.com/skia/+/2a42f48b58f11c32017e2da6c8468a3e2cd9bd8c
Patch Set 1 #Patch Set 2 : Add docs #
Total comments: 15
Patch Set 3 : Address feedback (mostly spacing) #Patch Set 4 : fix signalBug goof #Patch Set 5 : Fix pow ambiguity #Messages
Total messages: 24 (11 generated)
Description was changed from ========== Add ability to fuzz images using scaling and different modes This also fixes the tryAllocPixels/SkColorTable mismatch which was causing the "Image might be too large (32 x 32)" problems. BUG=skia:4952 ========== to ========== Add ability to fuzz images using scaling and different modes This also fixes the tryAllocPixels/SkColorTable mismatch which was causing the "Image might be too large (32 x 32)" problems. BUG=skia:4952 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
kjlubick@google.com changed reviewers: + msarett@google.com
Mode 3, kSubset_Mode only seemed to work for my stock webp images. All others exited "subset codec not supported". Is that normal?
scroggo@google.com changed reviewers: + scroggo@google.com
lgtm On 2016/02/16 19:01:05, kjlubick wrote: > Mode 3, kSubset_Mode only seemed to work for my stock webp images. All others > exited "subset codec not supported". Is that normal? Yes. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode23 fuzz/fuzz.cpp:23: __SK_FORCE_IMAGE_DECODER_LINKING; Note to self: We can remove this once we switch SKPs to use SkCodec (crrev.com/1671193002) https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode35 fuzz/fuzz.cpp:35: static uint8_t calculateMulti(SkData*); nit: Typically we name static functions as calculate_multi On another note, this name is mysterious to me. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode57 fuzz/fuzz.cpp:57: case 'i': return fuzz_img(bytes, strcmp(FLAGS_type[0], "image_scale") != 0 ? 0: multi, strcmp(FLAGS_type[0], "image_mode") != 0 ? 0: multi); Split this up over multiple lines? https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode156 fuzz/fuzz.cpp:156: colorPtr, colorCountPtr)) { nit: This should align with decodeInfo, above (or be indented 8 spaces?) https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode157 fuzz/fuzz.cpp:157: case SkCodec::kSuccess: nit: This should be indented four more spaces. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode163 fuzz/fuzz.cpp:163: case SkCodec::kInvalidConversion: Is this happening? If so, this is a bug. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode174 fuzz/fuzz.cpp:174: colorCountPtr)) { nit: Normally we would line this up with "decodeInfo", above.
lgtm https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode163 fuzz/fuzz.cpp:163: case SkCodec::kInvalidConversion: On 2016/02/16 19:17:38, scroggo wrote: > Is this happening? If so, this is a bug. Testing if we can remove this from DM. I can't remember why it's there. https://codereview.chromium.org/1704433003
https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp File fuzz/fuzz.cpp (right): https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode23 fuzz/fuzz.cpp:23: __SK_FORCE_IMAGE_DECODER_LINKING; On 2016/02/16 at 19:17:38, scroggo wrote: > Note to self: We can remove this once we switch SKPs to use SkCodec (crrev.com/1671193002) I added a TODO https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode35 fuzz/fuzz.cpp:35: static uint8_t calculateMulti(SkData*); On 2016/02/16 at 19:17:38, scroggo wrote: > nit: Typically we name static functions as > > calculate_multi > > On another note, this name is mysterious to me. Done. option seems a bit better. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode57 fuzz/fuzz.cpp:57: case 'i': return fuzz_img(bytes, strcmp(FLAGS_type[0], "image_scale") != 0 ? 0: multi, strcmp(FLAGS_type[0], "image_mode") != 0 ? 0: multi); On 2016/02/16 at 19:17:38, scroggo wrote: > Split this up over multiple lines? Done. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode156 fuzz/fuzz.cpp:156: colorPtr, colorCountPtr)) { On 2016/02/16 at 19:17:38, scroggo wrote: > nit: This should align with decodeInfo, above (or be indented 8 spaces?) Done. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode157 fuzz/fuzz.cpp:157: case SkCodec::kSuccess: On 2016/02/16 at 19:17:38, scroggo wrote: > nit: This should be indented four more spaces. Done. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode163 fuzz/fuzz.cpp:163: case SkCodec::kInvalidConversion: On 2016/02/16 at 19:17:38, scroggo wrote: > Is this happening? If so, this is a bug. It's not, AFAIK, but I can add signalBug so we know if it is. https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode174 fuzz/fuzz.cpp:174: colorCountPtr)) { On 2016/02/16 at 19:17:38, scroggo wrote: > nit: Normally we would line this up with "decodeInfo", above. Done.
On 2016/02/16 at 19:54:01, msarett wrote: > lgtm > > https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp > File fuzz/fuzz.cpp (right): > > https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode163 > fuzz/fuzz.cpp:163: case SkCodec::kInvalidConversion: > On 2016/02/16 19:17:38, scroggo wrote: > > Is this happening? If so, this is a bug. > > Testing if we can remove this from DM. I can't remember why it's there. > https://codereview.chromium.org/1704433003 2016 failures X_X Should I change back the segfaulting?
On 2016/02/16 20:07:54, kjlubick wrote: > On 2016/02/16 at 19:54:01, msarett wrote: > > lgtm > > > > https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp > > File fuzz/fuzz.cpp (right): > > > > https://codereview.chromium.org/1698963003/diff/20001/fuzz/fuzz.cpp#newcode163 > > fuzz/fuzz.cpp:163: case SkCodec::kInvalidConversion: > > On 2016/02/16 19:17:38, scroggo wrote: > > > Is this happening? If so, this is a bug. > > > > Testing if we can remove this from DM. I can't remember why it's there. > > https://codereview.chromium.org/1704433003 > > 2016 failures X_X Should I change back the segfaulting? 2016 failures if we remove the check for kInvalidConversion? I'll clear that up. You're fine to leave it in for now.
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1698963003/#ps60001 (title: "fix signalBug goof")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698963003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698963003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by kjlubick@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/1698963003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698963003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kjlubick@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com, scroggo@google.com Link to the patchset: https://codereview.chromium.org/1698963003/#ps80001 (title: "Fix pow ambiguity")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1698963003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1698963003/80001
Message was sent while issue was closed.
Description was changed from ========== Add ability to fuzz images using scaling and different modes This also fixes the tryAllocPixels/SkColorTable mismatch which was causing the "Image might be too large (32 x 32)" problems. BUG=skia:4952 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add ability to fuzz images using scaling and different modes This also fixes the tryAllocPixels/SkColorTable mismatch which was causing the "Image might be too large (32 x 32)" problems. BUG=skia:4952 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2a42f48b58f11c32017e2da6c8468a3e2cd9bd8c ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/2a42f48b58f11c32017e2da6c8468a3e2cd9bd8c |
