Closed Bug 1251886 Opened 4 years ago Closed 4 years ago

WebVR rendering is broken

Categories

(Core :: Graphics: Layers, defect)

47 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: caseyyee.ca, Assigned: kip)

Details

(Whiteboard: [webvr])

Attachments

(1 file)

On the latest Firefox Nightly running Windows 10 with 0.8 Oculus runtime:

First reported on reddit: https://www.reddit.com/r/WebVR/comments/47ftcp/help_why_is_the_image_in_the_dk2_set_off_to_the/


Examples that can reproduce the issue:

- three.js VR examples renders stereo viewport offset incorrectly with a offset.
http://threejs.org/examples/#webvr_cubes

- When entering VR in any WebVR example (ie. mozvr.com), I get a cropped, rectangle view of the scene.
Flags: needinfo?(kgilbert)
Adding from Slack discussion:
@kip with checking regression using MozRegression

"I didn't see any bug for those issues specifically.  I was hoping that the MozRegression would point to an already identified regression, but it doesn't appear so"
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
I have found the cause of the issue.  The inputFrameID was coming in as -1 to gfxOculus::SubmitFrame, resulting in invalid pose information being used.

This only happens with e10s enabled.  Disabling e10s effectively works around the problem until the fix is posted.
- ContainerLayerComposite::ContainerRenderVR failed to find
  a CanvasLayerComposite and get an inputFrameId as it was
  only looking at immediate children layers.
- Initialize Oculus SDK structures with 0 to ensure code is less
  brittle when Oculus SDK is updated in the future.
- Added assert and sanity check to ensure that valid inputFrameID's
  are used when indexing the Oculus pose buffers.

Review commit: https://reviewboard.mozilla.org/r/37537/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37537/
Attachment #8725506 - Flags: review?(dmu)
Comment on attachment 8725506 [details]
MozReview Request: Bug 1251886 - Correct inputFrameID selection when using e10s

https://reviewboard.mozilla.org/r/37537/#review34365

Most of all are good. Just have a concern about layer search.

::: gfx/layers/composite/ContainerLayerComposite.cpp:287
(Diff revision 1)
> +            searchLayers.push(sibling->AsLayerComposite());

Will it add searchLayer back to searchLayers? IIUC, ```Layer* sibling = searchLayer->GetLayer();``` will get itself and push back to searchLayers? That needs to be changed to like below?
```
 if (sibling) {
     sibling = sibling->GetNextSibling();
     searchLayers.push(sibling->AsLayerComposite());
 }
```
Attachment #8725506 - Flags: review?(dmu)
https://reviewboard.mozilla.org/r/37537/#review34365

> Will it add searchLayer back to searchLayers? IIUC, ```Layer* sibling = searchLayer->GetLayer();``` will get itself and push back to searchLayers? That needs to be changed to like below?
> ```
>  if (sibling) {
>      sibling = sibling->GetNextSibling();
>      searchLayers.push(sibling->AsLayerComposite());
>  }
> ```

Just above that part, it does a "sibling = sibling->GetNextSibling()":

          Layer* sibling = searchLayer->GetLayer();

          if (sibling) {
            sibling = sibling->GetNextSibling();
          }

          if (sibling) {
            searchLayers.push(sibling->AsLayerComposite());
          }
          
I do this in a separate "if", so that I don't dereference a nullptr if there are no more siblings.
On each iteration, it should add both the first child and the next sibling so that it iterates over the entire tree of layers below the layerToRender.
Comment on attachment 8725506 [details]
MozReview Request: Bug 1251886 - Correct inputFrameID selection when using e10s

https://reviewboard.mozilla.org/r/37537/#review34611
Attachment #8725506 - Flags: review+
https://reviewboard.mozilla.org/r/37537/#review34365

> Just above that part, it does a "sibling = sibling->GetNextSibling()":
> 
>           Layer* sibling = searchLayer->GetLayer();
> 
>           if (sibling) {
>             sibling = sibling->GetNextSibling();
>           }
> 
>           if (sibling) {
>             searchLayers.push(sibling->AsLayerComposite());
>           }
>           
> I do this in a separate "if", so that I don't dereference a nullptr if there are no more siblings.
> On each iteration, it should add both the first child and the next sibling so that it iterates over the entire tree of layers below the layerToRender.

Ok. Got it!
Component: General → Graphics: Layers
Product: Firefox → Core
https://hg.mozilla.org/mozilla-central/rev/c00b55c469ec
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Casey Yee, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(caseyyee.ca)
This doesn't seem to be fixed for me in the latest Nightly (Version: 48.0a1, Build ID: 20160325085113)
Nevermind. I've run into a different bug.
(In reply to Brian Peiris from comment #14)
> Nevermind. I've run into a different bug.

What's the bug?
webvr-boilerplate unnecessarily changes the FoV of the HMD (https://github.com/borismus/webvr-boilerplate/pull/109/files) and it seems Nightly's WebVR implementation retains the FoV between sites.

Steps to repro:
1. Visit https://cdn.rawgit.com/mrdoob/three.js/r75/examples/vr_cubes.html, enter VR mode and see that the FoV is correct
2. Visit https://cdn.rawgit.com/borismus/webvr-boilerplate/ecd6afc/index.html, enter VR mode and see that Fov is incorrect
3. Visit https://cdn.rawgit.com/mrdoob/three.js/r75/examples/vr_cubes.html again and see that the FoV is incorrect this time.
looks to be now resolved.
Flags: needinfo?(caseyyee.ca)
Based on comment 17.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.