Enable gfxVRExternal for Android

RESOLVED FIXED in Firefox 63

Status

()

RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: rbarker, Assigned: imanol)

Tracking

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

(Whiteboard: [geckoview:fxr])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
Complete work needed to completely enable immersive mode in Firefox Reality.
(Reporter)

Updated

9 months ago
Whiteboard: [geckoview:fxr]
(Reporter)

Comment 1

9 months ago
Latest version of WIP patch.
Attachment #8986955 - Attachment is obsolete: true
(Assignee)

Comment 2

8 months ago
Latest patch with Enter/Exit WebVR working
Duplicate of this bug: 1462446
Attachment #8991079 - Flags: review?(kgilbert)
Comment on attachment 8991079 [details] [diff] [review]
latest_patch_webvr.diff

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

Looks great, just a couple of code-style nits.  r=me with that.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1852,1 @@
>    gl()->ReleaseSurface();

Perhaps it's helpful to keep this comment above the ReleaseSurface() call?

// ReleaseSurface internally calls MakeCurrent.

::: gfx/vr/gfxVRExternal.cpp
@@ +247,4 @@
>          // Service has shut down or hardware has been disconnected
>          return false;
>        }
> +    } else {

Nit: Can trim the empty else {} here.

@@ +616,4 @@
>  }
>  
>  void
> +VRSystemManagerExternal::PushState(VRBrowserState* aBrowserState, bool notifyCond)

Nit: prefix argument names with "a" and ensure name in header matches.

eg: bool aNotifyCond

::: gfx/vr/gfxVRExternal.h
@@ +97,4 @@
>                               const VRManagerPromise& aPromise) override;
>    virtual void StopVibrateHaptic(uint32_t aControllerIdx) override;
>    bool PullState(VRDisplayState* aDisplayState, VRHMDSensorState* aSensorState = nullptr);
> +  void PushState(VRBrowserState* aBrowserState, const bool SubmitFrame = false);

Nit: Prefix argument names with "a" and match with name used in implementation file.
Comment on attachment 8991079 [details] [diff] [review]
latest_patch_webvr.diff

Looks great, just a couple of code-style nits in the review.  r=me with that.
Attachment #8991079 - Flags: review?(kgilbert) → review+
One last change for the patch.

You should add a commit message that starts with the bug number, formatted like this:

Bug 1470348 - The description goes here

This ensures that our build infrastructure can reference back to this bug for context.
(Assignee)

Comment 7

8 months ago
Latest patch: Enable gfxVRExternal for Android
Comment hidden (mozreview-request)

Comment 10

8 months ago
mozreview-review
Comment on attachment 8991160 [details]
Bug 1470348 - Enable gfxVRExternal for Android;

https://reviewboard.mozilla.org/r/256126/#review263012

Already reviewed Imanol's patch attachment before pushing to reviewboard.  Running through try before autoland...
Attachment #8991160 - Flags: review?(kgilbert) → review+
Assignee: nobody → imanol
I had to rebase the patchset onto central and resolve conflicts to push to try.  While doing so, I noticed that some extra debug logging was enabled in the latest patch which wasn't in the first one reviewed, so stripped that off to match the original reviewed patch.

Also, I noticed that an early exit at the top of VRSystemManagerExternal::CloseShmem() used to prevent un-mapping pointers passed into the constructor was removed.  This is the right thing for Android, but not for desktop which will use this function for the external process as well as in-process VR interfaces.

I went ahead and added #ifdef's so it is only removed for android:

VRSystemManagerExternal::CloseShmem()
{
#if !defined(MOZ_WIDGET_ANDROID)
  if (mSameProcess) {
    return;
#endif

The final patch I pushed to try is attached to the MozReview.

The try run was all green except for build failures on Android due to handling warnings as errors.  In this case VRSystemManagerExternal::mSameProcess was an unused variable for Android builds resulting in a warning causing the build to be rejected.
To eliminate this warning, I can add an #ifdef to the declaration and initialization of VRSystemManagerExternal::mSameProcess to prevent it from being included in the android builds.

If you are happy with this solution, I can land this with the fix for you.
Flags: needinfo?(imanol)
(Assignee)

Comment 13

8 months ago
Perfect, I'm happy with the solution. You can land it with the fix. Thanks
Flags: needinfo?(imanol)

Comment 14

8 months ago
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10ce7c1ca2d3
Enable gfxVRExternal for Android; r=kip

Comment 15

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/10ce7c1ca2d3
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox62=wontfix because this VR bug is not a Focus+GV blocker.
status-firefox61: --- → wontfix
status-firefox62: affected → wontfix
status-firefox-esr52: --- → wontfix
status-firefox-esr60: --- → wontfix

Comment 17

8 months ago
mozreview-review
Comment on attachment 8991160 [details]
Bug 1470348 - Enable gfxVRExternal for Android;

https://reviewboard.mozilla.org/r/256126/#review267476

::: dom/canvas/WebGLContext.cpp:2303
(Diff revision 1)
> +#if defined(MOZ_WIDGET_ANDROID)
> +already_AddRefed<layers::SharedSurfaceTextureClient>
> +WebGLContext::GetVRFrame()
> +{
> +  if (IsContextLost()) {
> +    ForceRestoreContext();

Woah, what? This really doesn't seem right.
Also this is dom/canvas/WebGL*, which needs my review.

Please explain why this should try to restore the context here.
Attachment #8991160 - Flags: review-
(Assignee)

Comment 18

8 months ago
mozreview-review-reply
Comment on attachment 8991160 [details]
Bug 1470348 - Enable gfxVRExternal for Android;

https://reviewboard.mozilla.org/r/256126/#review267476

> Woah, what? This really doesn't seem right.
> Also this is dom/canvas/WebGL*, which needs my review.
> 
> Please explain why this should try to restore the context here.

Actually we can remove that line. It was an attempt to recover from this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1474847

But we plan to submit a patch to fix the real issue soon. We can remove that line in that patch, unless you want a separate patch to remove that code.
You need to log in before you can comment on or make changes to this bug.