Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2239463002:
Create gradient fuzzers (Closed)
DescriptionCreate gradient fuzzers
This would have caught the bug patched by https://codereview.chromium.org/2234663002
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2239463002
Committed: https://skia.googlesource.com/skia/+/1eda1eb2039518f534c4b893482c8b0f8c4abeab
Patch Set 1 #Patch Set 2 : Clean up duplicate code #Patch Set 3 : Last gradient #Patch Set 4 : Use global and local matrices #
Total comments: 12
Patch Set 5 : Address feedback #Patch Set 6 : Clean up initGradiantParams #Messages
Total messages: 20 (12 generated)
Description was changed from ========== First shake at Linear gradient fuzzing BUG=skia: ========== to ========== First shake at Linear gradient fuzzing BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2239463002 ==========
Description was changed from ========== First shake at Linear gradient fuzzing BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2239463002 ========== to ========== Create gradient fuzzers This would have caught the bug patched by https://codereview.chromium.org/2234663002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2239463002 ==========
kjlubick@google.com changed reviewers: + fmalita@chromium.org
Looks good, mostly nits. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:18: SkScalar scaleX, skewX, transX, skewY, scaleY, transY, persp0, persp1, persp2; General formatting: 4-space indentation, per Skia style https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:34: bool initColors(Fuzz* fuzz, uint32_t* count, SkColor** colors, SkScalar** pos, Nit: rename to initGradientParams? https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:94: SkScalar* pos; Do we ever deallocate these? Maybe refactor using SkTDArray<SkColor>, SkTDArray<SkScalar> to avoid ownership woes? Then we could pass SkTDArray<SkColor>*/SkTDArray<SkScalar>* to initColors(). https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:107: if (!fuzz->next(&flags)) { The (poorly documented?) flags parameter is not related to using a local matrix, we should move it outside the useLocalMatrix conditional. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:114: } Nit: Skia-idiomatic refactor idea for this block, since the latter factory version is equivalent to MakeLinear(pts, colors, pos, count, mode, 0, nullptr) uint32_t flags; if (!fuzz->next(&flags)) { return; } SkTLazy<SkMatrix> localMatrix; if (useLocalMatrix && !makeMatrix(fuzz, localMatrix.init())) { return; } SkPaint p; p.setShader(SkGradientShader::MakeLinear(pts, colors, pos, count, mode, flags, localMatrix.getMaybeNull()); (also applicable to the other gradient flavors) https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:116: sk_sp<SkSurface> surface(SkSurface::MakeRasterN32Premul(50, 50)); I think this would run much faster if we hoisted the surface allocation from the main fuzzer loop. It'll probably take some major surgery (adding a stateful context?), so just an idea if you want to improve perf in the future.
https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp File fuzz/FuzzGradients.cpp (right): https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:18: SkScalar scaleX, skewX, transX, skewY, scaleY, transY, persp0, persp1, persp2; On 2016/08/11 at 16:06:20, f(malita) wrote: > General formatting: 4-space indentation, per Skia style Done. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:34: bool initColors(Fuzz* fuzz, uint32_t* count, SkColor** colors, SkScalar** pos, On 2016/08/11 at 16:06:20, f(malita) wrote: > Nit: rename to initGradientParams? Done. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:94: SkScalar* pos; On 2016/08/11 at 16:06:20, f(malita) wrote: > Do we ever deallocate these? > > Maybe refactor using SkTDArray<SkColor>, SkTDArray<SkScalar> to avoid ownership woes? > > Then we could pass SkTDArray<SkColor>*/SkTDArray<SkScalar>* to initColors(). See below for explanation that when the return happens, the program dies. I can still wrap these in SkTDArrays if you want, but they get "deallocated" when the program ends after this method returns. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:107: if (!fuzz->next(&flags)) { On 2016/08/11 at 16:06:20, f(malita) wrote: > The (poorly documented?) flags parameter is not related to using a local matrix, we should move it outside the useLocalMatrix conditional. Done, using your refactor below. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:114: } On 2016/08/11 at 16:06:20, f(malita) wrote: > Nit: Skia-idiomatic refactor idea for this block, since the latter factory version is equivalent to MakeLinear(pts, colors, pos, count, mode, 0, nullptr) > > uint32_t flags; > if (!fuzz->next(&flags)) { > return; > } > > SkTLazy<SkMatrix> localMatrix; > if (useLocalMatrix && !makeMatrix(fuzz, localMatrix.init())) { > return; > } > > SkPaint p; > p.setShader(SkGradientShader::MakeLinear(pts, colors, pos, count, mode, flags, localMatrix.getMaybeNull()); > > > (also applicable to the other gradient flavors) Done. https://codereview.chromium.org/2239463002/diff/60001/fuzz/FuzzGradients.cpp#... fuzz/FuzzGradients.cpp:116: sk_sp<SkSurface> surface(SkSurface::MakeRasterN32Premul(50, 50)); On 2016/08/11 at 16:06:20, f(malita) wrote: > I think this would run much faster if we hoisted the surface allocation from the main fuzzer loop. > > It'll probably take some major surgery (adding a stateful context?), so just an idea if you want to improve perf in the future. This fuzzer code isn't like the "fuzz unit tests" where we run this in a for loop. It's powered by an external source, called afl-fuzz. It runs Skia many times, each short lived, so there's no way to keep this "allocated", because afl-fuzz kills the process each time. Also, It's running at about 1500 times/s on my desktop (single core). I shoot for > 500, so we are good.
Thanks for the explanation, LGTM.
The CQ bit was checked by kjlubick@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/v2/patch-status/codereview.chromium.or...
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 fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2239463002/#ps100001 (title: "Clean up initGradiantParams")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create gradient fuzzers This would have caught the bug patched by https://codereview.chromium.org/2234663002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2239463002 ========== to ========== Create gradient fuzzers This would have caught the bug patched by https://codereview.chromium.org/2234663002 BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2239463002 Committed: https://skia.googlesource.com/skia/+/1eda1eb2039518f534c4b893482c8b0f8c4abeab ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/1eda1eb2039518f534c4b893482c8b0f8c4abeab |
