Bug 86666 – Simplify TransformationMatrix rotation code to improve precision

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Shawn Singh]

TransformationMatrix rotate3d()  (both overloaded versions)  seem to use unnecessary indirect computations to initialize the rotation matrices.   Specifically, instead of using cos(theta) and sin(theta) directly, this code computes cos(theta/2), sin(theta/2) and sin^2 (theta/2), and then uses trig identities to compute the same traditional rotation matrices.  I did some quick and dirty fprintf experiment, which suggested that using traditional sin/cos we can gain 1-2 bits of precision in some cases, particularly for the sin^2 term, where the squaring compounds the error.

Perhaps this code should be simplified to use just sin/cos to create the canonical rotation matrices.

Two possible caveats of simplifying this code:
  - perhaps there was a good reason to compute theta/2 and then use those trig identities?  I cannot think of why.
  - There is a chance this will require rebaselining a vast number of pixel tests, and I would need some suggestions how to minimize the risk and pain of breaking all pixel tests for a short while.

Dean and Chris, do either of you feel that this code should stay the way it is now?  Doing this is not high priority, but if you want it done, I think its a reasonable safe change to make except for the rebaselining.  Thanks in advance.

[Shawn Singh]

Created attachment 144691 [details]
Patch

Tested with webkit_unit_tests and layout tests on mac; no obvious regressions. But I would prefer to run layout tests a few more times with/without this patch to double check that some of the flakiness was not the fault of this patch.

[Shawn Singh]

+enne for the webkit_unit_tests parts of this patch.

[Shawn Singh]

>   - There is a chance this will require rebaselining a vast number of pixel tests, and I would need some suggestions how to minimize the risk and pain of breaking all pixel tests for a short while.

last comment for now... I was completely wrong about this, nothing should need rebaselining. =)

[Darin Adler]

Comment on attachment 144691 [details]
Patch

Why a Chromium-specific test for a cross-platform bug fix?

[Adrienne Walker]

(In reply to comment #4)
> (From update of attachment 144691 [details])
> Why a Chromium-specific test for a cross-platform bug fix?

Looking at Shawn's test, it would be extremely awkward to try to expose that code into a cross-platform layout test.  In my opinion, unit tests seem like the best way to test a change like this.

If other ports had C++ unit tests, it certainly could be cross-platform.

[Shawn Singh]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 144691 [details] [details])
> > Why a Chromium-specific test for a cross-platform bug fix?
> 
> Looking at Shawn's test, it would be extremely awkward to try to expose that code into a cross-platform layout test.  In my opinion, unit tests seem like the best way to test a change like this.
> 
> If other ports had C++ unit tests, it certainly could be cross-platform.

It's also worth pointing out that this change is already covered by existing layout tests, specifically transforms/cssmatrix-3d-interface.xhtml which did a great job catching a subtle mistake I had before submitting this patch.

[Adrienne Walker]

Comment on attachment 144691 [details]
Patch

Increasing precision seems non-controversial and your math looks correct.  R=me.

[Shawn Singh]

Comment on attachment 144691 [details]
Patch

Thanks for reviewing!

[WebKit Review Bot]

Comment on attachment 144691 [details]
Patch

Clearing flags on attachment: 144691

Committed r119008: <http://trac.webkit.org/changeset/119008>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.