1400457 - Isolate VR Rendering from Compositor

Status: RESOLVED FIXED

(Core :: WebVR, enhancement, P1)

Description

[:kip (Kearwood Gilbert)]

These patches were originally applied as a fix for Bug 1381085, which will be landed with a simpler solution for FF57 using ID3DDeviceContextState.

The solution based on ID3DDeviceContextState will not work when VRManager is moved to its own process in Bug 1362578.  Additionally, many of the other changes in the patch will be required to prepare for the VR process and multithreaded VR.

The patches to isolate the VR rendering from the Compositor have been moved here to land in FF58.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1400457 - Part 2: Create new D3D11Device for WebVR

- As we are creating a separate VR process in Bug 1362578,
  we will need our own D3D11Device.
- Eventually, we will be allowing the VR device runtimes to
  help select the appropriate adapter / device based on the
  physical connection.  This function will be updated at that
  time.

Review commit: https://reviewboard.mozilla.org/r/180526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180526/

Comment 2

[:kip (Kearwood Gilbert)]

Attachment: Bug 1400457 - Isolate VR Rendering from Compositor

- Move VR to its own D3D11Device
- WebVR is no longer dependent on PTexture, TextureParent,
  TextureHost, and TextureChild.  It continues to use TextureClient
  for pooling and coordinating locks with other Gecko code.
- As VR is now rendered in its own context, it no longer causes
  conflicts with Advanced Layers, which were due to VR frame submission
  updating D3D11 state without updating the mirrored state in MLGDevice.
- PreserveDrawingBuffer now behaving correctly for 2d display mirroring

Review commit: https://reviewboard.mozilla.org/r/180528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180528/

Comment 3

[:kip (Kearwood Gilbert)]

This also is correcting a crash when visiting one of the WebVR.info sample pages:

https://webvr.info/samples/09-splash-screen.html

Comment 4

[:kip (Kearwood Gilbert)]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180528/diff/1-2/

Comment 5

[:kip (Kearwood Gilbert)]

Comment on attachment 8908912 [details]
Bug 1400457 - Part 2: Create new D3D11Device for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180526/diff/1-2/

Comment 6

[:kip (Kearwood Gilbert)]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180528/diff/2-3/

Comment 7

[:kip (Kearwood Gilbert)]

Comment on attachment 8908912 [details]
Bug 1400457 - Part 2: Create new D3D11Device for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180526/diff/2-3/

Comment 8

[:kip (Kearwood Gilbert)]

Comment on attachment 8908912 [details]
Bug 1400457 - Part 2: Create new D3D11Device for WebVR

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180526/diff/3-4/

Comment 9

[:kip (Kearwood Gilbert)]

Moving the part 2 patch to a separate bug, Bug 1404534.

Comment 10

[:kip (Kearwood Gilbert)]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180528/diff/3-4/

Comment 11

[:kip (Kearwood Gilbert)]

Could @jgilbert please look at the dom/* changes in the patch and @daoshengmu look at the gfx/vr/* changes?

I look forward to your review, thanks!

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

https://reviewboard.mozilla.org/r/180528/#review190306

::: gfx/vr/VRDisplayHost.cpp:285
(Diff revision 4)
> -  if (autoLock.Failed()) {
> -    NS_WARNING("Failed to lock the VR layer texture");
> -    return;
> +        return;
> -  }
> +      }
> -
> -  CompositableTextureSourceRef source;
> +      const SurfaceDescriptorD3D10& surf = aTexture.get_SurfaceDescriptorD3D10();
> +      RefPtr<ID3D11Texture2D> dxTexture = nullptr;

Initializing a refptr to null is redundant.

::: gfx/vr/VRDisplayHost.cpp:298
(Diff revision 4)
>  
> -  IntSize texSize = source->GetSize();
> -
> -  TextureSourceD3D11* sourceD3D11 = source->AsSourceD3D11();
> -  if (!sourceD3D11) {
> -    NS_WARNING("VRDisplayHost::SubmitFrame failed to get a TextureSourceD3D11");
> +      // Similar to LockD3DTexture in TextureD3D11.cpp
> +      RefPtr<IDXGIKeyedMutex> mutex;
> +      dxTexture->QueryInterface((IDXGIKeyedMutex**)getter_AddRefs(mutex));
> +      if (mutex) {
> +        HRESULT hr = mutex->AcquireSync(0, 10000);

It's unlikely you actually want to wait for 10s here. (You'll probably hit a TDR first between 2 and 4s)

::: gfx/vr/VRDisplayHost.cpp:312
(Diff revision 4)
> -    return;
> +          return;
> -  }
> +        }
> -
> -  if (!SubmitFrame(sourceD3D11, texSize, aLeftEyeRect, aRightEyeRect)) {
> +      }
> +      bool success = true;
> +      if (!SubmitFrame(dxTexture, surf.size(), aLeftEyeRect, aRightEyeRect)) {
> +        success = false;

Why not:
bool success = SubmitFrame...

::: gfx/vr/VRDisplayHost.cpp:327
(Diff revision 4)
> -  }
> +      }
> -
> +      break;
> +    }
>  #elif defined(XP_MACOSX)
> -
> -  TextureHost* th = TextureHost::AsTextureHost(aTexture);
> +    case SurfaceDescriptor::TSurfaceDescriptorMacIOSurface: {
> +      const SurfaceDescriptorMacIOSurface& desc = aTexture.get_SurfaceDescriptorMacIOSurface();

Really just const auto&, especially for long types like this.

::: gfx/vr/gfxVROculus.cpp:1111
(Diff revision 4)
>    mContext->VSSetShader(mQuadVS, nullptr, 0);
>    mContext->PSSetShader(mQuadPS, nullptr, 0);
> -  ID3D11ShaderResourceView* srView = aSource->GetShaderResourceView();
> -  if (!srView) {
> -    NS_WARNING("Failed to get SRV for Oculus");
> -    return false;
> +
> +  RefPtr<ID3D11ShaderResourceView> srView;
> +  HRESULT hr = mDevice->CreateShaderResourceView(aSource, nullptr, getter_AddRefs(srView));
> +  if (FAILED(hr) || !srView) {

You shouldn't need to check both.
If you do need to check both, leave a comment as to why.

::: gfx/vr/ipc/VRLayerChild.cpp:73
(Diff revision 4)
>      return;
>    }
> +  VRManagerChild* vrmc = VRManagerChild::Get();
> +  mThisFrameTexture->SyncWithObject(vrmc->GetSyncObject());
> +  if (!gfxPlatform::GetPlatform()->DidRenderingDeviceReset()) {
> +    if (vrmc->GetSyncObject() &&

Why isn't `vrmc->GetSyncObject()` assigned to a temporary or alias?

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

https://reviewboard.mozilla.org/r/180528/#review190376

LGTM. thanks

::: gfx/vr/VRDisplayHost.cpp:289
(Diff revision 4)
> -
> -  CompositableTextureSourceRef source;
> -  if (!th->BindTextureSource(source)) {
> -    NS_WARNING("The TextureHost was successfully locked but can't provide a TextureSource");
> +      const SurfaceDescriptorD3D10& surf = aTexture.get_SurfaceDescriptorD3D10();
> +      RefPtr<ID3D11Texture2D> dxTexture = nullptr;
> +      HRESULT hr = mDevice->OpenSharedResource((HANDLE)surf.handle(),
> +        __uuidof(ID3D11Texture2D),
> +        (void**)(ID3D11Texture2D**)getter_AddRefs(dxTexture));
> +      if (FAILED(hr)) {

Please add a condition for validating dxTexture is available.

Comment 14

[:kip (Kearwood Gilbert)]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

https://reviewboard.mozilla.org/r/180528/#review190306

> Initializing a refptr to null is redundant.

Fixed, thanks for spotting it.

Comment 15

[:kip (Kearwood Gilbert)]

Comment on attachment 8908913 [details]
Bug 1400457 - Isolate VR Rendering from Compositor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180528/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c13f5102b030
Isolate VR Rendering from Compositor r=daoshengmu,jgilbert

Comment 17

[Kelsey Gilbert [:jgilbert]]

(In reply to :kip (Kearwood Gilbert) from comment #11)
> Could @jgilbert please look at the dom/* changes in the patch and
> @daoshengmu look at the gfx/vr/* changes?
> 
> I look forward to your review, thanks!

You don't need to ni? me if you r? me, btw!

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c13f5102b030