Remove D3D11 ImageBridge device

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Depends on 1 bug)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

3 years ago
As per bug 1284672, we want to use as few D3D11 devices as possible to avoid driver bugs.

D3D11 devices should already support multithreading fine, we just can't use the immediate context on multiple threads concurrently.

If we remove any code that tries to use immediate context on the image bridge device, then we can instead just use the content device and get rid of the extra device.
Assignee

Comment 6

3 years ago
As a follow-up it would be nice to create wrappers around ID3D11Device and ID3D11DeviceContext that assert if anyone tries to access or use the context on any thread other than the one it is 'assigned' (main thread for content, compositor thread for compositor) to. I'm not signing up for that right now though.
Comment on attachment 8774965 [details] [diff] [review]
Part 1: Make SharedSurfaceANGLE code use the content D3D11 device since we should only ever run on the main thread and would have used this device already.

Review of attachment 8774965 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/SharedSurfaceANGLE.cpp
@@ +177,5 @@
>      explicit ScopedLockTexture(ID3D11Texture2D* texture, bool* succeeded)
>        : mIsLocked(false)
>        , mTexture(texture)
>      {
> +        MOZ_ASSERT(NS_IsMainThread(), "Must be on the main thread to use d3d11 immediate context");

What's the plan for webgl-in-workers?
Attachment #8774965 - Flags: review?(jgilbert) → review+
Assignee

Comment 8

3 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #7)
 
> What's the plan for webgl-in-workers?

One option is to create a new device for this and just accept that it puts us at a higher risk of crashing.

Another is to follow the instructions at [1] and grab the direct2d lock when we're running webgl code so that we know nothing can race.


[1] https://msdn.microsoft.com/en-us/library/windows/desktop/jj569217(v=vs.85).aspx
Comment on attachment 8774967 [details] [diff] [review]
Part 3: Add a new constructor for D3D11TextureData that does threadsafe uploading.

Review of attachment 8774967 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +389,5 @@
> +  if (aSurface) {
> +    srcSurf = aSurface->GetDataSurface();
> +
> +    if (!srcSurf) {
> +      gfxCriticalError() << "Failed to GetDataSurface in UpdateFromSurface (D3D11).";

Perhaps Create should take a DataSourceSurface directly if it only handles this type.
Comment on attachment 8774968 [details] [diff] [review]
Part 4: Make SourceSurfaceImage::GetTextureClient use the threadsafe upload with D3D11 so that we no longer rely on having a separate device.

Review of attachment 8774968 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +650,5 @@
>  
>  bool
>  D3D11TextureData::UpdateFromSurface(gfx::SourceSurface* aSurface)
>  {
> +  MOZ_ASSERT(false, "UpdateNotSurface not supported for D3D11! Use CreateFromSurface instead");

nit UpdateFromSurface

Also, please document why we can't support updating a D3D11 texture here (or if we can why we are not doing it).
Attachment #8774968 - Flags: review?(nical.bugzilla) → review+
Attachment #8774969 - Flags: review?(dvander) → review+
Attachment #8774966 - Flags: review?(bas) → review+
Comment on attachment 8774967 [details] [diff] [review]
Part 3: Add a new constructor for D3D11TextureData that does threadsafe uploading.

Review of attachment 8774967 [details] [diff] [review]:
-----------------------------------------------------------------

If we're going to do this ::UpdateFromSurface should probably get an NS_IsMainThread() assert, I can already imagine this going wrong otherwise.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +421,5 @@
> +  // If we created the texture with a keyed mutex, then we expect all operations
> +  // on it to be synchronized using it. If we did an initial upload using aSurface
> +  // then bizarely this isn't covered, so we insert a manual lock/unlock pair
> +  // to force this.
> +  if (aSurface && newDesc.MiscFlags == D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX) {

Any reason not to use AutoLockTexture here?
Attachment #8774967 - Flags: review?(bas) → review+

Comment 12

3 years ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69e260d5dc7
Part 1: Make SharedSurfaceANGLE code use the content D3D11 device since we should only ever run on the main thread and would have used this device already. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d303f553f817
Part 2: Stop using the D3D11 immediate context to upload texture data in IMFYCbCrImage and stick to threadsafe APIs so that we can use the content device. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/d55d65c8fb73
Part 3: Add a new constructor for D3D11TextureData that does threadsafe uploading. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e93b1e3adf0
Part 4: Make SourceSurfaceImage::GetTextureClient use the threadsafe upload with D3D11 so that we no longer rely on having a separate device. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4ae62cc21a
Part 5: Delete the D3D11 image bridge device since it no longer has any callers. r=dvander
Sorry had to back out this for the Assertion failures on DeviceManagerD3D11.cpp, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=33067365&repo=mozilla-inbound#L91289
Flags: needinfo?(matt.woodrow)
Assignee

Comment 16

3 years ago
Flags: needinfo?(matt.woodrow)
Attachment #8777176 - Flags: review?(dvander)
Attachment #8777176 - Flags: review?(dvander) → review+

Comment 17

3 years ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74830f10d2d4
Part 1: Make SharedSurfaceANGLE code use the content D3D11 device since we should only ever run on the main thread and would have used this device already. r=jgilbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8d8ed98be4
Part 2: Stop using the D3D11 immediate context to upload texture data in IMFYCbCrImage and stick to threadsafe APIs so that we can use the content device. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b190ac7145
Part 3: Add a new constructor for D3D11TextureData that does threadsafe uploading. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/f21a0200838c
Part 4: Make SourceSurfaceImage::GetTextureClient use the threadsafe upload with D3D11 so that we no longer rely on having a separate device. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f2351e0b61f
Part 5: Delete the D3D11 image bridge device since it no longer has any callers. r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/02bab8358e59
Part 6: Allow access to the D3D11 content device from other threads. r=dvander
good news, this change showed an improvement on tp5o private bytes for windows 7:
https://treeherder.mozilla.org/perf.html#/alerts?id=2234
As of this bug, we don't call DoesAlphaTextureSharingWork anymore, probably other things as well.  Is this OK?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(dvander)
Assignee

Comment 21

3 years ago
(In reply to Milan Sreckovic [:milan] from comment #20)
> As of this bug, we don't call DoesAlphaTextureSharingWork anymore, probably
> other things as well.  Is this OK?

Probably not. I will fix it. Leaving ni? so that I don't forget.
Flags: needinfo?(dvander)
Assignee

Updated

3 years ago
Depends on: 1305326
Assignee

Comment 22

3 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> (In reply to Milan Sreckovic [:milan] from comment #20)
> > As of this bug, we don't call DoesAlphaTextureSharingWork anymore, probably
> > other things as well.  Is this OK?
> 
> Probably not. I will fix it. Leaving ni? so that I don't forget.

I filed bug 1305326 for this.
Flags: needinfo?(matt.woodrow)
Depends on: 1305490
Depends on: 1313883
No longer depends on: 1313883
You need to log in before you can comment on or make changes to this bug.