Implement chrome-only API to manage VR sessions

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

(Blocks 3 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
A set of chromeonly functions needs to be implemented and exposed to privileged code that will be managing the UI of Firefox when VR presentation is active.

These functions need to enable the switching between VR presentation layers created by the browser chrome interface and the web content.

In order to know when to switch, functions need to be provided to detect WebVR content, determine if WebVR presentation is active, and detect when content fails to submit VR frames at a sufficient rate for a comfortable VR experience.

These functions will be used to implement the VR browser UI, loading indicators during link traversal, fallback UI when pages break, and VR Lobby environments that are displayed when starting the browser natively in VR.

The first iteration of the VR UI and lobby environment will have a simple interface.  This interface will not require web content, lobby environments, or browser UI to be composited together simultaneously.  The web content will simply disappear while the user interacts with the browser interface and lobby.
Assignee

Comment 1

2 years ago
I am experimenting with an implementation based on this webidl:

partial interface Navigator {
  [ChromeOnly]
  readonly attribute boolean isWebVRContentDetected();
};

partial interface VRDisplay {
  /**
   * contentIsPresenting is true when any non-chrome content has an
   * active VR presentation.
   */
  [ChromeOnly]
  readonly attribute boolean contentIsPresenting;
  /**
   * contentVRQualityOneSecond and contentVRQualityQuarterSecond are values
   * between 0.0 and 1.0 that indicate how well non-chrome content presenting
   * to this VR display is keeping up.  A value of 1.0 indicates that there are
   * no missed frames, while a value of 0.0 means that no frames have been
   * submitted during the integration interval.
   */
  [ChromeOnly]
  readonly attribute float contentQualityOneSecond;
  /**
   * contentVRQualityOneSecond is integrated over the period of one second.
   */
  [ChromeOnly]
  readonly attribute float contentQualityQuarterSecond;
  /**
   * contentVRQualityOneSecond is integrated over the period of 1/4 second.
   */
  [ChromeOnly]
  attribute boolean suppressContent;
  /**
   * Setting suppressContent to true causes the non-chrome content to be
   * invisible, revealing the chrome content.  Non-chrome content is not
   * aware of the value of suppressContent.  VRDisplay.RequestAnimationFrame
   * will still fire while suppressContent is true and
   * contentQualityOneSecond/contentQualityQuarterSecond will continue to
   * reflect the performance of the non-chrome content.
   */
};
Assignee

Comment 2

2 years ago
These functions are not expected to be standardized or exposed to web content.  They are targeted towards the special needs of the browser UI and lobby and are expected to evolve along with the browser UI as it matures.

At some point in the future, once stabilized, these could become part of a mozbrowser-like extension for embedding VR browsers.
Assignee

Updated

2 years ago
Blocks: 1287929
Assignee

Comment 3

2 years ago
In cases where the content is too slow, it probably doesn't want to emit a vr blur event..

Think of it as a modal dialogue box being presented to the user asking if they wish to keep waiting for the unresponsive site, to go back to the previous site, or return to the lobby

The site is still the "focused" one.

We need the site to continue rendering and submitting frames so we can tell if it has resumed to a performant state.


Some cases where we would set suppressContent to true, but not emit a vrblur:

- During link traversal (Front end displays info during loading and tells user that it is not ready to display yet)
- When a site stops submitting frames or is too slow
- when the user taps a button to choose actions in the browser, but is not leaving the page.  (Maybe they are just bookmarking it or wish to refresh the page)

Cases where we would set suppressContent to true and emit a vrblur:
- We implement a way to switch between VR "tabs"
- We return to the lobby, but haven't closed the page or launched a different one
It looks good to me. The only question I had was related to the focus / blur events on the VRDisplay. It seems we want to decouple them from the supressContent flag since there are scenarios where we want to suppress content rendering but without stopping the render loop. e.g: bad content performance. We want to render a message on screen until content is able to submit frames again fast enough.
Assignee

Comment 5

2 years ago
We might need some experimentation to see if the one-second and 1/4-second intervals are most useful

I expect one to be lower latency and used to quicky detect when a site has started submitting frames

And expect the other one to be used to measure the degradation of quality and determine when to kick in and display a prompt to the user
Summary: Implement chrome-only API to manage VR sessons → Implement chrome-only API to manage VR sessions
Assignee

Comment 7

2 years ago
While content layers are hidden, it's important to prevent the content VR render loop from running unthrottled.

This is similar to the case with the Oculus Health and Safety warning in Bug 1351681.
Depends on: 1351681
Comment hidden (mozreview-request)
Assignee

Comment 9

2 years ago
I will split this patch, which includes a fix for the unthrottled render loop identified in Comment 7.
Assignee

Comment 10

2 years ago
Rather than just exposing contentQualityOneSecond and contentQualityQuarterSecond, it might be more useful to expose the timestamps of the last x number of frames and allow the chrome code implement the best integration for its purpose.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1366099
Comment hidden (mozreview-request)
Assignee

Comment 14

2 years ago
I have broken the performance monitoring parts of the API to a separate bug, Bug 1366099.
Assignee

Comment 15

2 years ago
In this iteration of the patch, I have changed a few things:

- Separating "content" and "chrome" VR sessions led to confusing function names and was too specialized.  I have replaced this with the concept of VR session "groups", which can be assigned only by Chrome code.  All VR sessions created by non-chrome code will have a group of "1".  These groups are a bitmask, enabling their visibility to be controlled with a "groupMask" attribute accessible to chrome javascript on the VRDisplay object.

- Rather than exposing contentQualityOneSecond and contentQualityQuarterSecond, I have opted to include statistics for the last 100 frames in PVRManager to be exposed to a chrome-only API.  This will allow for better dev tools, variability of the integration length, and ability to generate histograms.  I have broken the API portion of this into Bug 1366099.

- I have implemented a mechanism to throttle VR render loops for content that is either invisible due to VRDisplay.submitFrame failing internally (ie. during Oculus health and safety warning) or when not included in the "groupMask" for the VRDisplay.  Previously, the content would have run unthrottled, leaving no CPU/GPU time available to render the chrome UI and making the browser unresponsive.

- I have removed the last synchronous IPC call in PVRManager protocol, getSensorState.  The PVRManager protocol has been simplified such that a single IPC call can update the state of the VR display's sensors and trigger a VSync / VRDisplay.requestAnimationFrame callback.  This has eliminated two other IPC calls in PVRManager and is setting up for a later implementation of a shared memory lock-free data structure method of updating VR device state.  We will temporarily lose the benefit of "late-frame-pose-prediction-updates", but that is offset by the elimination of the latency added by the sync IPC call.  Later on, we will be able to switch to the lock-free IPC algorithm, updating the state such as headset predicted pose multiple times within each frame.
Assignee

Comment 16

2 years ago
I will be OOO for the next few days and hope to get this ready for landing soon after I return.

@daoshengmu I would be interested in your feedback on my approach in Comment 15 and on the implementation in my WIP patch.  Perhaps we could later roll in the gamepad pose information at a later date.
Flags: needinfo?(daoshengmu)
Flags: needinfo?(daoshengmu) → needinfo?(dmu)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8865016 [details]
Bug 1362213 - Implement chrome-only API to manage VR sessions

https://reviewboard.mozilla.org/r/136674/#review144994

::: gfx/vr/VRDisplayHost.cpp:96
(Diff revision 5)
> +   * submission.  This acts as a "watchdog" in case content fails to call
> +   * VRDisplay.SubmitFrame.
> +   * The slowest expected refresh rate for a VR display currently is an
> +   * Oculus CV1 when ASW (Asynchronous Space Warp) is enabled, at 45hz.
> +   * A kVRDisplayRAFMaxDuration value of 32 milliseconds results in a 30hz
> +   * rate, which avoids inadvertent triggering of the watchdog during

I don't understand what is the relation between kVRDisplayRAFMaxDuration to be 32 ms and 45 hz for ASW. If you look forward our the minimal FPS is 45, the max duration time should be 22 ms.
Kip, I have read your patch. I don't see any problem on your WIP patch.

IIUC, you would like to provide VR presentation layers for the browser chrome interface and the web content. In your approach, it seems to we can send a textureD3D11 that is from WebGL content and an another textureD3D11 is used for the chrome interface. Then, sending both of them to VR hardware compositor for compositing.
Flags: needinfo?(dmu)
Comment hidden (mozreview-request)
Assignee

Comment 20

2 years ago
(In reply to Daosheng Mu[:daoshengmu] from comment #17)
> Comment on attachment 8865016 [details]
> Bug 1362213 - WIP, don't commit yet
> 
> https://reviewboard.mozilla.org/r/136674/#review144994
> 
> ::: gfx/vr/VRDisplayHost.cpp:96
> (Diff revision 5)
> > +   * submission.  This acts as a "watchdog" in case content fails to call
> > +   * VRDisplay.SubmitFrame.
> > +   * The slowest expected refresh rate for a VR display currently is an
> > +   * Oculus CV1 when ASW (Asynchronous Space Warp) is enabled, at 45hz.
> > +   * A kVRDisplayRAFMaxDuration value of 32 milliseconds results in a 30hz
> > +   * rate, which avoids inadvertent triggering of the watchdog during
> 
> I don't understand what is the relation between kVRDisplayRAFMaxDuration to
> be 32 ms and 45 hz for ASW. If you look forward our the minimal FPS is 45,
> the max duration time should be 22 ms.

Thanks for the feedback Daosheng!

I've cleaned up the patch a bit and added some comments to better explain kVRDisplayRAFMaxDuration.

Once the patch passes a try push and I have done some sanity checks on the new API's, I'll assign this out to reviewers.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8865016 - Flags: review?(kyle)
Assignee

Updated

2 years ago
Attachment #8865016 - Flags: review?(kyle)
Attachment #8865016 - Flags: review?(kchen)
Attachment #8865016 - Flags: review?(bugs)
Assignee

Comment 26

2 years ago
smaug:

Would you be able to review the .webidl bits?
We are adding only "chrome-only" api's for use by later front-end, qbrt, and webextension usage.
Flags: needinfo?(bugs)
Assignee

Comment 27

2 years ago
kanru:

We have modified the PVRManager.ipdl protocol, which should be simpler now.  As we are removing the last sync message in this protocol, I have also updated the sync-messages.ini file.

Would you be able to review the PVRManager.ipdl and sync-messages.ini file changes?
Flags: needinfo?(kanru)
Assignee

Comment 28

2 years ago
Daosheng:

For some reason, Reviewboard or Bugzilla is clearing my r? for you..  Would you be able to review the rest of the patch?
Flags: needinfo?(dmu)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

2 years ago
mozreview-review
Comment on attachment 8865016 [details]
Bug 1362213 - Implement chrome-only API to manage VR sessions

https://reviewboard.mozilla.org/r/136674/#review147342

r+ for the .webidl
Attachment #8865016 - Flags: review?(bugs) → review+

Comment 32

2 years ago
mozreview-review
Comment on attachment 8865016 [details]
Bug 1362213 - Implement chrome-only API to manage VR sessions

https://reviewboard.mozilla.org/r/136674/#review147392

LGTM, just have to fix some nits and provide some info about this patch. Good Job!

::: dom/base/nsGlobalWindow.cpp:13577
(Diff revision 13)
>    MOZ_ASSERT(IsInnerWindow());
>  
> +  // Returns true only if any WebVR API call or related event
> +  // has been used
>    return mHasVREvents;
>  }

I didn't see any place would make mHasVREvent be true? Can you explain it? thanks

::: gfx/vr/VRDisplayHost.h:40
(Diff revision 13)
>    const VRDisplayInfo& GetDisplayInfo() const { return mDisplayInfo; }
>  
>    void AddLayer(VRLayerParent* aLayer);
>    void RemoveLayer(VRLayerParent* aLayer);
>  
> -  virtual VRHMDSensorState GetSensorState() = 0;
> +  

Please remove this redundant space.

::: gfx/vr/VRDisplayHost.cpp:102
(Diff revision 13)
> +   * as it would result in an unthrottled render loop that would free run at
> +   * potentially extreme frame rates.  To ensure that content has a chance to
> +   * resume its presentation when the frames are accepted once again, we rely
> +   * on this "watchdog" to act as a VR refresh driver cycling at a rate defined
> +   * by kVRDisplayRAFMaxDuration.
> +   * 

This seems to be a redundant space or new line?

::: gfx/vr/VRManager.cpp:250
(Diff revision 13)
> -    Unused << iter.Get()->GetKey()->SendNotifyVRVSync(aDisplayID);
> +  if (display) {
> +    display->StartFrame();
>    }
> +
> +  RefreshVRDisplays();
>  }

I am interested in why we do RefreshVRDisplays again here? We have already called RefreshVRDisplays at NotifyVSync().

::: gfx/vr/gfxVRPuppet.cpp:453
(Diff revision 13)
>        break;
>      }
>    }
>  
> -  // Trigger the next VSync immediately
> -  vm->NotifyVRVsync(mDisplayInfo.mDisplayID);
> +  // We will always return false for gfxVRPuppet to ensure that the fallback "watchdog"
> +  // code in VRDisplayHost::NotifyVSync() throttles the render loop.

It sounds like we prefer to let VRPuppet::SubmitFrame() be called in Vsync time (16 ms) instead of simulating real devices 90 HZ?

::: gfx/vr/ipc/VRManagerParent.h:99
(Diff revision 13)
>    virtual mozilla::ipc::IPCResult RecvControllerListenerAdded() override;
>    virtual mozilla::ipc::IPCResult RecvControllerListenerRemoved() override;
>    virtual mozilla::ipc::IPCResult RecvVibrateHaptic(const uint32_t& aControllerIdx, const uint32_t& aHapticIndex,
>                                                      const double& aIntensity, const double& aDuration, const uint32_t& aPromiseID) override;
>    virtual mozilla::ipc::IPCResult RecvStopVibrateHaptic(const uint32_t& aControllerIdx) override;
>    

Please remove this redundant space
Attachment #8865016 - Flags: review?(dmu) → review+
reviewed already.
Flags: needinfo?(dmu)
Assignee

Comment 34

2 years ago
mozreview-review-reply
Comment on attachment 8865016 [details]
Bug 1362213 - Implement chrome-only API to manage VR sessions

https://reviewboard.mozilla.org/r/136674/#review147392

> I didn't see any place would make mHasVREvent be true? Can you explain it? thanks

mHasVREvent is set to true in nsGlobalWindow::EnableVRUpdates, which was added before the patch.  This is called by nsGlobalWindow::EventListenerAdded, which will also set mHasVRDisplayActivateEvents.

> This seems to be a redundant space or new line?

Removed redundant space.  New line in comment was intentional to separate paragraph.

> I am interested in why we do RefreshVRDisplays again here? We have already called RefreshVRDisplays at NotifyVSync().

RefreshVRDisplays previously only updated state such as the proximity sensor and hardware connection and disconnection.  After this patch, it also updates the pose of the headset and needs to be called more often.

I expect this to be called even more frequently later on to update many times per frame.  When this happens, I suggest we use a persistent shared memory area and use lock-free methods to broadcast updates to all processes without an IPC queue.  This would eventually become the interface between Gecko and code shared between Servo and firefox.

> It sounds like we prefer to let VRPuppet::SubmitFrame() be called in Vsync time (16 ms) instead of simulating real devices 90 HZ?

I have updated the comment with some more details:

```
// This "watchdog" will
// result in a refresh rate that is quite low compared to real hardware, but should be
// sufficient for non-performance oriented tests.  If we wish to simulate realistic frame
// rates with VRDisplayPuppet, we should block here for the appropriate amount of time and
// return true to indicate that we have blocked.
```

Perhaps we could add a preference or API function to set the simulated frame rate of the puppet device.  This would control the amount of time to block in VRDisplayPuppet::SubmitFrame.  Once VRDisplayPuppet::SubmitFrame is blocking, we can return false there to disable the "watchdog" function.
Comment hidden (mozreview-request)
Assignee

Comment 36

2 years ago
Thanks @daosheng.  I have updated the patch with your feedback.
Assignee

Comment 37

2 years ago
Thanks for the review!  I'll remove the redundant NI..
Flags: needinfo?(bugs)
Assignee

Updated

2 years ago
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Blocks: 1188199

Comment 38

2 years ago
mozreview-review
Comment on attachment 8865016 [details]
Bug 1362213 - Implement chrome-only API to manage VR sessions

https://reviewboard.mozilla.org/r/136674/#review148284

r+ for the sync removal
Attachment #8865016 - Flags: review?(kchen) → review+
Comment hidden (mozreview-request)

Comment 40

2 years ago
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/155a190e5c86
Implement chrome-only API to manage VR sessions r=daoshengmu,kanru,smaug
Assignee

Comment 41

2 years ago
Clearing redundant NI for Kanru.  Thanks for reviewing!
Flags: needinfo?(kanru)

Comment 42

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/155a190e5c86
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Depends on: 1369772
You need to log in before you can comment on or make changes to this bug.