Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2143133005:
Guard SkMatrix::get*Scale[s]() against negative nearly-zero values. (Closed)
|
Created:
4 years, 5 months ago by Krzysztof Olczyk Modified:
4 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionGuard SkMatrix::get*Scale[s]() against negative nearly-zero values.
BUG=skia:4718
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2143133005
Committed: https://skia.googlesource.com/skia/+/cea22ae2349502334312bd1d9e10ffa894f70e05
Patch Set 1 #Patch Set 2 : guard against inf too, update the condition to better suit negative nearly-zeros and added unit tes… #
Total comments: 2
Messages
Total messages: 26 (11 generated)
Description was changed from ========== Guard SkMatrix::get*Scale[s]() against negative nearly-zero values. BUG=skia:4718 ========== to ========== Guard SkMatrix::get*Scale[s]() against negative nearly-zero values. BUG=skia:4718 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2143133005 ==========
kolczyk@opera.com changed reviewers: + bsalomon@google.com, reed@google.com
Hi reed, bsalomon, please take a look at this change addressing another hit of skia:4718 assertion we could see.
reed@google.com changed reviewers: + jvanverth@google.com
The referenced bug talks initially about infinities. - Should we use the same bug for both issues (inf and negatives)? - Is there a unittest we can add to test this fix? - Should we try to fix the infinities at the same time?
On 2016/07/14 at 14:10:00, reed wrote: > The referenced bug talks initially about infinities. As I have commented in the bug, I have the impression it talks in general about this failing assert. > - Should we use the same bug for both issues (inf and negatives)? And NaNs. First CL landed fixed it against NaNs.
kolczyk@opera.com changed reviewers: + reed@google.comh - reed@google.com
I've uploaded the unit test with one of the matrices I was getting failure on. Also changed IsNaN check into !IsFinite. While in the arithmetic calculation here, inifinities turn into NaNs, it still makes more sense to check if the number is finite. PTAL.
The CQ bit was checked by kolczyk@opera.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.
lgtm
https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > -SK_ScalarNearlyZero) { I missed something here -- why was this changed to only clamp negative nearly zero values? This seems asymmetric.
On 2016/07/15 13:49:05, jvanverth1 wrote: > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp > File src/core/SkMatrix.cpp (right): > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... > src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > > -SK_ScalarNearlyZero) { > I missed something here -- why was this changed to only clamp negative nearly > zero values? This seems asymmetric. This function is supposed to return positive scale factors. Tiny positive numbers are ok but negatives are not.
On 2016/07/15 13:50:53, bsalomon wrote: > On 2016/07/15 13:49:05, jvanverth1 wrote: > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp > > File src/core/SkMatrix.cpp (right): > > > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... > > src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > > > -SK_ScalarNearlyZero) { > > I missed something here -- why was this changed to only clamp negative nearly > > zero values? This seems asymmetric. > > This function is supposed to return positive scale factors. Tiny positive > numbers are ok but negatives are not. But it will still return negative values if the value is <= -SK_ScalarNearlyZero. Why not just check for < 0 and clamp then?
On 2016/07/15 14:05:19, jvanverth1 wrote: > On 2016/07/15 13:50:53, bsalomon wrote: > > On 2016/07/15 13:49:05, jvanverth1 wrote: > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp > > > File src/core/SkMatrix.cpp (right): > > > > > > > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... > > > src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > > > > -SK_ScalarNearlyZero) { > > > I missed something here -- why was this changed to only clamp negative > nearly > > > zero values? This seems asymmetric. > > > > This function is supposed to return positive scale factors. Tiny positive > > numbers are ok but negatives are not. > > But it will still return negative values if the value is <= > -SK_ScalarNearlyZero. Why not just check for < 0 and clamp then? We probably could do that but we may also want an assert that it isn't "too negative" since only numerical error should lead to a negative.
On 2016/07/15 14:10:57, bsalomon wrote: > On 2016/07/15 14:05:19, jvanverth1 wrote: > > On 2016/07/15 13:50:53, bsalomon wrote: > > > On 2016/07/15 13:49:05, jvanverth1 wrote: > > > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp > > > > File src/core/SkMatrix.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... > > > > src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > > > > > -SK_ScalarNearlyZero) { > > > > I missed something here -- why was this changed to only clamp negative > > nearly > > > > zero values? This seems asymmetric. > > > > > > This function is supposed to return positive scale factors. Tiny positive > > > numbers are ok but negatives are not. > > > > But it will still return negative values if the value is <= > > -SK_ScalarNearlyZero. Why not just check for < 0 and clamp then? > > We probably could do that but we may also want an assert that it isn't "too > negative" since only numerical error should lead to a negative. That sounds good to me.
On 2016/07/15 at 14:05:19, jvanverth wrote: > On 2016/07/15 13:50:53, bsalomon wrote: > > On 2016/07/15 13:49:05, jvanverth1 wrote: > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp > > > File src/core/SkMatrix.cpp (right): > > > > > > > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... > > > src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > > > > -SK_ScalarNearlyZero) { > > > I missed something here -- why was this changed to only clamp negative nearly > > > zero values? This seems asymmetric. > > > > This function is supposed to return positive scale factors. Tiny positive > > numbers are ok but negatives are not. > > But it will still return negative values if the value is <= -SK_ScalarNearlyZero. Why not just check for < 0 and clamp then? No, it won't return negative, as it will be caught by an assert below. Basically, small negative value is probably a precision error to be clamped to zero, while big negative is probably an error in the numerical method, so it should be OK to keep the assert. Of course, where this border line of "precision error" lie is not so obvious, but I assume the value of SK_ScalarNearlyZero was reasonably chosen for such purposes.
The CQ bit was checked by kolczyk@opera.com
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 ========== Guard SkMatrix::get*Scale[s]() against negative nearly-zero values. BUG=skia:4718 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2143133005 ========== to ========== Guard SkMatrix::get*Scale[s]() against negative nearly-zero values. BUG=skia:4718 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2143133005 Committed: https://skia.googlesource.com/skia/+/cea22ae2349502334312bd1d9e10ffa894f70e05 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/cea22ae2349502334312bd1d9e10ffa894f70e05
Message was sent while issue was closed.
reed@google.com changed reviewers: + reed@google.com
Message was sent while issue was closed.
https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp File src/core/SkMatrix.cpp (right): https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > -SK_ScalarNearlyZero) { On 2016/07/15 13:49:05, jvanverth1 wrote: > I missed something here -- why was this changed to only clamp negative nearly > zero values? This seems asymmetric. Agreed. This reads oddly -- we clamp nearly-zero-negative values, and then assert that we're never negative. Why not add whatever assert you want ahead of time, and then clamp all negative values?
Message was sent while issue was closed.
On 2016/07/17 at 00:15:17, reed wrote: > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp > File src/core/SkMatrix.cpp (right): > > https://codereview.chromium.org/2143133005/diff/20001/src/core/SkMatrix.cpp#n... > src/core/SkMatrix.cpp:1574: if (results[0] < 0 && results[0] > -SK_ScalarNearlyZero) { > On 2016/07/15 13:49:05, jvanverth1 wrote: > > I missed something here -- why was this changed to only clamp negative nearly > > zero values? This seems asymmetric. > > Agreed. This reads oddly -- we clamp nearly-zero-negative values, and then assert that we're never negative. Why not add whatever assert you want ahead of time, and then clamp all negative values? Ah, I see what you mean here now. Sorry for having it landed already, I understood you were fine with this after bsalomon's clarifications. I will send a fixup CL soon. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
