Bug 137134 - [iOS] Normalize our DeviceOrientation beta/gamma per spec
Summary: [iOS] Normalize our DeviceOrientation beta/gamma per spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL: https://w3c.github.io/deviceorientati...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-09-25 16:57 PDT by Benjamin Poulain
Modified: 2014-12-12 01:28 PST (History)
6 users (show)

See Also:


Attachments
Normalize iOS DeviceOrientation beta/gamma per spec (4.41 KB, patch)
2014-09-26 01:33 PDT, Rich Tibbett
no flags Details | Formatted Diff | Diff
Normalize iOS DeviceOrientation beta/gamma per spec (5.71 KB, patch)
2014-09-26 01:53 PDT, Rich Tibbett
no flags Details | Formatted Diff | Diff
Normalize iOS DeviceOrientation beta/gamma per spec (4.40 KB, patch)
2014-09-26 02:03 PDT, Rich Tibbett
no flags Details | Formatted Diff | Diff
Normalize iOS DeviceOrientation beta/gamma per spec (alternate fix) (3.55 KB, patch)
2014-09-29 01:43 PDT, Rich Tibbett
no flags Details | Formatted Diff | Diff
Normalize iOS DeviceOrientation beta/gamma per spec (alternate fix) (4.32 KB, patch)
2014-10-03 02:15 PDT, Rich Tibbett
no flags Details | Formatted Diff | Diff
Normalize iOS DeviceOrientation beta/gamma per spec (5.34 KB, patch)
2014-11-03 06:01 PST, Rich Tibbett
no flags Details | Formatted Diff | Diff
Normalize iOS DeviceOrientation beta/gamma per spec (5.34 KB, patch)
2014-11-03 06:11 PST, Rich Tibbett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2014-09-25 16:57:59 PDT
The W3C specification for DeviceOrientation says that
alpha should be [0, 360]
beta should be [-180, 180]
gamma should be [-90, 90]

CoreMotion gives us a beta in [-90, 90] and a gamma in [-180, 180]. We should consider renormalizing to match the spec.

<rdar://problem/9414459>
Comment 1 Benjamin Poulain 2014-09-25 16:59:34 PDT
Generous patch by Rich Tibbett: https://github.com/richtr/webkit/commit/92101b56132ea1a6ec186b857a9414f0505d5c24

I need to change my SDK before testing this. I'll have a look next week.
Poke me if I forget.
Comment 2 Rich Tibbett 2014-09-26 01:33:28 PDT
Created attachment 238704 [details]
Normalize iOS DeviceOrientation beta/gamma per spec
Comment 3 WebKit Commit Bot 2014-09-26 01:42:00 PDT
Attachment 238704 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:267:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:267:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:268:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:268:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:269:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:269:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:270:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:270:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:271:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:271:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:272:  Extra space after ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ios/WebCoreMotionManager.mm:272:  Extra space before )  [whitespace/parens] [2]
Total errors found: 12 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Rich Tibbett 2014-09-26 01:53:43 PDT
Created attachment 238705 [details]
Normalize iOS DeviceOrientation beta/gamma per spec
Comment 5 Rich Tibbett 2014-09-26 02:03:26 PDT
Created attachment 238706 [details]
Normalize iOS DeviceOrientation beta/gamma per spec
Comment 6 Rich Tibbett 2014-09-29 01:43:32 PDT
Created attachment 238844 [details]
Normalize iOS DeviceOrientation beta/gamma per spec (alternate fix)

I've uploaded two different patches. 

The first patch fixes the issue by creating its own compatible ZXY-based rotation matrix. The second patch reuses the rotation matrix supplied by the CMAttitude class. This _should_ be equivalent to the first solution but I'm unable to test this on the iOS hardware.

Could you take a look and run both patches on iOS hardware and compare the output with e.g. Chromium on Android? The only difference should be that `alpha` is not compass-oriented on iOS (the `alpha` readings will be different but the `beta` and `gamma` readings should now be identical).
Comment 7 Benjamin Poulain 2014-09-29 12:43:18 PDT
(In reply to comment #6)
> Created an attachment (id=238844) [details]
> Normalize iOS DeviceOrientation beta/gamma per spec (alternate fix)
> 
> I've uploaded two different patches. 
> 
> The first patch fixes the issue by creating its own compatible ZXY-based rotation matrix. The second patch reuses the rotation matrix supplied by the CMAttitude class. This _should_ be equivalent to the first solution but I'm unable to test this on the iOS hardware.
> 
> Could you take a look and run both patches on iOS hardware and compare the output with e.g. Chromium on Android? The only difference should be that `alpha` is not compass-oriented on iOS (the `alpha` readings will be different but the `beta` and `gamma` readings should now be identical).

I am finishing a big refactoring and my SDK is not right for testing this.

As soon as I am finished with my big patch (should be this week), I'll get the right SDK and start looking into this.

Do you know an easy test page to verify this? Otherwise I'll write one.
Comment 8 Rich Tibbett 2014-09-30 01:49:22 PDT
(In reply to comment #7)
> As soon as I am finished with my big patch (should be this week), I'll get the right SDK and start looking into this.

Thanks!

> Do you know an easy test page to verify this? Otherwise I'll write one.

That's an interesting question. You can visually verifying that the mathematics make sense via the Marine Compass demo: http://people.opera.com/richt/release/demos/orientation/marinecompass/. That will not be *compass* oriented yet on iOS but the compass should hold its position.

Secondly, you can run the general calibration test I created for deviceorientation at http://sensortest.org.

Existing iOS result: http://sensortest.org/?show=415ef021148ebfe1
*Expected* iOS result (with this patch): http://sensortest.org/?show=e21248215282d46c

The results may not necessarily match exactly (there is some noise introduced by non-iOS device accelerometers) but the general data patterns should be visible and should match.

I also have a VR-like demo @ https://richtr.github.io/Full-Tilt-JS/examples/vr_test.html that should work equally well with this patch.
Comment 9 Rich Tibbett 2014-09-30 15:58:54 PDT
(In reply to comment #8)
> Secondly, you can run the general calibration test I created for deviceorientation at http://sensortest.org.

Also, iOS accelerometer data is opposite to the conventions defined in the W3C spec so the SensorTest.org results may look inverted. See http://lists.w3.org/Archives/Public/public-geolocation/2014Jul/0024.html for further details.
Comment 10 Benjamin Poulain 2014-10-02 21:42:33 PDT
Those two demos are really cool!
-http://people.opera.com/richt/release/demos/orientation/marinecompass/
-https://richtr.github.io/Full-Tilt-JS/examples/vr_test.html

Really beautifully done.

I tried the second patch: https://bug-137134-attachments.webkit.org/attachment.cgi?id=238844 but it looks like all the axis are inverted.

I'll have a look at the first patch tomorrow.

I'll need to brush up some math to do the review, last time I did serious coordinate system manipulation was over 10 years ago :)
Comment 11 Rich Tibbett 2014-10-03 02:15:52 PDT
Created attachment 239195 [details]
Normalize iOS DeviceOrientation beta/gamma per spec (alternate fix)

> I tried the second patch: https://bug-137134-attachments.webkit.org/attachment.cgi?id=238844 but it looks like all the axis are inverted.

I have uploaded a replacement patch for this. If the axis are inverted in https://bugs.webkit.org/attachment.cgi?id=238844 then this new patch should fix this.

Let me know when you get a chance to test this on iOS hardware. 

Thank you for your help :)
Comment 12 Benjamin Poulain 2014-10-03 16:23:33 PDT
First patch also has incorrect orientation in the demos :(

I think we should postpone this by a month when I am done with CSS 4. The 30 minutes clean build is a bit of a pain in the ass at the moment.
Comment 13 Rich Tibbett 2014-10-06 02:45:44 PDT
(In reply to comment #12)
> First patch also has incorrect orientation in the demos :(

I'm trying to understand what went wrong. I have made a JavaScript-based test that mirrors the native code provided in the first patch (https://bug-137134-attachments.webkit.org/attachment.cgi?id=238706). 

This test can be found at: http://people.opera.com/richt/release/tests/orientation/ios_deviceorientation_fix/vr_test.html.

You must lock your device's screen rotation to the default screen orientation (i.e. with the home button at the bottom of the screen) to observe the correct behaviour. The other demos posted above apply screen rotation fixes which will report correct *screen-adjusted* Euler angle results. Is that why this seemed broken perhaps?

> 
> I think we should postpone this by a month when I am done with CSS 4. The 30 minutes clean build is a bit of a pain in the ass at the moment.

This can wait as I know you are busy. If you do get a chance to test patch 1 again though I would be very thankful. If it is not working perhaps you could provide more detailed feedback on what is actually happening.
Comment 14 Benjamin Poulain 2014-10-07 17:15:38 PDT
I am sorry, I am not helping you much with my fuzzy reports.

I currently work on some CSS features on OS X. I expect to work on iOS device by the end of the month or early next month. It is gonna be easier for me to work on this fix then.

If I have not given sign of life on this bug by November 1, please write me an angry email to wake me up :)
Comment 15 Benjamin Poulain 2014-10-29 21:02:54 PDT
So I just tested the first patch again to make a video of what is wrong...and everything works.

I think I probably messed up last time and applied the second patch twice by accident. :(

Let's move forward to review? Can you add a changelog?
Comment 16 Rich Tibbett 2014-11-03 06:01:56 PST
Created attachment 240843 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

Added a ChangeLog entry for this patch at Source/WebCore/ChangeLog.
Comment 17 Rich Tibbett 2014-11-03 06:11:54 PST
Created attachment 240845 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

Fixed ChangeLog entry for this patch at Source/WebCore/ChangeLog.
Comment 18 Benjamin Poulain 2014-11-07 12:27:37 PST
Rich, is this for review?
Comment 19 Benjamin Poulain 2014-12-11 17:40:48 PST
Committed r177198: <http://trac.webkit.org/changeset/177198>