Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1527083002:
add caps for hairline moveTo / close (Closed)
DescriptionThis brings hairlines into agreement with thick strokes.
Add more testing and a pixel magnification to GM.
R=reed@google.com, fmalita@chromium.org
BUG=skia:4599
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1527083002
Committed: https://skia.googlesource.com/skia/+/1f17ab59929f5f5eb399e09690ef9dbc21525ccc
Patch Set 1 #Patch Set 2 : extend capped paths for moveTo / close #Patch Set 3 : paths add data only once #Patch Set 4 : clean up gm #
Total comments: 2
Patch Set 5 : add comment #Patch Set 6 : check if readpixels fails #Messages
Total messages: 25 (14 generated)
Description was changed from ========== wip; zero stroke BUG=skia: ========== to ========== wip; zero stroke BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== wip; zero stroke BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This brings hairlines into agreement with thick strokes. The src/ change is simple and echoes the path fill code. There's however a couple of mysteries around the GM that draws the test cases at their normal size and magnified. - Resizing the window causes the 1 pixel wide hairlines to draw increasingly darker - Removing the clips causes less to draw R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
caryclark@google.com changed reviewers: + fmalita@chromium.org, reed@google.com
Description was changed from ========== This brings hairlines into agreement with thick strokes. The src/ change is simple and echoes the path fill code. There's however a couple of mysteries around the GM that draws the test cases at their normal size and magnified. - Resizing the window causes the 1 pixel wide hairlines to draw increasingly darker - Removing the clips causes less to draw R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This brings hairlines into agreement with thick strokes. The src/ change is simple and echoes the path fill code. There's however a couple of mysteries around the GM that draws the test cases at their normal size and magnified. - Resizing the window causes the 1 pixel wide hairlines to draw increasingly darker - Removing the clips causes less to draw R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== This brings hairlines into agreement with thick strokes. The src/ change is simple and echoes the path fill code. There's however a couple of mysteries around the GM that draws the test cases at their normal size and magnified. - Resizing the window causes the 1 pixel wide hairlines to draw increasingly darker - Removing the clips causes less to draw R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This brings hairlines into agreement with thick strokes. The src/ change is simple and echoes the path fill code. There's however a couple of mysteries around the GM that draws the test cases at their normal size and magnified. - Resizing the window causes the 1 pixel wide hairlines to draw increasingly darker - Removing the clips causes less to draw R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by caryclark@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/1527083002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1527083002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== This brings hairlines into agreement with thick strokes. The src/ change is simple and echoes the path fill code. There's however a couple of mysteries around the GM that draws the test cases at their normal size and magnified. - Resizing the window causes the 1 pixel wide hairlines to draw increasingly darker - Removing the clips causes less to draw R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This brings hairlines into agreement with thick strokes. Add more testing and a pixel magnification to GM. R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
PTAL. Florin, after this lands, do you have time to rebaseline Chrome layout tests with SK_LEGACY_HAIR_IGNORES_CAPS removed?
lgtm https://codereview.chromium.org/1527083002/diff/60001/src/core/SkScan_Hairlin... File src/core/SkScan_Hairline.cpp (right): https://codereview.chromium.org/1527083002/diff/60001/src/core/SkScan_Hairlin... src/core/SkScan_Hairline.cpp:474: if (SkPaint::kButt_Cap != capStyle && prevVerb == SkPath::kMove_Verb) { maybe a comment saying we know what we're doing, that we're calling this with a degenerate segment, but we still want it capped.
https://codereview.chromium.org/1527083002/diff/60001/src/core/SkScan_Hairlin... File src/core/SkScan_Hairline.cpp (right): https://codereview.chromium.org/1527083002/diff/60001/src/core/SkScan_Hairlin... src/core/SkScan_Hairline.cpp:474: if (SkPaint::kButt_Cap != capStyle && prevVerb == SkPath::kMove_Verb) { On 2015/12/16 14:55:57, reed1 wrote: > maybe a comment saying we know what we're doing, that we're calling this with a > degenerate segment, but we still want it capped. Done.
The CQ bit was checked by caryclark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1527083002/#ps80001 (title: "add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1527083002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1527083002/80001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by caryclark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1527083002/#ps100001 (title: "check if readpixels fails")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1527083002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1527083002/100001
Message was sent while issue was closed.
Description was changed from ========== This brings hairlines into agreement with thick strokes. Add more testing and a pixel magnification to GM. R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This brings hairlines into agreement with thick strokes. Add more testing and a pixel magnification to GM. R=reed@google.com, fmalita@chromium.org BUG=skia:4599 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/1f17ab59929f5f5eb399e09690ef9dbc21525ccc ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/1f17ab59929f5f5eb399e09690ef9dbc21525ccc
Message was sent while issue was closed.
(in case this is not on your radar already) looks like the ASAN bot went red due to an int underflow: http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-Golo-GPU-GT6... ( 594/732 MB 1865) 439ms gpu gm contour_start../../../include/core/SkRect.h:212:17: runtime error: signed integer overflow: -2147483648 + -1 cannot be represented in type 'int' SUMMARY: AddressSanitizer: undefined-behavior ../../../include/core/SkRect.h:212:17 in |
