Closed Bug 1407423 Opened 7 years ago Closed 7 years ago

When no HMD is connected, Firefox is not unloading the Oculus ovrLib dll when requested by the Oculus runtime

Categories

(Core :: WebVR, defect, P2)

Product:
Core
Component:
WebVR
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: kip, Assigned: kip)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1407423 - Ensure that any time we have loaded the Oculus runtime libary, we are polling ShouldQuit,
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1407423 - Part 2: Update gfxVRGVR to match refactoring,
59 bytes, text/x-review-board-request
rbarker
: review+
Details
Bug 1407423 - Part 3: Update Puppet Device Implementation,
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1407423 - Part 4: Disable broken mochitest on Android (To fix in Bug 1348246)
59 bytes, text/x-review-board-request
Details
Bug 1407423 - Part 5: Remove assert from test_vrController_displayId.html
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details 
Normal WebVR Oculus bootstrap, with an HMD connected:
- User Visits a WebVR site
- Load ovrLib into the Firefox “GPU” process.
- Call ovr_Initialize with ovrInit_RequestVersion | ovrInit_MixedRendering | ovrInit_Invisible
- Call ovr_GetHmdDesc(NULL)
- When returned ovrHmdDesc::Type != ovrHmd_None, the headset is detected so we continue...
- Create a session with ovr_Create
- Call ovr_SetTrackingOriginType with ovrTrackingOrigin_FloorLevel
- While any WebVR site is loaded, we continuously poll ovr_GetSessionStatus, ovr_GetPredictedDisplayTime, and ovr_GetTrackingState

- If the WebVR page starts a VR presentation:
	- We stop the existing "ovrInit_Invisible" session:
		- Call ovr_Destroy
		- Call ovr_shutdown
		- (We don't unload and re-load the ovrLib dll when re-creating sessions)
	- Create a new session without "ovrInit_Invisible":
		- Call ovr_Initialize with ovrInit_RequestVersion | ovrInit_MixedRendering
	- Create swap chain and buffers for Oculus (ovr_CreateTextureSwapChainDX, ovr_GetTextureSwapChainLength, ovr_GetTextureSwapChainBufferDX)
	- Poll ovr_GetSessionStatus, ovr_GetPredictedDisplayTime, and ovr_GetTrackingState every submitted frame (called at least 15hz if web content fails to submit frames)
	- When frames submitted by content we call ovr_CommitTextureSwapChain, and ovr_Submit

- There are two shutdown conditions:
	- If ovr_GetSessionStatus returns with ovrSessionStatus::ShouldQuit true, we immediately shutdown
	- When the user hasn't visited a WebVR site for 30 seconds, a timer is started.  Shutdown starts when this timer elapses.

- Shutting down sequence:
	- Call ovr_Destroy
	- Call ovr_shutdown
	- Unload the ovrLib dll from the Firefox "GPU" process.


Sequence when no HMD is connected:
- User Visits a WebVR site
- Load ovrLib into the Firefox “GPU” process.
- Call ovr_Initialize with ovrInit_RequestVersion | ovrInit_MixedRendering | ovrInit_Invisible
- Call ovr_GetHmdDesc(NULL)
- While any WebVR site is loaded, we continuously poll ovr_GetHmdDesc(NULL), which doesn't find any headset
- When the user leaves the WebVR site a 30 second timer is started.
- When the timer elapses, the ovrLib dll is unloaded from the "GPU" process

The problem in the second case (no HMD connected), is that it keeps the ovrLib dll loaded without polling ovr_GetSessionStatus.

Keeping the ovRLib dll loaded can prevent the Oculus runtime from running its self-update.
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox58: --- → affected
Priority: -- → P2
Blocks: 1384279
Attachment #8925749 - Flags: review?(dmu)
Comment on attachment 8925749 [details]
Bug 1407423 - Ensure that any time we have loaded the Oculus runtime libary, we are polling ShouldQuit,

https://reviewboard.mozilla.org/r/196904/#review202132

lgtm. Thanks.

::: gfx/vr/gfxVROculus.h:104
(Diff revision 1)
>    virtual bool SubmitFrame(ID3D11Texture2D* aSource,
>                             const IntSize& aSize,
>                             const gfx::Rect& aLeftEyeRect,
>                             const gfx::Rect& aRightEyeRect) override;
>    void UpdateStageParameters();
> -
> +  

redundant space.
Attachment #8925749 - Flags: review?(dmu) → review+
The VR reftests and mochitests are failing with this patchset.

I've got a fix that involves updating the puppet display and controller implementation to more closely match the way the real hardware interfaces are implemented.  I'll add that as a 3rd patch in the series here before landing.
Comment on attachment 8928362 [details]
Bug 1407423 - Part 3: Update Puppet Device Implementation

https://reviewboard.mozilla.org/r/199574/#review204686


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/vr/gfxVRPuppet.cpp:769
(Diff revision 1)
>  bool
>  VRSystemManagerPuppet::GetIsPresenting()
>  {
> -  if (mPuppetHMD) {
> -    VRDisplayInfo displayInfo(mPuppetHMD->GetDisplayInfo());
> -    return displayInfo.GetPresentingGroups() != kVRGroupNone;
> +  for (auto iter = mPuppetHMDs.Iter(); !iter.Done(); iter.Next()) {
> +    VRDisplayPuppet* display = iter.UserData();
> +    VRDisplayInfo displayInfo(display->GetDisplayInfo());

Warning: The variable 'displayinfo' is copy-constructed from a const reference but is only used as const reference; consider making it a const reference [clang-tidy: performance-unnecessary-copy-initialization]

    VRDisplayInfo displayInfo(display->GetDisplayInfo());
                  ^
    const        &
Comment on attachment 8928273 [details]
Bug 1407423 - Part 2: Update gfxVRGVR to match refactoring,

https://reviewboard.mozilla.org/r/199488/#review204886
Attachment #8928273 - Flags: review?(rbarker) → review+
Comment on attachment 8928362 [details]
Bug 1407423 - Part 3: Update Puppet Device Implementation

https://reviewboard.mozilla.org/r/199574/#review206176


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/vr/gfxVRPuppet.cpp:712
(Diff revision 2)
>  void
>  VRSystemManagerPuppet::NotifyVSync()
>  {
>    VRSystemManager::NotifyVSync();
> -  if (mPuppetHMD) {
> -    mPuppetHMD->Refresh();
> +
> +  for (auto display: mPuppetHMDs) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

  for (auto display: mPuppetHMDs) {
            ^
       const  &

::: gfx/vr/gfxVRPuppet.cpp:787
(Diff revision 2)
>  }
>  
>  bool
>  VRSystemManagerPuppet::GetIsPresenting()
>  {
> -  if (mPuppetHMD) {
> +  for (auto display: mPuppetHMDs) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

  for (auto display: mPuppetHMDs) {
            ^
       const  &
Attachment #8928362 - Flags: review?(dmu)
Comment on attachment 8928362 [details]
Bug 1407423 - Part 3: Update Puppet Device Implementation

https://reviewboard.mozilla.org/r/199574/#review207292

lgtm. just some nits for fixing. Good job!

::: gfx/vr/gfxVRPuppet.cpp:757
(Diff revision 7)
> -  if (VRSystemManager::ShouldInhibitEnumeration()) {
> -    return true;
> +  if (aDeviceID >= mPuppetDisplayCount) {
> +    MOZ_ASSERT(false);
> +    return;
>    }
> -  if (mPuppetHMD) {
> -    // When we find an a VR device, don't
> +  mPuppetDisplayInfo[aDeviceID] = aDisplayInfo;
> +  if (mPuppetHMDs.Length() > aDeviceID) {

It looks like mPuppetHMDs.Length() should not <= aDeviceID? Do we need put an assersion or move it as an early return?
Like:

if (aDeviceID >= mPuppetDisplayCount ||
    mPuppetHMDs.Length() <= aDeviceID) {
    MOZ_ASSERT(false);
    return;
}
Then, we just set mPuppetHMDs[aDeviceID]->SetDisplayInfo(aDisplayInfo); directly.

::: gfx/vr/gfxVRPuppet.cpp:771
(Diff revision 7)
> +  if (aDeviceID >= mPuppetDisplayCount) {
> +    MOZ_ASSERT(false);
> +    return;
> +  }
> +  mPuppetDisplaySensorState[aDeviceID] = aSensorState;
> +  if (mPuppetHMDs.Length() > aDeviceID) {

same here.

::: gfx/vr/ipc/VRManagerParent.cpp:299
(Diff revision 7)
>        }
>        ++controllerIdx;
>      }
>    }
>  
> -  MOZ_ASSERT(controllerPuppet);
> +  

redundant space

::: gfx/vr/ipc/VRManagerParent.cpp:349
(Diff revision 7)
> +  // aDeviceID for controllers start at 1 and are
> +  // used as a key to mVRControllerTests
>    RefPtr<impl::VRControllerPuppet> controllerPuppet;
>    mVRControllerTests.Get(aDeviceID,
>                           getter_AddRefs(controllerPuppet));
>    MOZ_ASSERT(controllerPuppet);

Please add an assertion for aDeviceID < 1
Attachment #8928362 - Flags: review?(dmu) → review+
Attachment #8928362 - Attachment is obsolete: true
(In reply to Daosheng Mu[:daoshengmu] from comment #24)
> Comment on attachment 8928362 [details]
> Bug 1407423 - Part 3: Update Puppet Device Implementation
> 
> https://reviewboard.mozilla.org/r/199574/#review207292
> 
> lgtm. just some nits for fixing. Good job!
> 
> ::: gfx/vr/gfxVRPuppet.cpp:757
> (Diff revision 7)
> > -  if (VRSystemManager::ShouldInhibitEnumeration()) {
> > -    return true;
> > +  if (aDeviceID >= mPuppetDisplayCount) {
> > +    MOZ_ASSERT(false);
> > +    return;
> >    }
> > -  if (mPuppetHMD) {
> > -    // When we find an a VR device, don't
> > +  mPuppetDisplayInfo[aDeviceID] = aDisplayInfo;
> > +  if (mPuppetHMDs.Length() > aDeviceID) {
> 
> It looks like mPuppetHMDs.Length() should not <= aDeviceID? Do we need put
> an assersion or move it as an early return?
> Like:
> 
> if (aDeviceID >= mPuppetDisplayCount ||
>     mPuppetHMDs.Length() <= aDeviceID) {
>     MOZ_ASSERT(false);
>     return;
> }
> Then, we just set mPuppetHMDs[aDeviceID]->SetDisplayInfo(aDisplayInfo);
> directly.
There are cases when in normal operation mPuppetHMDs would not be
populated yet.  When this happens, we still need to update
mPuppetDisplayInfo. I've added some comments to the code to help
explain these cases.

> 
> ::: gfx/vr/gfxVRPuppet.cpp:771
> (Diff revision 7)
> > +  if (aDeviceID >= mPuppetDisplayCount) {
> > +    MOZ_ASSERT(false);
> > +    return;
> > +  }
> > +  mPuppetDisplaySensorState[aDeviceID] = aSensorState;
> > +  if (mPuppetHMDs.Length() > aDeviceID) {
> 
> same here.

There are cases when in normal operation mPuppetHMDs would not be
populated yet.  When this happens, we still need to update
mPuppetDisplaySensorState. I've added some comments to the code to help
explain these cases.
> 
> ::: gfx/vr/ipc/VRManagerParent.cpp:299
> (Diff revision 7)
> >        }
> >        ++controllerIdx;
> >      }
> >    }
> >  
> > -  MOZ_ASSERT(controllerPuppet);
> > +  
> 
> redundant space
Fixed

> 
> ::: gfx/vr/ipc/VRManagerParent.cpp:349
> (Diff revision 7)
> > +  // aDeviceID for controllers start at 1 and are
> > +  // used as a key to mVRControllerTests
> >    RefPtr<impl::VRControllerPuppet> controllerPuppet;
> >    mVRControllerTests.Get(aDeviceID,
> >                           getter_AddRefs(controllerPuppet));
> >    MOZ_ASSERT(controllerPuppet);
> 
> Please add an assertion for aDeviceID < 1
Fixed
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b625d265761
Ensure that any time we have loaded the Oculus runtime libary, we are polling ShouldQuit,r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/16d247b8091d
Part 2: Update gfxVRGVR to match refactoring,r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9dbe5af9a06
Part 3: Update Puppet Device Implementation,r=daoshengmu
Permaorange on opt, and merely intermittent on debug, not sure how that could be.
See Also: → 1420167
See Also: → 1348246
This patchset causes more failures of the dom/vr/test/test_vrDisplay_getFrameData.html mochitest on Android, likely due to differences in how vsync refreshes are pushed (or not pushed on Android) to VRManager.  We will be supporting WebVR on Android through GeckoView, which will be pushing VSyncs to VRManager is a different way when this lands.

Essentially, the issues are with the test but not with the changes in this bug's patch set.

I would like to disable the test for now on Android so the refactoring in this patchset can land, and have the test re-enabled for Android once the intermittent test failures are corrected in Bug 1348246.
Attachment #8933478 - Flags: review?(dmu)
Comment on attachment 8931113 [details]
Bug 1407423 - Part 3: Update Puppet Device Implementation,

Note that this was already r+'ed by :daoshengmu, but after rebasing MozReview cleared the r+ flag.  I'll add r=daosheng during push to m-i
Attachment #8931113 - Flags: review?(dmu)
Comment on attachment 8933478 [details]
Bug 1407423 - Part 4: Disable broken mochitest on Android (To fix in Bug 1348246)

https://reviewboard.mozilla.org/r/204410/#review209992

ok. Let's enable it again after Bug 1348246.
Attachment #8933478 - Flags: review?(dmu) → review+
Comment on attachment 8931113 [details]
Bug 1407423 - Part 3: Update Puppet Device Implementation,

https://reviewboard.mozilla.org/r/202194/#review210870

::: gfx/vr/ipc/VRManagerParent.cpp
(Diff revision 3)
>  {
>    VRManager* vm = VRManager::Get();
>    vm->CreateVRTestSystem();
> -  mDisplayTestID = 0;
> -  mControllerTestID = 0;
> +  // The mControllerTestID is 1 based
> +  mControllerTestID = 1;
> -  mVRDisplayTests.Clear();

Why doesn't mVRDisplayTests need to be cleared? mVRControllerTests needs to be cleared that is for sure. Otherwise, when re-running the same test, the controller index from the array would be accumlated.

Please run `./mach mochitest --repeat=5 dom/vr/test/mochitest/test_vrController_displayId.html` in local and make sure there is no error before land.
Attachment #8931113 - Flags: review?(dmu) → review+
Attachment #8934359 - Flags: review?(dmu)
Test Validation has passed on the last try run.  Hopefully with this part 5 fix, the changes can stick
(In reply to Daosheng Mu[:daoshengmu] from comment #44)
> Comment on attachment 8931113 [details]
> Bug 1407423 - Part 3: Update Puppet Device Implementation,
> 
> https://reviewboard.mozilla.org/r/202194/#review210870
> 
> ::: gfx/vr/ipc/VRManagerParent.cpp
> (Diff revision 3)
> >  {
> >    VRManager* vm = VRManager::Get();
> >    vm->CreateVRTestSystem();
> > -  mDisplayTestID = 0;
> > -  mControllerTestID = 0;
> > +  // The mControllerTestID is 1 based
> > +  mControllerTestID = 1;
> > -  mVRDisplayTests.Clear();
> 
> Why doesn't mVRDisplayTests need to be cleared? mVRControllerTests needs to
> be cleared that is for sure. Otherwise, when re-running the same test, the
> controller index from the array would be accumlated.
> 
> Please run `./mach mochitest --repeat=5
> dom/vr/test/mochitest/test_vrController_displayId.html` in local and make
> sure there is no error before land.
With the refactoring, mVRDisplayTests doesn’t exist any more in VRManagerParent.  The call to CreateVRTestSystem will clear them out from the VRPuppetManager, which tracks them now.  They are always created and destroyed in VRPupperManager::Enumerate now, so we can excersize more of the VRDisplayHost and VRManager code and catch issues sooner.
Comment on attachment 8934359 [details]
Bug 1407423 - Part 5: Remove assert from test_vrController_displayId.html

https://reviewboard.mozilla.org/r/205288/#review210906

r=me after verifying the mVRDisplayTests is not the issue.

::: dom/vr/test/mochitest/test_vrController_displayId.html
(Diff revision 1)
>            listenControllerEvents();
>            return VRSimulationDriver.AttachWebVRDisplay().then(() => {
>              return navigator.getVRDisplays().then((displays) => {
>                vrDisplay = displays[0];
>                assert_equals(displays.length, 1, "displays.length must be one after attach.");
> -              assert_equals(displays[0].displayId, 1, "displayId must be one.");

The purpose for this test is for verifying the vrController.displayId is equal to vrDisplay.displayId. I think it is ok to remove this line.

But when running Puppet test without other real devices, the vrDisplay.displayId would be 1, right? Is it possible that you forget to clear mVRDisplayTests as I mentioned at the patch 3.
Attachment #8934359 - Flags: review?(dmu) → review+
Comment on attachment 8931113 [details]
Bug 1407423 - Part 3: Update Puppet Device Implementation,

https://reviewboard.mozilla.org/r/202194/#review210870

> Why doesn't mVRDisplayTests need to be cleared? mVRControllerTests needs to be cleared that is for sure. Otherwise, when re-running the same test, the controller index from the array would be accumlated.
> 
> Please run `./mach mochitest --repeat=5 dom/vr/test/mochitest/test_vrController_displayId.html` in local and make sure there is no error before land.

With the refactoring, mVRDisplayTests doesn’t exist any more in VRManagerParent.  The call to CreateVRTestSystem will clear them out from the VRPuppetManager, which tracks them now.  They are always created and destroyed in VRPupperManager::Enumerate now, so we can excersize more of the VRDisplayHost and VRManager code and catch issues sooner.
(In reply to Daosheng Mu[:daoshengmu] from comment #47)
> Comment on attachment 8934359 [details]
> Bug 1407423 - Part 5: Remove assert from test_vrController_displayId.html
> 
> https://reviewboard.mozilla.org/r/205288/#review210906
> 
> r=me after verifying the mVRDisplayTests is not the issue.
> 
> ::: dom/vr/test/mochitest/test_vrController_displayId.html
> (Diff revision 1)
> >            listenControllerEvents();
> >            return VRSimulationDriver.AttachWebVRDisplay().then(() => {
> >              return navigator.getVRDisplays().then((displays) => {
> >                vrDisplay = displays[0];
> >                assert_equals(displays.length, 1, "displays.length must be one after attach.");
> > -              assert_equals(displays[0].displayId, 1, "displayId must be one.");
> 
> The purpose for this test is for verifying the vrController.displayId is
> equal to vrDisplay.displayId. I think it is ok to remove this line.
> 
> But when running Puppet test without other real devices, the
> vrDisplay.displayId would be 1, right? Is it possible that you forget to
> clear mVRDisplayTests as I mentioned at the patch 3.

This was failing during the test validation on the try server.  Its now possible to create more than one puppet display; however, the tests only create one and they are cleared before the next test run.  The difference with the updated implementation; however, is that if the display is removed and re-added, it gets a new displayId returned by VRSystemManager::AllocateDisplayID() in the VRDisplayHost's CTOR.  In this case, there is still only one display, but with a new displayID value.
(In reply to :kip (Kearwood Gilbert) from comment #49)
> (In reply to Daosheng Mu[:daoshengmu] from comment #47)
> > Comment on attachment 8934359 [details]
> > Bug 1407423 - Part 5: Remove assert from test_vrController_displayId.html
> > 
> > https://reviewboard.mozilla.org/r/205288/#review210906
> > 
> > r=me after verifying the mVRDisplayTests is not the issue.
> > 
> > ::: dom/vr/test/mochitest/test_vrController_displayId.html
> > (Diff revision 1)
> > >            listenControllerEvents();
> > >            return VRSimulationDriver.AttachWebVRDisplay().then(() => {
> > >              return navigator.getVRDisplays().then((displays) => {
> > >                vrDisplay = displays[0];
> > >                assert_equals(displays.length, 1, "displays.length must be one after attach.");
> > > -              assert_equals(displays[0].displayId, 1, "displayId must be one.");
> > 
> > The purpose for this test is for verifying the vrController.displayId is
> > equal to vrDisplay.displayId. I think it is ok to remove this line.
> > 
> > But when running Puppet test without other real devices, the
> > vrDisplay.displayId would be 1, right? Is it possible that you forget to
> > clear mVRDisplayTests as I mentioned at the patch 3.
> 
> This was failing during the test validation on the try server.  Its now
> possible to create more than one puppet display; however, the tests only
> create one and they are cleared before the next test run.  The difference
> with the updated implementation; however, is that if the display is removed
> and re-added, it gets a new displayId returned by
> VRSystemManager::AllocateDisplayID() in the VRDisplayHost's CTOR.  In this
> case, there is still only one display, but with a new displayID value.

After some investigation, it appears that the test failures were intermittent due to having a vsync or not having a vsync between the test system creation and the puppet display creation.  When there is a vsync between these events, the display is deleted and then re-created rather than recycled between iterations of the test.
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cee79cb6c23
Ensure that any time we have loaded the Oculus runtime libary, we are polling ShouldQuit,r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8b24db533eb
Part 2: Update gfxVRGVR to match refactoring,r=rbarker
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d74edc6164
Part 3: Update Puppet Device Implementation,r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/a379a8106123
Part 4: Disable broken mochitest on Android (To fix in Bug 1348246),r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/46ae1d92851d
Part 5: Remove assert from test_vrController_displayId.html,r=daoshengmu
Depends on: 1424378
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: