Bug 86666

Summary: Simplify TransformationMatrix rotation code to improve precision
Product: WebKit Reporter: Shawn Singh <shawnsingh>
Component: Layout and RenderingAssignee: Shawn Singh <shawnsingh>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, dino, enne, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Shawn Singh 2012-05-16 13:09:46 PDT
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.
Comment 1 Shawn Singh 2012-05-29 21:08:52 PDT
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.
Comment 2 Shawn Singh 2012-05-29 21:09:25 PDT
+enne for the webkit_unit_tests parts of this patch.
Comment 3 Shawn Singh 2012-05-29 21:10:41 PDT
>   - 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. =)
Comment 4 Darin Adler 2012-05-30 12:20:09 PDT
Comment on attachment 144691 [details]
Patch

Why a Chromium-specific test for a cross-platform bug fix?
Comment 5 Adrienne Walker 2012-05-30 12:45:32 PDT
(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.
Comment 6 Shawn Singh 2012-05-30 12:53:57 PDT
(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.
Comment 7 Adrienne Walker 2012-05-30 17:16:28 PDT
Comment on attachment 144691 [details]
Patch

Increasing precision seems non-controversial and your math looks correct.  R=me.
Comment 8 Shawn Singh 2012-05-30 17:17:34 PDT
Comment on attachment 144691 [details]
Patch

Thanks for reviewing!
Comment 9 WebKit Review Bot 2012-05-30 17:49:15 PDT
Comment on attachment 144691 [details]
Patch

Clearing flags on attachment: 144691

Committed r119008: <http://trac.webkit.org/changeset/119008>
Comment 10 WebKit Review Bot 2012-05-30 17:49:19 PDT
All reviewed patches have been landed.  Closing bug.