1215143 - FF Android VR returns orientation that is 90deg off on x-axis

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Casey Yee]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2535.0 Safari/537.36

Steps to reproduce:

When using VR mode using requestFullscreen w/VRDisplay, the forward orientation is 90deg rotated on the x-axis


Actual results:

VR content is displayed pointing towards the ground instead of ahead of the user.

Calling resetSensor() on the device when the user is in the proper start orientation resolves the issue.  


Expected results:

Should start with rotation in the proper forward orientation.

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Quick fix for vr rotation in landscape mode

Hi Casey,
I think this patch could fix your problem.

This is a workaround version for VR orientation in the landscape mode. I still need more time to figure out the coordinate transformation between the screen and the sensor to fix it completely.

Comment 2

[:kip (Kearwood Gilbert)]

Comment on attachment 8675987 [details] [diff] [review]
Quick fix for vr rotation in landscape mode

Review of attachment 8675987 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for finding this!

Perhaps it would be easier to eliminate the q0 variable and multiply q1 directly into q.

r=me with that

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: FF Android VR returns orientation that is 90deg off on x-axis.

V2: 
Follow Comment 2, removing q0 variable. 

In the landscape mode, the y and z axis are swapped, so we have to switch y and z terms at Quaternion q and multiply 90degree at x-axis.

Comment 4

[Daosheng Mu[:daoshengmu]]

Attachment: FF Android VR returns orientation that is 90deg off on x-axis

V2: 
Follow Comment 2, removing q0 variable. 

In the landscape mode, the y and z axis are swapped, so we have to switch y and z terms at Quaternion q and multiply 90degree at x-axis.

V3: Add const qualifier

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8677361 [details] [diff] [review]
FF Android VR returns orientation that is 90deg off on x-axis

This isn't the right fix -- if things aren't working out right, then it's an issue in one or some of:

1) not receiving orientation notifications -- is https://dxr.mozilla.org/mozilla-central/source/gfx/vr/gfxVRCardboard.cpp#137 being called, and does the orientation have the correct value?

2) the mScreenTransform being incorrectly set in the above location; this should be the thing that sets up the default coord space to be "normal" for the orientation

3) is Remap happening correctly at https://dxr.mozilla.org/mozilla-central/source/gfx/vr/gfxVRCardboard.cpp#45 ? Again this will depend on if the orientation is set right in mOrient

Comment 6

[Daosheng Mu[:daoshengmu]]

Attachment: 0001-Bug-1215143-FF-Android-VR-returns-orientation-that-i.patch

V2:

I have confirmed the notify function will receive event while orientation changed except PortraitSecondary type. I fix this bug by modifying mScreenTransform while receiving event. In my observation, in portrait mode, it would shift 90 degree at x-axis, so we have to rotate 90 degree at x-axis. In landscape mode, we need to rotate z-axis 90 degree first because its coordinate system have to be changed from the initial portrait mode, and then correcting 90 degree from x-axis. But while we are at landscape mode, the x-axis is swapped to y-axis, so we should use y-axis to transform it.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8679212 [details] [diff] [review]
0001-Bug-1215143-FF-Android-VR-returns-orientation-that-i.patch

> void
> HMDInfoCardboard::Notify(const mozilla::hal::ScreenConfiguration& config)
> {
>   mOrient = config.orientation();
> 
>   if (mOrient == eScreenOrientation_LandscapePrimary) {
>+    const Quaternion q0(0, (float) M_SQRT1_2, 0, (float) M_SQRT1_2);  // At landscape mode, x axis is swapped to y
>     mScreenTransform = Quaternion(0.f, 0.f, (float) M_SQRT1_2, (float) M_SQRT1_2);
>+
>+    mScreenTransform *= q0;   // Fixes for 90 degree on x-axis

Instead of doing this multiplication by q0 -- just calculate the correct final quaternion transform and set it explicitly.  The code is basically multiplying a constant by a constant.


>   } else if (mOrient == eScreenOrientation_LandscapeSecondary) {
>+    const Quaternion q0(0, (float) -M_SQRT1_2, 0, (float) M_SQRT1_2); // At landscape mode, x axis is swapped to y
>     mScreenTransform = Quaternion(0.f, 0.f, (float) -M_SQRT1_2, (float) M_SQRT1_2);
>+  
>+    mScreenTransform *= q0;   // Fixes for 90 degree on x-axis

Same here.

>+  } else if (mOrient == eScreenOrientation_PortraitPrimary) {  
>+    mScreenTransform = Quaternion((float) -M_SQRT1_2, 0.f, 0.f, (float) M_SQRT1_2); // Fixes for 90 degree on x-axis
>+
>+  } else if (mOrient == eScreenOrientation_PortraitSecondary) {   // PortraitSecondary event can't receive
>+    mScreenTransform = Quaternion((float) M_SQRT1_2, 0.f, 0.f, (float) M_SQRT1_2);  // Fixes for 90 degree on x-axis
>   }
> }

Also put some more informative comments at the start of this function -- "Fixes for 90 degree on x-axis" doesn't tell someone who's looking at the code much, other than that something was broken that involved 90 degrees and the x-axis.  Instead, add a more descriptive comment like "Android sends us events that have a 90-degree rotation about the x axis compared to what we want (phone flat vs. phone held in front of the eyes).  Correct for this by applying a transform to undo this rotation."

Comment 8

[Daosheng Mu[:daoshengmu]]

Attachment: Correct FF Android VR orientation (V3)

V3:
According to Comment 7, set the correct value to mScreenTransform instead of multiplying constants. In addition, add more descriptive comments at the beginning of this function.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8680086 [details] [diff] [review]
Correct FF Android VR orientation (V3)

Review of attachment 8680086 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 10

[Daosheng Mu[:daoshengmu]]

Please help land attachment 8680086 [details] [diff] [review] to m-c

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dad7e0ee9905

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dad7e0ee9905

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dad7e0ee9905

Comment 14

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion