Matt Woodrow (:mattwoodrow) Assignee
	Description • 8 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.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 6 • 8 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.

	Kelsey Gilbert [:jgilbert]
	Comment 7 • 8 years ago

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?

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 8 • 8 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

	Nicolas Silva [:nical]
	Comment 9 • 8 years ago

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.

	Nicolas Silva [:nical]
	Comment 10 • 8 years ago

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

	Bas Schouten (:bas.schouten)
	Comment 11 • 8 years ago

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?

	Pulsebot
	Comment 12 • 8 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

	Pulsebot
	Comment 13 • 8 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce69082c2fb
Fix non-Windows build bustage

	Iris Hsiao [:ihsiao]
	Comment 14 • 8 years ago

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

	Pulsebot
	Comment 15 • 8 years ago

Backout by ihsiao@mozilla.com:
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

	Pulsebot
	Comment 17 • 8 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

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

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

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 19 • 8 years ago

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

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 20 • 8 years ago

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

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 21 • 8 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.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 22 • 8 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.