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

RESOLVED FIXED in Firefox 45

Status

()

Firefox
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Casey Yee, Assigned: daoshengmu)

Tracking

44 Branch
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [webvr])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(vladimir)
Whiteboard: [webvr]
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

2 years ago
Created attachment 8675987 [details] [diff] [review]
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 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
Attachment #8675987 - Flags: review+
(Assignee)

Comment 3

2 years ago
Created attachment 8677314 [details] [diff] [review]
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.
Attachment #8675987 - Attachment is obsolete: true
Attachment #8677314 - Flags: review?(kgilbert)
(Assignee)

Comment 4

2 years ago
Created attachment 8677361 [details] [diff] [review]
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
Attachment #8677314 - Attachment is obsolete: true
Attachment #8677314 - Flags: review?(kgilbert)
Attachment #8677361 - Flags: review?(kgilbert)
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
Flags: needinfo?(vladimir)
Attachment #8677361 - Flags: review?(kgilbert) → review-
(Assignee)

Comment 6

2 years ago
Created attachment 8679212 [details] [diff] [review]
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.
Attachment #8677361 - Attachment is obsolete: true
Attachment #8679212 - Flags: review?(vladimir)
Assignee: nobody → dmu
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."
(Assignee)

Comment 8

2 years ago
Created attachment 8680086 [details] [diff] [review]
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.
Attachment #8679212 - Attachment is obsolete: true
Attachment #8679212 - Flags: review?(vladimir)
Attachment #8680086 - Flags: review?(vladimir)
Comment on attachment 8680086 [details] [diff] [review]
Correct FF Android VR orientation (V3)

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

Looks good, thanks!
Attachment #8680086 - Flags: review?(vladimir) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 10

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

Comment 11

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/dad7e0ee9905
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dad7e0ee9905
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dad7e0ee9905
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: --- → ---
You need to log in before you can comment on or make changes to this bug.