Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
508303005:
SkMatrix44::preserves2dAxisAlignment() (Closed)
DescriptionSkMatrix44::preserves2dAxisAlignment()
Convenience function requested for Chrome compositor that may have a performance
advantage.
BUG=skia:1017
R=reed@google.com,danakj@chromium.org
Committed: https://skia.googlesource.com/skia/+/faccb8eb53f1bc745d3dd530d8df8b40e0dedc23
Patch Set 1 #Patch Set 2 : Imported chrome unit test #Patch Set 3 : Fix perspective check, add tolerance #
Total comments: 3
Patch Set 4 : Use approximate math, perspective divide #
Total comments: 18
Patch Set 5 : Review fixes #Patch Set 6 : unit test review fixes #
Total comments: 1
Messages
Total messages: 21 (1 generated)
Sketch of a CL. My concerns about it are at more length on http://skbug.com/1017/: * Not sure this is much more performant? * Not sure this is acceptable correctness?
I see that we already have computeTypeMask(), so we are in a position to extend that to compute/cache this predicate. If the current name/predicate is well defined (and documented) and the current impl is correct, I'm fine to land this as-is (after we add some unittests). If the callers later decide this needs to be cached (at the expense of slowing down slightly computeTypeMask, we can always add that later.
Unfortunately this implementation fails the *very last* unit test in the Chrome test suite.
danakj@chromium.org changed reviewers: + danakj@chromium.org
Because perspective on z axis does preserve 2d alignment?
tomhudson@google.com changed reviewers: - danakj@google.com
tomhudson@google.com changed reviewers: + vollick@chromium.org
On 2014/08/28 21:54:06, danakj wrote: > Because perspective on z axis does preserve 2d alignment? The empirical test of the last test case returns 'true', but my SkMatrix44 implementation returned 'false'. This is presumably the test case that Ian was referring to in the discussion on the bug; the transform looks like 6e-17 1 0 0 -1 6e-17 0 0 0 0 1 -0.1 0 0 0 1 So I really do need to be checking abs() > epsilon. Chrome compositor is our only major consumer for SkMatrix44, so we could add this to computeMatrixType(), compute lazily, and cache the value. However, there'd need to be a microbenchmark you believe in to justify that. Dana, Ian, anybody else who might have an idea how to do the performance test?
On 2014/08/29 13:22:08, tomhudson wrote: > On 2014/08/28 21:54:06, danakj wrote: > > Because perspective on z axis does preserve 2d alignment? After fixing the Z bug that Dana pointed out, and adding approximation to the matrix component check, I fixed the last test case. And broke the first *empirical* perspective check. I believe my implementation now matches Chrome's, yet Chrome apparently passes the tests: // Perspective cases. transform.MakeIdentity(); transform.ApplyPerspectiveDepth(10.0); transform.RotateAboutYAxis(45.0); EXPECT_FALSE(EmpiricallyPreserves2dAxisAlignment(transform)); EXPECT_FALSE(transform.Preserves2dAxisAlignment()); Our version: // Perspective cases. transform.setIdentity(); transform.set(3, 2, -0.1); transform2.setRotateDegreesAbout(0.0, 1.0, 0.0, 45.0); transform.preConcat(transform2); REPORTER_ASSERT(reporter, !empirically_preserves_2d_axis_alignment(reporter, transform)); REPORTER_ASSERT(reporter, !transform.preserves2dAxisAlignment()); My implementation returns true for the first expectation, and false for the second. Inspecting the state of things when we're running that test: Matrix: [0.707107 0 -0.707107 0.0707107][0 1 0 0][0.707107 0 0.707107 -0.0707107][0 0 0 1] After transformation of the input rectangle during empirically_preserves_2d_axis_alignment(): p1 3.535534 5.000000 -3.535534 1.353553 p2 7.071068 5.000000 -7.071068 1.707107 p3 7.071068 20.000000 -7.071068 1.707107 p4 3.535534 20.000000 -3.535534 1.353553 That's clearly a set of four points that should pass the QuadF::IsRectilinear() test. This looks like a place where Preserves2dAxisAlignment() is being overly-conservative (as documented in gfx::Transform) and giving us a false negative, but then how does the Chrome unit test pass? ...color me confused...
https://codereview.chromium.org/508303005/diff/40001/tests/Matrix44Test.cpp File tests/Matrix44Test.cpp (right): https://codereview.chromium.org/508303005/diff/40001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:525: return (p1.fData[0] == p2.fData[0] && p2.fData[1] == p3.fData[1] && The gfx unit test uses within-std::epsilon<float> for this comparison, but I guess that can't be the reason for the difference, as that would make yours report fewer things as preserves, not more. https://codereview.chromium.org/508303005/diff/40001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:540: p1 = transform * p1; The gfx TransformPoint() method does something more/different than this: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/transform.c... Is that the difference? Have you tried printing out the 4 points final values in this case in that test? I suspect it's that because a rotate Y could cause clipping with the viewport.
https://codereview.chromium.org/508303005/diff/40001/tests/Matrix44Test.cpp File tests/Matrix44Test.cpp (right): https://codereview.chromium.org/508303005/diff/40001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:540: p1 = transform * p1; On 2014/09/03 19:59:22, danakj wrote: > The gfx TransformPoint() method does something more/different than this: > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/transform.c... > > Is that the difference? Have you tried printing out the 4 points final values in > this case in that test? > > I suspect it's that because a rotate Y could cause clipping with the viewport. I wonder if dividing through by w to get the perspective effects would help?
Mike, this now has unit tests and so should be ready per your earlier comment. Thanks to Dana and Ian; I needed to both do the perspective divide *and* use approximate equality tests in empirically_preserves_axis_alignment. This'll get the fundamental math out of gfx:: and into Skia, and if it ever shows up significantly hot we can migrate it to the computeTypeMask() cache system.
On 2014/09/19 16:16:55, tomhudson wrote: > Mike, this now has unit tests and so should be ready per your earlier comment. > > Thanks to Dana and Ian; I needed to both do the perspective divide *and* use > approximate equality tests in empirically_preserves_axis_alignment. > > This'll get the fundamental math out of gfx:: and into Skia, and if it ever > shows up significantly hot we can migrate it to the computeTypeMask() cache > system. lg2m!
LGTM thanks! https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:12: // Taken from Chromium's gfx::Transform Haha, hilarious since it says "Taken from SkMatrix44." https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp File tests/Matrix44Test.cpp (right): https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:764: transform.set(3, 2, -0.1); can you "// Perspective depth 10" here? https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:771: transform.set(3, 2, -0.1); and here?
https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:12: // Taken from Chromium's gfx::Transform On 2014/09/19 16:29:47, danakj wrote: > Haha, hilarious since it says "Taken from SkMatrix44." Yeah, it was taken from here, but has since been removed. :)
https://codereview.chromium.org/508303005/diff/60001/include/utils/SkMatrix44.h File include/utils/SkMatrix44.h (right): https://codereview.chromium.org/508303005/diff/60001/include/utils/SkMatrix44... include/utils/SkMatrix44.h:380: bool preserves2dAxisAlignment () const; Dox? e.g. - what does it mean? - does it have a tolerance? - does it handle 90 deg rotations? - etc. https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:13: const SkMScalar kEpsilon = 1e-8f; Can we actually document here the meaning/reason for this value? https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:867: bool SkMatrix44::preserves2dAxisAlignment () const { Did you consider taking an explicit epsilon (perhaps with a default parameter value) so we aren't tied to the current heuristic? https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:880: if (sk_float_abs(fMat[0][0]) > kEpsilon) { Since SkMScalar may not always be a float, we should use some indirection for abs, so we don't accidentally force a downscale of the value just to perform the compare. e.g. create SkMScalarAbs() https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:896: if (col0 > 1 || col1 > 1 || row0 > 1 || row1 > 1) return false; add comment why 1 big value is OK, but two are not? nit: needs {} https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp File tests/Matrix44Test.cpp (right): https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:708: // Try a few more practical situations to check precision can these be refactored into some sort of table? As is it is a lot of repeated text that is kinda hard to parse as a reader/maintainer.
https://codereview.chromium.org/508303005/diff/60001/include/utils/SkMatrix44.h File include/utils/SkMatrix44.h (right): https://codereview.chromium.org/508303005/diff/60001/include/utils/SkMatrix44... include/utils/SkMatrix44.h:380: bool preserves2dAxisAlignment () const; On 2014/09/19 16:45:14, reed1 wrote: > Dox? > > e.g. > - what does it mean? > - does it have a tolerance? > - does it handle 90 deg rotations? > - etc. Done. https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:13: const SkMScalar kEpsilon = 1e-8f; On 2014/09/19 16:45:14, reed1 wrote: > Can we actually document here the meaning/reason for this value? If you go back a few years I think the comment in this code reads "just picked a small value. not sure how to pick the "right" one" It looks like SK_ScalarNearlyZero is sufficient, so I changed this to a parameter as suggested below and made that the default value. https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:867: bool SkMatrix44::preserves2dAxisAlignment () const { On 2014/09/19 16:45:14, reed1 wrote: > Did you consider taking an explicit epsilon (perhaps with a default parameter > value) so we aren't tied to the current heuristic? Done. https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:880: if (sk_float_abs(fMat[0][0]) > kEpsilon) { On 2014/09/19 16:45:14, reed1 wrote: > Since SkMScalar may not always be a float, we should use some indirection for > abs, so we don't accidentally force a downscale of the value just to perform the > compare. > > e.g. create SkMScalarAbs() Done. https://codereview.chromium.org/508303005/diff/60001/src/utils/SkMatrix44.cpp... src/utils/SkMatrix44.cpp:896: if (col0 > 1 || col1 > 1 || row0 > 1 || row1 > 1) return false; On 2014/09/19 16:45:14, reed1 wrote: > add comment why 1 big value is OK, but two are not? > > nit: needs {} Done. https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp File tests/Matrix44Test.cpp (right): https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:708: // Try a few more practical situations to check precision On 2014/09/19 16:45:14, reed1 wrote: > can these be refactored into some sort of table? As is it is a lot of repeated > text that is kinda hard to parse as a reader/maintainer. Done. Doesn't save much line count, but should be more readable. https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:764: transform.set(3, 2, -0.1); On 2014/09/19 16:29:47, danakj wrote: > can you "// Perspective depth 10" here? Done. https://codereview.chromium.org/508303005/diff/60001/tests/Matrix44Test.cpp#n... tests/Matrix44Test.cpp:771: transform.set(3, 2, -0.1); On 2014/09/19 16:29:47, danakj wrote: > and here? Done.
lgtm https://codereview.chromium.org/508303005/diff/100001/src/utils/SkMatrix44.cpp File src/utils/SkMatrix44.cpp (right): https://codereview.chromium.org/508303005/diff/100001/src/utils/SkMatrix44.cp... src/utils/SkMatrix44.cpp:894: return false; evil, but you could say for speed: if ((col0 | col1 | row0 | row1) > 1) { return false; }
The CQ bit was checked by tomhudson@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/508303005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as faccb8eb53f1bc745d3dd530d8df8b40e0dedc23 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
