1289640 - Remove D3D11 ImageBridge device

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

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.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: 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.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: 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.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Add a new constructor for D3D11TextureData that does threadsafe uploading.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Make SourceSurfaceImage::GetTextureClient use the threadsafe upload with D3D11 so that we no longer rely on having a separate device.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 5: Delete the D3D11 image bridge device since it no longer has any callers.

Comment 6

[Matt Woodrow (:mattwoodrow)]

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 7

[Kelsey Gilbert [:jgilbert]]

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?

Comment 8

[Matt Woodrow (:mattwoodrow)]

(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 9

[Nicolas Silva [:nical]]

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 10

[Nicolas Silva [:nical]]

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).

Comment 11

[Bas Schouten (:bas.schouten)]

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?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce69082c2fb
Fix non-Windows build bustage

Comment 14

[Iris Hsiao [:ihsiao]]

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

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a82cca925ee
Backed out changeset e69e260d5dc7 for Assertion failure on DeviceManagerD3D11.cpp
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e73d131111f
Backed out changeset 5ce69082c2fb 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2c39a54699
Backed out changeset 0d4ae62cc21a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2beb50801fc3
Backed out changeset 2e93b1e3adf0 
https://hg.mozilla.org/integration/mozilla-inbound/rev/873c836f7f35
Backed out changeset d55d65c8fb73 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5661153e694f
Backed out changeset d303f553f817

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 6: Allow access to the content device from other threads

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74830f10d2d4
https://hg.mozilla.org/mozilla-central/rev/2a8d8ed98be4
https://hg.mozilla.org/mozilla-central/rev/d3b190ac7145
https://hg.mozilla.org/mozilla-central/rev/f21a0200838c
https://hg.mozilla.org/mozilla-central/rev/3f2351e0b61f
https://hg.mozilla.org/mozilla-central/rev/02bab8358e59

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

good news, this change showed an improvement on tp5o private bytes for windows 7:
https://treeherder.mozilla.org/perf.html#/alerts?id=2234

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

As of this bug, we don't call DoesAlphaTextureSharingWork anymore, probably other things as well.  Is this OK?

Comment 21

[Matt Woodrow (:mattwoodrow)]

(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.

Comment 22

[Matt Woodrow (:mattwoodrow)]

(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.