Closed Bug 1470348 Opened 4 years ago Closed 4 years ago

Enable gfxVRExternal for Android

Categories

(Core :: WebVR, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: rbarker, Assigned: mortimergoro)

References

Details

(Whiteboard: [geckoview:fxr])

Attachments

(5 files, 1 obsolete file)

Complete work needed to completely enable immersive mode in Firefox Reality.
Whiteboard: [geckoview:fxr]
Latest version of WIP patch.
Attachment #8986955 - Attachment is obsolete: true
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.
Latest patch: Enable gfxVRExternal for Android
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)
Perfect, I'm happy with the solution. You can land it with the fix. Thanks
Flags: needinfo?(imanol)
https://hg.mozilla.org/mozilla-central/rev/10ce7c1ca2d3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox62=wontfix because this VR bug is not a Focus+GV blocker.
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-
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.