Closed
Bug 1470348
Opened 6 years ago
Closed 6 years ago
Enable gfxVRExternal for Android
Categories
(Core :: WebVR, enhancement)
Core
WebVR
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: rbarker, Assigned: mortimergoro)
References
Details
(Whiteboard: [geckoview:fxr])
Attachments
(5 files, 1 obsolete file)
13.20 KB,
patch
|
Details | Diff | Splinter Review | |
13.05 KB,
patch
|
kip
:
review+
|
Details | Diff | Splinter Review |
16.97 KB,
patch
|
Details | Diff | Splinter Review | |
16.75 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
kip
:
review+
jgilbert
:
review-
|
Details |
Complete work needed to completely enable immersive mode in Firefox Reality.
Reporter | ||
Updated•6 years ago
|
Whiteboard: [geckoview:fxr]
Reporter | ||
Comment 1•6 years ago
|
||
Latest version of WIP patch.
Attachment #8986955 -
Attachment is obsolete: true
Assignee | ||
Comment 2•6 years ago
|
||
Latest patch with Enter/Exit WebVR working
Updated•6 years ago
|
Attachment #8991079 -
Flags: review?(kgilbert)
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
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+
Comment 6•6 years ago
|
||
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•6 years ago
|
||
Latest patch: Enable gfxVRExternal for Android
Assignee | ||
Comment 8•6 years ago
|
||
Comment hidden (mozreview-request) |
Comment 10•6 years 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+
Updated•6 years ago
|
Assignee: nobody → imanol
Comment 11•6 years ago
|
||
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.
Comment 12•6 years ago
|
||
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•6 years ago
|
||
Perfect, I'm happy with the solution. You can land it with the fix. Thanks
Flags: needinfo?(imanol)
Comment 14•6 years ago
|
||
Pushed by kgilbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/10ce7c1ca2d3 Enable gfxVRExternal for Android; r=kip
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/10ce7c1ca2d3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 16•6 years ago
|
||
status-firefox62=wontfix because this VR bug is not a Focus+GV blocker.
status-firefox61:
--- → wontfix
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Comment 17•6 years 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•6 years 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.
Description
•