Closed Bug 1381085 Opened 7 years ago Closed 7 years ago

When WebVR is presenting, the 2d window is repainted solid white when resized

Categories

(Core :: WebVR, defect, P1)

Product:
Core
Component:
WebVR
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: kip, Assigned: kip)

References

(
URL
)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1381085 - Submit VR frames with a separate ID3DDeviceContextState
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1381085 (Beta Uplift) - Submit VR frames with a separate ID3DDeviceContextState
12.78 KB, patch
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
STR:
- visit https://webvr.info/samples/03-vr-presentation.html
- Click the "Enter VR" button on the page (tested with an Oculus CV1 headset)
- Resize the 2d window to be more than 1/4 the area of the monitor (tested with a UHD resolution display)
- As each part of the window is repainted, including the browser chrome, it is filled with solid white.
- Resizing the window back to a smaller size results in the window being repainted normally again.
- After pressing "Exit VR", the effect is no longer seen.

A mozregression points to bug 1375743:

Enable Advanced Layers on Nightly Windows 8+ builds. (bug 1375743, r=milan)
Assignee: nobody → kgilbert
Also of note, the headset shows the VR content correctly during the STR above.
URL: https://webvr.info/samples/03-vr-pres...
Has Regression Range: --- → yes
Disabling the Advanced Layers works around the issue:

layers.mlgpu.dev-enabled = false
I can reproduce it when running https://webvr.info/samples/XX-vr-controllers.html with Oculus Rift after entering the present mode at Nightly debug build.

After layers.mlgpu.dev-enabled = false, it will be good.
Blocks: 1375743
The problem is that we are rendering to the same D3D11 device context as the 2D Compositor, which attempts to reduce state changes by mirroring the DX11 state.

When the VR code sets up the framebuffer, shader, and resources for Oculus and blits the texture in, it doesn't update the mirrored state in the 2d compositor.

My solution will be to use a separate DX11 context for the WebVR code.  This is something we will need anyways when we move it out to its own process.
WIP Patch, do not commit.

I have confirmed that using a separate D3D11 device context solves the issue.
Blocks: 1365336
Blocks: 1314182
This patch now includes refactoring aligned with other work in WebGLContext and HTMLCanvasElement to eliminate the mirror specific render paths.

This new implementation for the 2d mirroring also corrects the handling of preserveDrawingBuffer, fixing the mirroring on some sites like Sketchfab.

This patch is nearly ready to assign to review and land -- before landing it, I need to add a call-back in PVRManager to the child process to inform that the VR compositing is done with the texture.  (See the "HACK!" keyword in my patch)
See Also: → 1382104
Bug 1382104 includes a patch that does much of the same refactoring included in this patch, removing IsMirror:

"Bug 1382104 - Remove IsMirror concept in favor of checking forwarders"
Blocks: 1394600
Splitting patches for easier review...

VRDisplay.requestFrame bug fix split to separate patch in Bug 1394600.
Comment on attachment 8902044 [details]
Bug 1381085 - Remove IsMirror concept in favor of checking forwarders.

This patch should be the same as the one in Bug 1382104:

"Bug 1382104 - Remove IsMirror concept in favor of checking forwarders"

I've duplicated it here so this can land independently and be easily uplifted to Beta, but should apply it only in the bug that lands first.
Attachment #8902044 - Flags: review?(jgilbert)
Comment on attachment 8902045 [details]
Bug 1381085 - Part 1: Create new D3D11Device for WebVR

To avoid stomping over the D3D11 mirrored state that the new advanced layers tracks, we create a new context for the VR compositing here.
Attachment #8902045 - Flags: review?(jgilbert)
Comment on attachment 8893158 [details]
Bug 1381085 - Part 2: Move VR to its own D3D11Device

Jeff: Could you please review the bits outside of gfx/vr and dom/vr?  Also, could you check out our changes to the ipdl?

Daosheng: Please check the gfx/vr and dom/vr parts
Attachment #8893158 - Flags: review?(jgilbert)
Attachment #8893158 - Flags: review?(dmu)
Comment on attachment 8893158 [details]
Bug 1381085 - Part 2: Move VR to its own D3D11Device

https://reviewboard.mozilla.org/r/164162/#review179348

r=me for gfx/vr and dom/vr parts.

::: gfx/vr/gfxVROculus.cpp:1114
(Diff revision 9)
> -    NS_WARNING("Failed to get SRV for Oculus");
> +  if (!device) {
>      return false;
>    }
> -  mContext->PSSetShaderResources(0 /* 0 == TexSlot::RGB */, 1, &srView);
> +
> +  RefPtr<ID3D11ShaderResourceView> srView;
> +  HRESULT hr = device->CreateShaderResourceView(aSource, nullptr, getter_AddRefs(srView));

I feel calling CreateShaderResourceView() at VSync update loop will bring some penalty on performance.

Can we keep the srcView ptr for reusing?

::: gfx/vr/gfxVRPuppet.cpp:447
(Diff revision 9)
>        mContext->PSSetShader(mQuadPS, nullptr, 0);
> -      ID3D11ShaderResourceView* srView = aSource->GetShaderResourceView();
> -      if (!srView) {
> -        NS_WARNING("Failed to get SRV for Puppet");
> -        return false;
> +
> +      RefPtr<ID3D11ShaderResourceView> srView;
> +      HRESULT hr = device->CreateShaderResourceView(aSource, nullptr, getter_AddRefs(srView));
> +      if (FAILED(hr) || !srView) {
> +        gfxWarning() << "Could not create shader resource view for Puppet: " << hexa(hr);

Same here.

I feel calling CreateShaderResourceView() at VSync update loop will bring some penalty on performance.

Can we keep the srcView ptr for reusing?
Attachment #8893158 - Flags: review?(dmu) → review+
Comment on attachment 8902044 [details]
Bug 1381085 - Remove IsMirror concept in favor of checking forwarders.

https://reviewboard.mozilla.org/r/173450/#review179824

IIRC this patch is insufficient to guarantee correct behavior by itself, but I don't remember exactly why. I needed to update how we pull frames from WebGL in order for this to work properly.

Marking r- for now, because of this.
Attachment #8902044 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #27)
> Comment on attachment 8902044 [details]
> Bug 1381085 - Remove IsMirror concept in favor of checking forwarders.
> 
> https://reviewboard.mozilla.org/r/173450/#review179824
> 
> IIRC this patch is insufficient to guarantee correct behavior by itself, but
> I don't remember exactly why. I needed to update how we pull frames from
> WebGL in order for this to work properly.
> 
> Marking r- for now, because of this.

On its own, this patch would not work properly.  The other, "Bug 1381085 - Move VR to its own D3D11Device" patch removes the code that needed the mirroring, making the code removed in this patch effectively dead.

With all these patches in place, we are restoring the WebGL and Canvas element code back to how it was prior to the WebVR implementation and taking another approach.

The new approach no longer involves creating a new TextureForwarder and calling Morph().  Rather, it steals the shared texture from the existing Textureclient, sends it directly to the VR compositor, and ensures that the Textureclient lives long enough and doesn't get recycled into the pool until we have finished reading from the texture.  Effectively, it solves the issue we were working around initially -- sharing a texture made in one thread with two others.

After seeing what I am doing with the other patch, could you let me know if this is a good direction?  Also, could you advise if my way of pulling the frames would run into the same issues you were referring to?
Flags: needinfo?(jgilbert)
(In reply to :kip (Kearwood Gilbert) from comment #6)
> The problem is that we are rendering to the same D3D11 device context as the
> 2D Compositor, which attempts to reduce state changes by mirroring the DX11
> state.
> 
> When the VR code sets up the framebuffer, shader, and resources for Oculus
> and blits the texture in, it doesn't update the mirrored state in the 2d
> compositor.
> 
> My solution will be to use a separate DX11 context for the WebVR code.  This
> is something we will need anyways when we move it out to its own process.

You can use ID3DDeviceContextState for this.

Bug 1382104 has/will have a change to how frames are pulled from WebGL. I really really do not want to have related changes go in before that, given how long it's already taken to stabilize.
Flags: needinfo?(jgilbert)
Blocks: 1385051
(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> (In reply to :kip (Kearwood Gilbert) from comment #6)
> > The problem is that we are rendering to the same D3D11 device context as the
> > 2D Compositor, which attempts to reduce state changes by mirroring the DX11
> > state.
> > 
> > When the VR code sets up the framebuffer, shader, and resources for Oculus
> > and blits the texture in, it doesn't update the mirrored state in the 2d
> > compositor.
> > 
> > My solution will be to use a separate DX11 context for the WebVR code.  This
> > is something we will need anyways when we move it out to its own process.
> 
> You can use ID3DDeviceContextState for this.
> 
> Bug 1382104 has/will have a change to how frames are pulled from WebGL. I
> really really do not want to have related changes go in before that, given
> how long it's already taken to stabilize.

The motivation for pushing this bug through, is that it prevents major browser breakage after Advanced Layers are turned on (Bug 1385051).  IIUC, advanced layers will be enabled in FF57.  Is your patch planned to land within FF57 as well?

If it makes it easier, I can defer much of the cleanup (ie. removing dead code) and minify this patch a bit, with any other feedback you have.  I can also rebase it on top of your patches and line it up for landing after yours.

Would that help?  I would like to coordinate with you in any way to be sure not to de-stabilize your work.
Flags: needinfo?(jgilbert)
We've been trying to keep the number of D3D devices low, so that's another reason to investigate using an ID3DDeviceContextState object. Can you give that a shot before we move forward on these more invasive changes?
Flags: needinfo?(jgilbert) → needinfo?(kgilbert)
(In reply to Jeff Gilbert [:jgilbert] from comment #31)
> We've been trying to keep the number of D3D devices low, so that's another
> reason to investigate using an ID3DDeviceContextState object. Can you give
> that a shot before we move forward on these more invasive changes?

I'll give it a try, thanks!

Later, we will need the full D3D11Device as we will be moving the VR code out to its own process.  The immediate need for this bug may be solved with the ID3DDeviceContextState.
Flags: needinfo?(kgilbert)
That's fine. We're just trying to keep the number of devices per process down.
WIP patch splitting..  Moved much of the patch to Bug 1394600
Blocks: 1400407
Attachment #8902044 - Attachment is obsolete: true
I have moved all the cleanup refactoring to a separate bug that can be landed after this one, bug 1400407.

Now just need to update the patches to use ID3DDeviceContextState rather than a separate ID3D11Device.
It is still necessary to keep the refactoring that changes the PVRLayer protocol to use a SurfaceDescriptor rather than PTexture due to TextureHost and its friends all being hard coded to use the compositor's ID3D11Device.

The alternative would be to make extensive changes to many classes including TextureHost and TextureParent.  The decoupling of the VR rendering pipeline from PTexture, TextureHost, and TextureParent will also make upcoming work easier as we move WebVR's host side to its own process and thread using code shared with Servo.
I may be able to create a simpler patch based on ID3DDeviceContextState to fix the original issue for FF57, then land these patches in FF58+ to prepare for upcoming WebVR threading and process changes in Bug 1362578.
No longer blocks: 1400407
Attachment #8902045 - Attachment is obsolete: true
Attachment #8893158 - Attachment is obsolete: true
Comment on attachment 8909541 [details]
Bug 1381085 - Submit VR frames with a separate ID3DDeviceContextState

https://reviewboard.mozilla.org/r/181008/#review186264

::: gfx/vr/VRDisplayHost.h:65
(Diff revision 2)
>    bool GetIsConnected();
>  
> +  bool SaveRenderState();
> +  void RestoreRenderState();
> +
> +  class AutoRestoreRenderState {

Generally this sort of class should contain the previous value to restore, instead of reaching in to another class.

See ScopedContextState in gfx/gl/SharedSurfaceD3D11Interop.cpp.
Comment on attachment 8909541 [details]
Bug 1381085 - Submit VR frames with a separate ID3DDeviceContextState

https://reviewboard.mozilla.org/r/181008/#review186264

> Generally this sort of class should contain the previous value to restore, instead of reaching in to another class.
> 
> See ScopedContextState in gfx/gl/SharedSurfaceD3D11Interop.cpp.

I have moved the member holding the previous value to the AutoRestoreRenderState class.  The other D3D11 objects (Device, DeviceContext, and DeviceContextState) are accessed from VRDisplayHost.

Note that this code will likely be temporary, as we will need our own independent D3D11Device when we move this code to its own process in FF58.
I have also updated the patch to fix the non-windows builds and static analysis errata found in the prior try push.
Attachment #8909541 - Flags: review?(dmu)
(In reply to Jeff Gilbert [:jgilbert] from comment #47)
> Comment on attachment 8909541 [details]
> Bug 1381085 - Submit VR frames with a separate ID3DDeviceContextState
> 
> https://reviewboard.mozilla.org/r/181008/#review186264
> 
> ::: gfx/vr/VRDisplayHost.h:65
> (Diff revision 2)
> >    bool GetIsConnected();
> >  
> > +  bool SaveRenderState();
> > +  void RestoreRenderState();
> > +
> > +  class AutoRestoreRenderState {
> 
> Generally this sort of class should contain the previous value to restore,
> instead of reaching in to another class.
> 
> See ScopedContextState in gfx/gl/SharedSurfaceD3D11Interop.cpp.

Thanks for the feedback, I have updated the patch based on your suggestion.

I have assigned the new patch for review by @daosheng, as it is now self-contained in /gfx/vr/.  If you have more feedback, or would prefer to be the reviewer for this, please let me know.  Note that this is time critical (needed before FF57 uplift to avoid whole-browser breakage when users hit webvr sites).
Flags: needinfo?(jgilbert)
Severity: normal → major
status-firefox57: --- → affected
Priority: -- → P1
Comment on attachment 8909541 [details]
Bug 1381085 - Submit VR frames with a separate ID3DDeviceContextState

https://reviewboard.mozilla.org/r/181008/#review186862

r=me. Please make sure the D3D_FEATURE_LEVEL for d3d11 devices.

::: gfx/vr/VRDisplayHost.cpp:105
(Diff revision 3)
> +  if (!mDeviceContextState) {
> +    D3D_FEATURE_LEVEL featureLevels[] {
> +      D3D_FEATURE_LEVEL_11_1,
> +      D3D_FEATURE_LEVEL_11_0,
> +      D3D_FEATURE_LEVEL_10_1,
> +      D3D_FEATURE_LEVEL_10_0

I think we can just delcare D3D_FEATURE_LEVEL_11_1 and D3D_FEATURE_LEVEL_11_0 for D3D11 devices.
Attachment #8909541 - Flags: review?(dmu) → review+
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e98f894a8a3
Submit VR frames with a separate ID3DDeviceContextState r=daoshengmu
After pushing some experiments to the try servers with extra logging, I found that the failure on Windows 7 is due to lack of support for DirectX 11.1 on the aws instances.  Support for DirectX 11.1 can be added by installing the optional "Platform Update for Windows 7":

https://www.microsoft.com/en-ca/download/details.aspx?id=36805

Specifically, VRDisplayHost::CreateD3DObjects() is failing at getting an ID3D11Device1:

> if (FAILED(device->QueryInterface(__uuidof(ID3D11Device1), getter_AddRefs(mDevice)))) {

Note that ID3D11Device1 is needed in order to use ID3DDeviceContextState for the simpler fix, and not needed any more once we can land the refactoring that has been moved to Bug 1400407 and Bug 1400457.
Updated dom/vr/test/reftests/reftest.list to skip Windows 7.

Pushed to try to verify that the tests are running on the correct platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa7b9fa17b221e92ca937b80cee8228a5c357d2f
(In reply to :kip (Kearwood Gilbert) from comment #58)
> Updated dom/vr/test/reftests/reftest.list to skip Windows 7.
> 
> Pushed to try to verify that the tests are running on the correct platforms:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fa7b9fa17b221e92ca937b80cee8228a5c357d2f

I can see in the try push that the tests are running (and passing) on Windows 10 and now skipped on Windows 7.
Please review the change to dom/vr/test/reftest/reftest.list, which has changed since your r+.

It should simply be skipping the tests on Windows 7, as they run on machines that do not support DirectX 11.1.
Flags: needinfo?(dmu)
Looks good to me.
Flags: needinfo?(dmu)
(In reply to :kip (Kearwood Gilbert) from comment #62)
> Created attachment 8912355 [details] [diff] [review]
> Bug 1381085 (Beta Uplift) - Submit VR frames with a separate
> ID3DDeviceContextState
> 
> Rebased onto Beta for uplift.

Once this has been landed in Nightly for a bit and sticks, I will request a beta uplift for FF57.
NI? myself as a reminder to submit Beta uplift to FF57..
Flags: needinfo?(kgilbert)
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef26244d8a05
Submit VR frames with a separate ID3DDeviceContextState r=daoshengmu
https://hg.mozilla.org/mozilla-central/rev/ef26244d8a05
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
No longer blocks: 1365336
Comment on attachment 8912355 [details] [diff] [review]
Bug 1381085 (Beta Uplift) - Submit VR frames with a separate ID3DDeviceContextState

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1375743 - Enable Advanced Layers for Windows 8+ Nightly
[User impact if declined]:
When starting WebVR, the entire Firefox window will fail to redraw correctly, being filled with solid colors.
[Is this code covered by automated tests?]:
Yes, automated Reftests and Mochitests will hit the code paths affected.
[Has the fix been verified in Nightly?]:
Yes, the fix has landed in Nightly.  The problem no longer persists.
[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, STR:
- With an Oculus Rift CV1 installed, visit a WebVR page, such as https://webvr.info/samples/04-simple-mirroring.html
- Click the "enter vr" button.
- Resize the Firefox window.
- Expected results: The Firefox window will resize and repaint itself normally
- Failure results: The Firefox window will become corrupted and the contents obscured by solid filled rectangles.
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
Low risk.
[Why is the change risky/not risky?]:
Change is self-contained, affecting code paths hit only by WebVR sites when VR hardware is attached.
[String changes made/needed]:
N/A
Flags: needinfo?(kgilbert)
Attachment #8912355 - Flags: approval-mozilla-beta?
Comment on attachment 8912355 [details] [diff] [review]
Bug 1381085 (Beta Uplift) - Submit VR frames with a separate ID3DDeviceContextState

Improve a new cool feature, taking it.
Should be in 57b4
Attachment #8912355 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No longer blocks: 1314182
Flags: needinfo?(jgilbert)
Flags: qe-verify+
QA Contact: cristian.comorasu
I tried to reproduce this issue, but when I enter webVR the preview on the screen is gray ( https://imgur.com/a/BSRcG ). Am I missing something?
Flags: needinfo?(kgilbert)
(In reply to Cristian Comorasu [:CristiComo] from comment #70)
> I tried to reproduce this issue, but when I enter webVR the preview on the
> screen is gray ( https://imgur.com/a/BSRcG ). Am I missing something?

That particular example is expected to show gray by design.

Here is one that you can expect to mirror on the 2d display:

https://webvr.info/samples/04-simple-mirroring.html
Flags: needinfo?(kgilbert)
Sorry for the long pause.
I verified this issue using Oculus rift on Fx 59.0b9 and Fx 60.0a1(build ID: 20180213100127). 
I can confirm this issue is no longer reproducible.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: