Isolate VR Rendering from Compositor

RESOLVED FIXED in Firefox 58

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kip, Assigned: kip)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
status-firefox57: --- → wontfix
(Assignee)

Updated

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

Updated

2 years ago
Blocks: 1362578
(Assignee)

Updated

2 years ago
Blocks: 1388293
(Assignee)

Comment 3

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

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

Updated

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

Updated

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

Updated

2 years ago
Blocks: 1404534
(Assignee)

Comment 9

2 years ago
Moving the part 2 patch to a separate bug, Bug 1404534.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8908912 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8908913 - Flags: review?(jgilbert)
Attachment #8908913 - Flags: review?(dmu)
(Assignee)

Comment 11

2 years ago
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!
Flags: needinfo?(jgilbert)
Flags: needinfo?(dmu)

Comment 12

2 years ago
mozreview-review
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?
Attachment #8908913 - Flags: review?(jgilbert) → review+

Comment 13

2 years ago
mozreview-review
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.
Attachment #8908913 - Flags: review?(dmu) → review+
Flags: needinfo?(dmu)
(Assignee)

Comment 14

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 16

2 years ago
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c13f5102b030
Isolate VR Rendering from Compositor r=daoshengmu,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!
Flags: needinfo?(jgilbert)
https://hg.mozilla.org/mozilla-central/rev/c13f5102b030
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.