Closed
Bug 1289640
Opened 8 years ago
Closed 8 years ago
Remove D3D11 ImageBridge device
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
(Depends on 1 open bug)
Details
Attachments
(6 files)
2.30 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
3.94 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
10.31 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
9.57 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
4.39 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
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 1•8 years ago
|
||
Attachment #8774965 -
Flags: review?(jgilbert)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8774966 -
Flags: review?(bas)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8774967 -
Flags: review?(bas)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8774968 -
Flags: review?(nical.bugzilla)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8774969 -
Flags: review?(dvander)
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.
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?
Attachment #8774965 -
Flags: review?(jgilbert) → review+
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
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.
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).
Attachment #8774968 -
Flags: review?(nical.bugzilla) → review+
Attachment #8774969 -
Flags: review?(dvander) → review+
Updated•8 years ago
|
Attachment #8774966 -
Flags: review?(bas) → review+
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?
Attachment #8774967 -
Flags: review?(bas) → review+
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
Comment 13•8 years ago
|
||
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce69082c2fb Fix non-Windows build bustage
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
Flags: needinfo?(matt.woodrow)
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
Assignee | ||
Comment 16•8 years ago
|
||
Flags: needinfo?(matt.woodrow)
Attachment #8777176 -
Flags: review?(dvander)
Attachment #8777176 -
Flags: review?(dvander) → review+
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
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
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•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.
Flags: needinfo?(dvander)
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.
Flags: needinfo?(matt.woodrow)
You need to log in
before you can comment on or make changes to this bug.
Description
•