Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2073873002:
Simplify mask/clip intersection, making sure to explicitly check for an empty mask. (Closed)
DescriptionSimplify mask/clip intersection, making sure to explicitly check for an empty mask.
Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN.
A local run of nanobench --match text_ was not able to show this is faster or slower.
This patch fixed this first Chrome bug on my desktop, and the second is probably a dupe.
BUG=chromium:619378, chromium:613912
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002
Committed: https://skia.googlesource.com/skia/+/875e13ca0990e32da9db639743a913efe77f7e89
Patch Set 1 #Patch Set 2 : add test #Patch Set 3 : feh #Patch Set 4 : becoming frustrating #
Total comments: 1
Patch Set 5 : alt fix #Patch Set 6 : msvc #Patch Set 7 : revert old change #Messages
Total messages: 46 (23 generated)
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological coordinates like +Inf or NaN. BUG=chromium:619378 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological coordinates like +Inf or NaN. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ==========
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological coordinates like +Inf or NaN. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ==========
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ==========
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/1
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ==========
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. This patch fixed this Chrome bug on my desktop. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ==========
mtklein@chromium.org changed reviewers: + reed@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
If we make a unittest that calls drawPosText with Inf, would that trigger the bug (and show the fix)?
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. This patch fixed this Chrome bug on my desktop. BUG=chromium:619378 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. This patch fixed this first Chrome bug on my desktop, and the second is probably a dupe. BUG=chromium:619378,chromium:613912 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ==========
On 2016/06/16 at 20:49:49, reed wrote: > If we make a unittest that calls drawPosText with Inf, would that trigger the bug (and show the fix)? Yes indeed. Added in PS2.
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/40001
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...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2073873002/diff/60001/src/core/SkDraw.cpp File src/core/SkDraw.cpp (right): https://codereview.chromium.org/2073873002/diff/60001/src/core/SkDraw.cpp#new... src/core/SkDraw.cpp:1492: if (mask.fBounds.isEmpty() || I wonder 1. if it would be clearer to put this empty check after line 1478, since we care about empty/inverted in both cases. 2. we add a comment explaining why we need to check, even though the logic on lines 1474-5 would seem to mean we could never be empty/inverted.
On 2016/06/17 at 10:39:42, reed wrote: > https://codereview.chromium.org/2073873002/diff/60001/src/core/SkDraw.cpp > File src/core/SkDraw.cpp (right): > > https://codereview.chromium.org/2073873002/diff/60001/src/core/SkDraw.cpp#new... > src/core/SkDraw.cpp:1492: if (mask.fBounds.isEmpty() || > I wonder > > 1. if it would be clearer to put this empty check after line 1478, since we care about empty/inverted in both cases. > 2. we add a comment explaining why we need to check, even though the logic on lines 1474-5 would seem to mean we could never be empty/inverted. Started down this path and it forced me to read the whole function, which lead to PS 5. What do you think of this alternative fix? We were already trying to bail out if the float coordinates were out of safe range, just not taking NaN into account.
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/100001
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 mtklein@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/90003
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Whoops, forgot to ping the thread after trimming it down to just the fixed bounds checks. PTAL?
lgtm
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2073873002/90003
Message was sent while issue was closed.
Description was changed from ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. This patch fixed this first Chrome bug on my desktop, and the second is probably a dupe. BUG=chromium:619378,chromium:613912 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 ========== to ========== Simplify mask/clip intersection, making sure to explicitly check for an empty mask. Previously we were only asserting the mask wasn't empty, which isn't necessarily true when we're given pathological float coordinates like +Inf or NaN. A local run of nanobench --match text_ was not able to show this is faster or slower. This patch fixed this first Chrome bug on my desktop, and the second is probably a dupe. BUG=chromium:619378,chromium:613912 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2073873002 Committed: https://skia.googlesource.com/skia/+/875e13ca0990e32da9db639743a913efe77f7e89 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:90003) as https://skia.googlesource.com/skia/+/875e13ca0990e32da9db639743a913efe77f7e89 |
