Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567723004:
Fix handling of radii scaling to force the result to always be less (Closed)
DescriptionFix handling of radii scaling to force the result to always be less
than a given side.
BUG=472147
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1567723004
Committed: https://skia.googlesource.com/skia/+/f96bf1a2d091a917bb93f9e9a3a8d53bb39d068e
Patch Set 1 #Patch Set 2 : revert tests. #Patch Set 3 : remove spurious file. #Patch Set 4 : Remove debugging code #Patch Set 5 : fix near float problem #Patch Set 6 : small clean up. #Patch Set 7 : Get guard right. #Patch Set 8 : use c++ only #Patch Set 9 : fix nexttoward call. #Patch Set 10 : make nexttowardf force long double to make old arm libraries work #Patch Set 11 : use c99 version of nexttowardf. #Patch Set 12 : make code clearer #
Total comments: 12
Patch Set 13 : address Robert's comments #Patch Set 14 : Add assert. #Patch Set 15 : Change to be less intrusive. #Patch Set 16 : Change to be less intrusive. new file #Patch Set 17 : Add fuzzing test. #Patch Set 18 : Small cleanup. #
Messages
Total messages: 54 (18 generated)
Description was changed from ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 ========== to ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
revert tests.
remove spurious file.
Remove debugging code
Description was changed from ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
herb@google.com changed reviewers: + reed@google.com, robertphillips@google.com
The CQ bit was checked by herb@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/1567723004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567723004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...)
fix near float problem
small clean up.
Get guard right.
The CQ bit was checked by herb@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/1567723004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567723004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
use c++ only
The CQ bit was checked by herb@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/1567723004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567723004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
fix nexttoward call.
make nexttowardf force long double to make old arm libraries work
use c99 version of nexttowardf.
make code clearer
The CQ bit was checked by herb@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/1567723004/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567723004/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think this is ready for review. PTAL.
https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp File src/core/SkRRect.cpp (right): https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:128: if ((double)*a + (double)*b > limit) { minRadius & maxRadius ? https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:135: // new_min_radius must be float in order to give the actual value of the radius. newMinRadius ? https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:136: float new_min_radius = *min_radius * scale; Can it ever happen that newMinRadius is larger than limit ? https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:139: // than limit, but only by one ULP. newMaxRadius ? https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:155: } SkASSERT(*a >= 0.0f && *b >= 0.0f); ? https://codereview.chromium.org/1567723004/diff/210001/tests/DrawPathTest.cpp File tests/DrawPathTest.cpp (right): https://codereview.chromium.org/1567723004/diff/210001/tests/DrawPathTest.cpp... tests/DrawPathTest.cpp:315: Is there any way we could proactively run through all the questionable permutations of width,height and radii ?
I'm convinced - lgtm
address Robert's comments
Add assert.
https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp File src/core/SkRRect.cpp (right): https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:128: if ((double)*a + (double)*b > limit) { On 2016/01/07 17:56:41, robertphillips wrote: > minRadius & maxRadius ? Done. https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:135: // new_min_radius must be float in order to give the actual value of the radius. On 2016/01/07 17:56:41, robertphillips wrote: > newMinRadius ? Done. https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:136: float new_min_radius = *min_radius * scale; On 2016/01/07 17:56:41, robertphillips wrote: > Can it ever happen that newMinRadius is larger than limit ? Done. https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:139: // than limit, but only by one ULP. On 2016/01/07 17:56:41, robertphillips wrote: > newMaxRadius ? Done. https://codereview.chromium.org/1567723004/diff/210001/src/core/SkRRect.cpp#n... src/core/SkRRect.cpp:155: } On 2016/01/07 17:56:41, robertphillips wrote: > SkASSERT(*a >= 0.0f && *b >= 0.0f); ? Done. https://codereview.chromium.org/1567723004/diff/210001/tests/DrawPathTest.cpp File tests/DrawPathTest.cpp (right): https://codereview.chromium.org/1567723004/diff/210001/tests/DrawPathTest.cpp... tests/DrawPathTest.cpp:315: On 2016/01/07 17:56:41, robertphillips wrote: > Is there any way we could proactively run through all the questionable > permutations of width,height and radii ? I think this could be done, but in another CL.
The CQ bit was checked by reed@google.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1567723004/#ps250001 (title: "Add assert.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567723004/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567723004/250001
Message was sent while issue was closed.
Description was changed from ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f96bf1a2d091a917bb93f9e9a3a8d53bb39d068e ==========
Message was sent while issue was closed.
Committed patchset #14 (id:250001) as https://skia.googlesource.com/skia/+/f96bf1a2d091a917bb93f9e9a3a8d53bb39d068e
Message was sent while issue was closed.
Description was changed from ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f96bf1a2d091a917bb93f9e9a3a8d53bb39d068e ========== to ========== Fix handling of radii scaling to force the result to always be less than a given side. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/f96bf1a2d091a917bb93f9e9a3a8d53bb39d068e ==========
Iterate until in range.
Patchset #15 (id:270001) has been deleted
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:250001) has been created in https://codereview.chromium.org/1568063002/ by robertphillips@google.com. The reason for reverting is: Fear and doubt of NexusPlayer.
Message was sent while issue was closed.
Change to be less intrusive.
Message was sent while issue was closed.
Change to be less intrusive. new file
Message was sent while issue was closed.
Add fuzzing test.
Message was sent while issue was closed.
Small cleanup.
The CQ bit was checked by herb@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/1567723004/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567723004/350001
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-Arm7-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-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
