Bug 137134 – [iOS] Normalize our DeviceOrientation beta/gamma per spec

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Benjamin Poulain]

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>

[Benjamin Poulain]

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.

[Rich Tibbett]

Created attachment 238704 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

[WebKit Commit Bot]

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.

[Rich Tibbett]

Created attachment 238705 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

[Rich Tibbett]

Created attachment 238706 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

[Rich Tibbett]

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).

[Benjamin Poulain]

(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.

[Rich Tibbett]

(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.

[Rich Tibbett]

(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.

[Benjamin Poulain]

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 :)

[Rich Tibbett]

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 :)

[Benjamin Poulain]

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.

[Rich Tibbett]

(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.

[Benjamin Poulain]

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 :)

[Benjamin Poulain]

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?

[Rich Tibbett]

Created attachment 240843 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

Added a ChangeLog entry for this patch at Source/WebCore/ChangeLog.

[Rich Tibbett]

Created attachment 240845 [details]
Normalize iOS DeviceOrientation beta/gamma per spec

Fixed ChangeLog entry for this patch at Source/WebCore/ChangeLog.

[Benjamin Poulain]

Rich, is this for review?

[Benjamin Poulain]

Committed r177198: <http://trac.webkit.org/changeset/177198>