Convert 11tex to 12resource, give it to dawn and dcomp [chromium/src : main]
Rafael Cintron (Gerrit)
Rafael Cintron added 11 comments![]( "Open in Gerrit")
d3d11on12_device->ReleaseWrappedResources(nullptr, 0);Since you're passing nullptr to ReleaseWrappedResources, this call is a no-op.
ID3D11DeviceContext* device_context;Use a `ComPtr` for `device_context`.
Microsoft::WRL::ComPtr<ID3D11Texture2D> d3d11_texture) {`ID3D11Texture2D` inherits from `ID3D11DeviceChild`. `ID3D11DeviceChild` has a `GetDevice` method you can use to get the D3D11 device which created it.
d3d11on12_device2->ReturnUnderlyingResource(d3d11_texture.Get(), 1,
&fence_value, &fence_ptr);The D3D12 command queue Dawn uses is the same as Chromium uses, correct? If so, there shouldn't be a need to pass fences here. You might be able to use ReleaseWrappedResource to accomplish the same thing and save yourself from having to pass fences.
d3d12_resource_ =
UnwrapUnderlyingResource(context_state_.get(), d3d11_texture_.Get());This is very counterintuitive. If someone wants to `BeginAccessD3D11`, it means they want to use D3D11, not D3D12. Hence, we shouldn't unwrap to D3D12.
Since `BeginAccessD3D11` is paired with `EndAccessD3D11`, it means we call methods to wait on D3D11 objects while things are unwrapped to D3D12, further comingling things in surprising ways.
As part of your work, we should go back to the callers of `BeginAccessD3D11` and, if they need D3D11 for now, prepare the D3D11 texture and have a plan to move them to D3D12. For callers that support D3D12, we should add `BeginAccessD3D12` and call `UnwrapUnderlyingResource`
? static_cast<IUnknown*>(d3d12_resource_.Get())When you call `ReturnUnderlyingResource`, the next time you call `UnwrapUnderlyingResource`, you might get back a different D3D12 resource due to potential resource rename operations. Hence, we need to clear out the DComp texture on an unwrap operation ... or detect whether we get the same resource back as an optimization down the road.
Microsoft::WRL::ComPtr<ID3D12Device> d3d12_device;(No longer relevant given my earlier comment about passing nullptr to `CreateFromD3D12Fence`)
`ID3D12Fence` inherits from `ID3D12DeviceChild`. `ID3D12DeviceChild` has a `GetDevice` method you can call to get the D3D12 device.
d3d12_device, std::move(d3d12_fence), fence_value);Since the signaling device is the DComp device, Chromium's device, you need to pass nullptr for the first parameter of `CreateFromD3D12Fence` like we do on in the `CreateFromD3D11Fence` call below.
dcomp_texture_->Release();```suggestion
dcomp_texture_->Reset();
```
Use Reset so that the ptr_ member variable is cleared.
d3d12_resource_->Release();```suggestion
d3d12_resource_->Reset();
```
Use "Reset" so that the underlying `ptr_` inside of the ComPtr is set to nullptr.
::CloseHandle(fence_event);This appears to be test code but in the future, use `base::win::ScopedHandle` to prevent memory leaks.
Related details
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 19 comments![]( "Open in Gerrit")
d3d12_device.As(&d3d11_device);Sahir VellaniPlease "CHECK_EQ(..., S_OK)" the return value of `As`.
Done
HANDLE d3d12_event_handle_;Sahir VellaniI know that `d3d12_event_handle_` is temporary and will not be checked in. For future reference, however, please use `base::win::ScopedHandle` to ensure handles do not get leaked.
Acknowledged
d3d11on12_device->ReleaseWrappedResources(nullptr, 0);Since you're passing nullptr to ReleaseWrappedResources, this call is a no-op.
Acknowledged
Use a `ComPtr` for `device_context`.
Done
}Sahir VellaniPlease either early return from GetD3D11Device or use "else if"
Done
Microsoft::WRL::ComPtr<ID3D11Texture2D> d3d11_texture) {`ID3D11Texture2D` inherits from `ID3D11DeviceChild`. `ID3D11DeviceChild` has a `GetDevice` method you can use to get the D3D11 device which created it.
Done
IUnknown* comp_texture_resource,Sahir Vellani```suggestion
IUnknown* d3d_resource,
```
Done
d3d11on12_device2->ReturnUnderlyingResource(d3d11_texture.Get(), 1,
&fence_value, &fence_ptr);The D3D12 command queue Dawn uses is the same as Chromium uses, correct? If so, there shouldn't be a need to pass fences here. You might be able to use ReleaseWrappedResource to accomplish the same thing and save yourself from having to pass fences.
Acknowledged
GetD3D11Device(context_state->dawn_context_provider()->GetDevice(),Sahir VellaniWhy can this call context_state->GetD3D11Device()?
Acknowledged
if (d3d12_resource_ && base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kDCompD3D12)) {Sahir VellaniInstead of checking flags, can we do the work in this if statement based on whether d3d12_resource is nullptr or not?
Done
d3d12_resource_ =
UnwrapUnderlyingResource(context_state_.get(), d3d11_texture_.Get());This is very counterintuitive. If someone wants to `BeginAccessD3D11`, it means they want to use D3D11, not D3D12. Hence, we shouldn't unwrap to D3D12.
Since `BeginAccessD3D11` is paired with `EndAccessD3D11`, it means we call methods to wait on D3D11 objects while things are unwrapped to D3D12, further comingling things in surprising ways.
As part of your work, we should go back to the callers of `BeginAccessD3D11` and, if they need D3D11 for now, prepare the D3D11 texture and have a plan to move them to D3D12. For callers that support D3D12, we should add `BeginAccessD3D12` and call `UnwrapUnderlyingResource`
Done
When you call `ReturnUnderlyingResource`, the next time you call `UnwrapUnderlyingResource`, you might get back a different D3D12 resource due to potential resource rename operations. Hence, we need to clear out the DComp texture on an unwrap operation ... or detect whether we get the same resource back as an optimization down the road.
Done
(No longer relevant given my earlier comment about passing nullptr to `CreateFromD3D12Fence`)
`ID3D12Fence` inherits from `ID3D12DeviceChild`. `ID3D12DeviceChild` has a `GetDevice` method you can call to get the D3D12 device.
Acknowledged
Since the signaling device is the DComp device, Chromium's device, you need to pass nullptr for the first parameter of `CreateFromD3D12Fence` like we do on in the `CreateFromD3D11Fence` call below.
Acknowledged
```suggestion
dcomp_texture_->Reset();
```Use Reset so that the ptr_ member variable is cleared.
Done
```suggestion
d3d12_resource_->Reset();
```
Use "Reset" so that the underlying `ptr_` inside of the ComPtr is set to nullptr.
Done
ID3D11Texture2D* d3d11_texture) {Sahir VellaniID3D11Texture2D objects have a GetDevice method they inherit from their base class. You should be able to QI that for the 11on12 device to then call UnwrapUnderlyingResource.
Acknowledged
This appears to be test code but in the future, use `base::win::ScopedHandle` to prevent memory leaks.
Done
if (SUCCEEDED(d3d11_device.As(&d3d11on12_device))) {Sahir VellaniSince we need ID3D11on12Device2, here is a good place to QI for that interface and return false if the QI fails.
Same goes for `D3DImageBackingFactory::IsD3DSharedImageSupported`.
Done
Related details
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 14 comments![]( "Open in Gerrit")
Need to continue reviewing the L but here is what I have so far.
"LayerTreeHostUIResourceSoftware"});The improvements to the existing labels should be upstreamed as a separate CL.
switches::kDCompD3D12)) {To aid in running experiments, please make this be its own feature flag or add it as a parameter of the existing SkiaGraphite feature flag.
dawn::native::d3d12::GetD3D11On12Device(device_.Get());ID3D11On12Device1 does not exist on all versions of Windows that Chromium supports. We need to limit our support of the feature to only those where the QI for ID3D11On12Device1 returns S_OK. For all others, we need to gracefully fallback to single swap chain like we have today.
#include "base/containers/span.h"Unless I am missing something, we shouldn't need this include of span in `d3d_image_backing.h`
hr = d3d12_device->CreateCommandQueue(&command_queue_desc,Creating a command queue every time we call PrepareTextureForD3D12 will be costly. Please cache the queue in the context.
texture_d3d11_device_ = GetD3D11Device(The device to use should come from the passed in texture not from a global place.
d3d12_event_handle_ = CreateEventHandle();I opened a previous issue about needing to get rid of this event handle. Please do so.
If you need it for prototyping purposes while you make progress, please manage its lifetime a bit better. I see multiple places where the handle is unconditionally assigned to and never cleaned up, thus leaking the handle and bloating the process handle table.
hr = d3d12_fence->SetEventOnCompletion(fence_value, d3d12_event_handle_);
CHECK_EQ(hr, S_OK);
::WaitForSingleObject(d3d12_event_handle_, 10000);Can you please see what it would take to remove this hack? We should be able to go down the same path for fences that we do for D3D11.
dcomp_texture_.Reset();Creating and destroying DComp textures repeatedly is a known source of performance regressions. Please find a way to only release/re-create it when the underlying DirectX resource actually needs to be changed.
if (!d3d_image_backing->BeginAccessD3D(d3d11_device_,Why does D3D12VideoImageRepresentation need to exist? Is this for the case where we use D3D12 to do video? Or where Video is on a separate D3D11 device from the rest of Chromium? The answers to these questions will guide whether this class should call BeginAccessD3D11 or BeginAccessD3D12.
Microsoft::WRL::ComPtr<EXPERIMENTAL_IDCompositionDevice6> dcomp_device6;
HRESULT hr = dc_layer_tree_->dcomp_device_.As(&dcomp_device6);Please have `DCLayerTree` store the `IDCompositionDevice6` as a member variable and set it in its Initialize method if the QI succeeds. No need to check the command line switch in `DClayerTree.`
#include <dawn/native/D3D12Backend.h>Does `dcomp_presenter.cc` still need to `include dawn/native/D3D12Backend.h`?
// if (g_direct_composition_texture_supported.has_value()) {
// return g_direct_composition_texture_supported.value();
// }Placeholder issue to uncomment this code and implement `DirectCompositionTextureSupported` correctly.
Related details
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 3 comments![]( "Open in Gerrit")
SkAlphaType alpha_type_;```suggestion
const SkAlphaType alpha_type_;
```
bool want_dcomp_texture_;```suggestion
const bool want_dcomp_texture_;
```
context_state_(std::move(context_state)),What would it take to avoid passing context state to `D3DImageBacking` and, instead, determine what to do based on whether the device of `d3d11_texture` is an 11on12 device?
Related details
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tang (Gerrit)
Michael Tang added 16 comments![]( "Open in Gerrit")
I've left some comments, as with Rafael, I will continue to review this. Apologies for any dupe comments w/ rafael.
Please split out unrelated changes (logging, etc) to make this easier to review!
if (Microsoft::WRL::ComPtr<PREVIEW_IDCompositionDevice5> dcomp_device5;
SUCCEEDED(dcomp_device.As(&dcomp_device5))) {
return OutputSurface::DCSupportLevel::kDCompDynamicTexture;
}```suggestion
if (Microsoft::WRL::ComPtr<PREVIEW_IDCompositionDevice6> dcomp_device6;
SUCCEEDED(dcomp_device.As(&dcomp_device6))) {
return OutputSurface::DCSupportLevel::kDCompTextureD3D12;
}
if (Microsoft::WRL::ComPtr<PREVIEW_IDCompositionDevice5> dcomp_device5;
SUCCEEDED(dcomp_device.As(&dcomp_device5))) {
return OutputSurface::DCSupportLevel::kDCompDynamicTexture;
}
```
When you clean up this CL, this will be useful to gate your feature on API support. This can possibly replace `kDCompD3D12`.
presenter_->SetCommandQueue(std::move(command_queue));In practice, can the command queue change frame to frame? It not, we should initialize `DCLayerTree` with the command queue instead of passing it down every frame.
// Used to track D3D12 fence completion. Only a bandaid solution to get the
// prototype working.What is the intended solution?
const gfx::ColorSpace color_space_;
SkAlphaType alpha_type_;I believe this is already available on the base type: https://source.chromium.org/chromium/chromium/src/+/main:gpu/command_buffer/service/shared_image/shared_image_backing.h;l=138-140;drc=f2e13893c80b602c7f33120e71b540c5b6d68d7c
bool want_dcomp_texture_;```suggestion
const bool want_dcomp_texture_;
```
return dcomp_texture_ || (dxgi_shared_handle_state_ &&Is this only needed for dx12 comp textures?
LOG(INFO) << "GLTextureHolder::GLTextureHolder";If these logs are useful for posterity, we can do something like to check it in.
```suggestion
DVLOG(3) << "GLTextureHolder::GLTextureHolder"; // or __func__ << " " << this;
```
context_state_(std::move(context_state)),Can we extract the D3D11 device and simply bail (e.g. `want_dcomp_texture = false`) if the backend type is not correct?
I recall being told that we want to avoid passing the `SharedContextState` around, preferring to extract the specific stuff we need from it (since it's kind of an "everything" object that adds unnessecary dependencies).
texture_d3d11_device_ = GetD3D11Device(The device to use should come from the passed in texture not from a global place.
Is this not already set a couple lines up?
if (context_state_->dawn_context_provider()->backend_type() ==Do you know why `ProduceDawn` is provided with a `backend_type` instead of reading it off the `shared_context_state_` like you are here? I wonder if `ProduceVideo` isn't supposed to know about this detail-- if not, is there another signal for us to branch on here?
dcomp_texture_.Reset();Creating and destroying DComp textures repeatedly is a known source of performance regressions. Please find a way to only release/re-create it when the underlying DirectX resource actually needs to be changed.
I believe the dcomp texture also needs to stay alive
D3D12VideoImageRepresentation::D3D12VideoImageRepresentation(This seems to be pretty similar to `D3D11VideoImageRepresentation`, should the latter just be renamed to `D3DVideoImageRepresentation`?
case wgpu::BackendType::D3D12:Will this still be needed when we don't support dcomp textures, but want d3d12?
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;if we can initialize DCompPresenter with the command queue, then we can just have this field live on DCLayerTree.
if (FAILED(d3d11_device.As(&d3d11on12_device))) {This needs to be something like "if SUCCEEDED(qi to 11on12) AND we do not support dx12 comp textures"
Related details
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 13 comments![]( "Open in Gerrit")
To aid in running experiments, please make this be its own feature flag or add it as a parameter of the existing SkiaGraphite feature flag.
Done
SkAlphaType alpha_type_;Sahir Vellani```suggestion
const SkAlphaType alpha_type_;
```
Done
bool want_dcomp_texture_;Sahir Vellani```suggestion
const bool want_dcomp_texture_;
```
Done
Unless I am missing something, we shouldn't need this include of span in `d3d_image_backing.h`
Relic of some experimentation.
hr = d3d12_device->CreateCommandQueue(&command_queue_desc,Creating a command queue every time we call PrepareTextureForD3D12 will be costly. Please cache the queue in the context.
Done
The device to use should come from the passed in texture not from a global place.
Done
I opened a previous issue about needing to get rid of this event handle. Please do so.
If you need it for prototyping purposes while you make progress, please manage its lifetime a bit better. I see multiple places where the handle is unconditionally assigned to and never cleaned up, thus leaking the handle and bloating the process handle table.
Done
hr = d3d12_fence->SetEventOnCompletion(fence_value, d3d12_event_handle_);
CHECK_EQ(hr, S_OK);
::WaitForSingleObject(d3d12_event_handle_, 10000);Can you please see what it would take to remove this hack? We should be able to go down the same path for fences that we do for D3D11.
Done
Creating and destroying DComp textures repeatedly is a known source of performance regressions. Please find a way to only release/re-create it when the underlying DirectX resource actually needs to be changed.
Done
if (!d3d_image_backing->BeginAccessD3D(d3d11_device_,Why does D3D12VideoImageRepresentation need to exist? Is this for the case where we use D3D12 to do video? Or where Video is on a separate D3D11 device from the rest of Chromium? The answers to these questions will guide whether this class should call BeginAccessD3D11 or BeginAccessD3D12.
Oversight on my part. We don't need both image representations, whether we use 11 or 12 can be an implementation detail of BeginAccessD3D. Actually, we don't really need the D3D11VideoImageRepresentation at all when the backend type is D3D12. The D3D11 texture is currently used in situations that allow for it to be unwrapped.
Microsoft::WRL::ComPtr<EXPERIMENTAL_IDCompositionDevice6> dcomp_device6;
HRESULT hr = dc_layer_tree_->dcomp_device_.As(&dcomp_device6);Please have `DCLayerTree` store the `IDCompositionDevice6` as a member variable and set it in its Initialize method if the QI succeeds. No need to check the command line switch in `DClayerTree.`
Done
Does `dcomp_presenter.cc` still need to `include dawn/native/D3D12Backend.h`?
Done
void DCompPresenter::SetCommandQueue(
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue) {
command_queue_ = std::move(command_queue);
}Sahir VellaniInstead of adhoc ways of passing objects around, let's have the presenter get the command queue at construction time so that it is available.
When gpu_init.cc calls gl::InitializeDirectComposition, it should pass it the command queue to use for presentation.
Done
Related details
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 12 comments![]( "Open in Gerrit")
// Used to track D3D12 fence completion. Only a bandaid solution to get the
// prototype working.What is the intended solution?
Add the d3d12 fence to read/write fence sets and let the currently existing logic do its thing. Would've just been harder to debug that way.
const gfx::ColorSpace color_space_;
SkAlphaType alpha_type_;I believe this is already available on the base type: https://source.chromium.org/chromium/chromium/src/+/main:gpu/command_buffer/service/shared_image/shared_image_backing.h;l=138-140;drc=f2e13893c80b602c7f33120e71b540c5b6d68d7c
I missed that, thanks.
bool want_dcomp_texture_;Sahir Vellani```suggestion
const bool want_dcomp_texture_;
```
Done
return dcomp_texture_ || (dxgi_shared_handle_state_ &&Is this only needed for dx12 comp textures?
Yes
LOG(INFO) << "GLTextureHolder::GLTextureHolder";If these logs are useful for posterity, we can do something like to check it in.
```suggestion
DVLOG(3) << "GLTextureHolder::GLTextureHolder"; // or __func__ << " " << this;
```
Good idea, acknowledged.
context_state_(std::move(context_state)),Can we extract the D3D11 device and simply bail (e.g. `want_dcomp_texture = false`) if the backend type is not correct?
I recall being told that we want to avoid passing the `SharedContextState` around, preferring to extract the specific stuff we need from it (since it's kind of an "everything" object that adds unnessecary dependencies).
Removed context_state_.
What would it take to avoid passing context state to `D3DImageBacking` and, instead, determine what to do based on whether the device of `d3d11_texture` is an 11on12 device?
Not much, apparently.
if (context_state_->dawn_context_provider()->backend_type() ==Do you know why `ProduceDawn` is provided with a `backend_type` instead of reading it off the `shared_context_state_` like you are here? I wonder if `ProduceVideo` isn't supposed to know about this detail-- if not, is there another signal for us to branch on here?
Acknowledged
D3D12VideoImageRepresentation::D3D12VideoImageRepresentation(This seems to be pretty similar to `D3D11VideoImageRepresentation`, should the latter just be renamed to `D3DVideoImageRepresentation`?
Done
case wgpu::BackendType::D3D12:Will this still be needed when we don't support dcomp textures, but want d3d12?
Yes, nice catch.
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;if we can initialize DCompPresenter with the command queue, then we can just have this field live on DCLayerTree.
Done
This needs to be something like "if SUCCEEDED(qi to 11on12) AND we do not support dx12 comp textures"
Done
Related details
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 2 comments![]( "Open in Gerrit")
"LayerTreeHostUIResourceSoftware"});Sahir VellaniThe improvements to the existing labels should be upstreamed as a separate CL.
Acknowledged
presenter_->SetCommandQueue(std::move(command_queue));In practice, can the command queue change frame to frame? It not, we should initialize `DCLayerTree` with the command queue instead of passing it down every frame.
Done
Related details
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 2 comments![]( "Open in Gerrit")
dawn::native::d3d12::GetD3D11On12Device(device_.Get());ID3D11On12Device1 does not exist on all versions of Windows that Chromium supports. We need to limit our support of the feature to only those where the QI for ID3D11On12Device1 returns S_OK. For all others, we need to gracefully fallback to single swap chain like we have today.
Done
// if (g_direct_composition_texture_supported.has_value()) {
// return g_direct_composition_texture_supported.value();
// }Placeholder issue to uncomment this code and implement `DirectCompositionTextureSupported` correctly.
Done
Rafael Cintron (Gerrit)
Rafael Cintron added 14 comments![]( "Open in Gerrit")
Have not finished with current review but posting what I have so far.
bool is_texture_unwrapped_ GUARDED_BY(lock_) = false;Please add a comment to `is_texture_unwrapped_` like the other members in this class have.
const bool want_dcomp_texture_;Please add a comment to `want_dcomp_texture_` like the other members in this class have.
LOG(ERROR) << "D3D12 command queue is null";Will the caller ever pass a nullptr command queue. If not, we should just dereference it unconditionally like we already do with `d3d11_texture`.
d3d11on12_device->ReleaseWrappedResources(&d3d11_texture, 1);Might be better to encapsulate this logcin by having the BeginAccessD3D12 and EndAccessD3D12 methods be more explicit about releasing the wrapped resource or unwrapping it based on which one is called. With your current approach, I think you're calling ReleaseWrappedResource when you don't need to.
Did you test the case where media is on a separate device than Chromium?
D3DImageBacking::GetCommandQueueFor11On12() {Please write a detailed commend why this "11on12" command queue exists.
D3DImageBacking::GetCommandQueueFor11On12() {```suggestion
D3DImageBacking::GetOrCreateCommandQueueFor11On12() {
```
if (d3d12_command_queue_) {Please disambiguate the member variable. When I first read the code, I thought this was the command queue where actual work was being being submitted. We should call it (and the accessor) something like "d3d12_none_command_queue."
Microsoft::WRL::ComPtr<EXPERIMENTAL_IDCompositionDevice6> dcomp_device_6_;
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;Please add a comment for these members like the other ones in this class have.
dcomp_device_6_ = std::move(dcomp_device6);Later in the file, we check for `dcomp_device_6_` and unconditionally dereference `command_queue_` when we call `PresentCompositionTextures`.
If we're running on a machine with `IDCompositionDevice6` but D3D12 mode is disabled or the machine has no D3D12 drivers, you'll get a crash in `BuildTree`.
We should fix this by first calling `GetDirectCompositionD3D12CommandQueue`. If it returns a non-nullptr queue, only then do we set `dcomp_device_6_`. This way both variables are either nullptr or non-nullptr.
// Retrieves the global d3d12 command queue used by direct composition.```suggestion
// Retrieves the global d3d12 command queue created by Dawn used by SharedImage code to queue work when it runs in D3D12 mode.
```
// Global d3d12 command queue used by direct composition.```suggestion
// Global d3d12 command queue created by Dawn used by SharedImage code to queue work when it runs in D3D12 mode.
```
if (d3d12_command_queue) {If check of d3d12_command_queue is unnecessary.
#include <d3d12.h>
#include <wrl/client.h>Are these header files needed in presenter.h?
Related details
- Michael Tang
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 13 comments![]( "Open in Gerrit")
bool is_texture_unwrapped_ GUARDED_BY(lock_) = false;Please add a comment to `is_texture_unwrapped_` like the other members in this class have.
Done
const bool want_dcomp_texture_;Please add a comment to `want_dcomp_texture_` like the other members in this class have.
Done
LOG(ERROR) << "D3D12 command queue is null";Will the caller ever pass a nullptr command queue. If not, we should just dereference it unconditionally like we already do with `d3d11_texture`.
Yes a null command queue can be passed in.
d3d11on12_device->ReleaseWrappedResources(&d3d11_texture, 1);Might be better to encapsulate this logcin by having the BeginAccessD3D12 and EndAccessD3D12 methods be more explicit about releasing the wrapped resource or unwrapping it based on which one is called. With your current approach, I think you're calling ReleaseWrappedResource when you don't need to.
Did you test the case where media is on a separate device than Chromium?
I think you're right that I'm releasing when I don't have to. We check whether the texture is unwrapped prior to unwrapping anyway.
D3DImageBacking::GetCommandQueueFor11On12() {Sahir Vellani```suggestion
D3DImageBacking::GetOrCreateCommandQueueFor11On12() {
```
Done
D3DImageBacking::GetCommandQueueFor11On12() {Please write a detailed commend why this "11on12" command queue exists.
Done
if (d3d12_command_queue_) {Please disambiguate the member variable. When I first read the code, I thought this was the command queue where actual work was being being submitted. We should call it (and the accessor) something like "d3d12_none_command_queue."
Done
Microsoft::WRL::ComPtr<EXPERIMENTAL_IDCompositionDevice6> dcomp_device_6_;
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;Please add a comment for these members like the other ones in this class have.
Done
dcomp_device_6_ = std::move(dcomp_device6);Later in the file, we check for `dcomp_device_6_` and unconditionally dereference `command_queue_` when we call `PresentCompositionTextures`.
If we're running on a machine with `IDCompositionDevice6` but D3D12 mode is disabled or the machine has no D3D12 drivers, you'll get a crash in `BuildTree`.
We should fix this by first calling `GetDirectCompositionD3D12CommandQueue`. If it returns a non-nullptr queue, only then do we set `dcomp_device_6_`. This way both variables are either nullptr or non-nullptr.
Done
// Retrieves the global d3d12 command queue used by direct composition.```suggestion
// Retrieves the global d3d12 command queue created by Dawn used by SharedImage code to queue work when it runs in D3D12 mode.
```
Done
// Global d3d12 command queue used by direct composition.```suggestion
// Global d3d12 command queue created by Dawn used by SharedImage code to queue work when it runs in D3D12 mode.
```
Done
if (d3d12_command_queue) {If check of d3d12_command_queue is unnecessary.
Done
#include <d3d12.h>
#include <wrl/client.h>Are these header files needed in presenter.h?
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 1 comment![]( "Open in Gerrit")
Related details
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 16 comments![]( "Open in Gerrit")
return SUCCEEDED(hr) && !(d3d11_texture_desc.MiscFlags &
D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX);Nit: Please provide a comment why keyed mutexes are excluded.
Microsoft::WRL::ComPtr<ID3D11Resource> d3d11_texture,
Microsoft::WRL::ComPtr<ID3D12CommandQueue> d3d12_command_queue) {Since PrepareTextureForD3D12 does not take ownership of its parameters, please pass these by bald pointer.
Microsoft::WRL::ComPtr<ID3D12Resource> d3d12_resource;Nit: Please declare d3d12_resource closer to where it is used.
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11On12Device2: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}Per earlier checks, this QI for ID3D11On12Device2 should always succeed.
// Flushing operations.Nit: Please replace this "flushing operations" comment with a comment that describes why we need to flush. Include a link to the MSDN page that talks about why: https://learn.microsoft.com/en-us/windows/win32/api/d3d11on12/nf-d3d11on12-id3d11on12device2-unwrapunderlyingresource
void PrepareTextureForD3D11(Since PrepareTextureForD3D11 does not take ownership of its parameters, please pass these by bald pointer.
if (!dcomp_texture_) {
dcomp_texture_ =
want_dcomp_texture_ && (d3d11_texture_ || d3d12_resource_)
? CreateDCompTexture(
d3d12_resource_
? static_cast<IUnknown*>(d3d12_resource_.Get())
: static_cast<IUnknown*>(d3d11_texture_.Get()),
alpha_type(), color_space())
: nullptr;
}Why does "ProduceDawn" need to create DComp textures?
? CreateDCompTexture(static_cast<IUnknown*>(d3d11_texture_.Get()),D3D11 textures inherit from IUnknown. Unless I am missing something, this static cast should not be necessary.
is_texture_unwrapped_If `BeginAccessD3D12` sets `is_texture_unwrapped` to true below and `EndAccessD3D12` always sets it to false, when will it be true here on line 1600?
if (d3d12_resource && d3d12_resource != d3d12_resource_) {If `EndAccessCommon` always clears out `d3d12_resource_`, why are we assuming it is non-nullptr here? Is the `EndAccessCommon` code which clears the D3D resource not necessary?
if (d3d12_resource && d3d12_resource != d3d12_resource_) {If `d3d12_resource` is nullptr, we should clear out `d3d12_resource_` and return false.
d3d12_resource_ = d3d12_resource;```suggestion
d3d12_resource_ = std::move(d3d12_resource);
```
? CreateDCompTexture(static_cast<IUnknown*>(d3d12_resource_.Get()),Similarly here, D3D12 resources derive from IUnknown so this cast shouldn't be necessary.
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(d3d11_device);Why can't we rely on existing code in GetPendingWaitFences to add signaled fences to `d3d11_signaled_fence_map_` when needed? Is there a bug in GetPendingWaitFences?
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11On12Device2: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}Per if checks which run earlier, shouldn't this QI for ID3D11on12Device2 always succeed?
CHECK_EQ(hr, S_OK) << ", CreateCommandQueue failed: "
<< logging::SystemErrorCodeToString(hr);CreateCommandQueue, on the other hand, is something we should handle gracefully and not CHECK. If the device has become removed, it will fail.
Related details
- Michael Tang
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 16 comments![]( "Open in Gerrit")
return SUCCEEDED(hr) && !(d3d11_texture_desc.MiscFlags &
D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX);Nit: Please provide a comment why keyed mutexes are excluded.
Done
Microsoft::WRL::ComPtr<ID3D11Resource> d3d11_texture,
Microsoft::WRL::ComPtr<ID3D12CommandQueue> d3d12_command_queue) {Since PrepareTextureForD3D12 does not take ownership of its parameters, please pass these by bald pointer.
Done
Microsoft::WRL::ComPtr<ID3D12Resource> d3d12_resource;Nit: Please declare d3d12_resource closer to where it is used.
Done
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11On12Device2: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}Per earlier checks, this QI for ID3D11On12Device2 should always succeed.
Done
Nit: Please replace this "flushing operations" comment with a comment that describes why we need to flush. Include a link to the MSDN page that talks about why: https://learn.microsoft.com/en-us/windows/win32/api/d3d11on12/nf-d3d11on12-id3d11on12device2-unwrapunderlyingresource
Done
Since PrepareTextureForD3D11 does not take ownership of its parameters, please pass these by bald pointer.
Done
if (!dcomp_texture_) {
dcomp_texture_ =
want_dcomp_texture_ && (d3d11_texture_ || d3d12_resource_)
? CreateDCompTexture(
d3d12_resource_
? static_cast<IUnknown*>(d3d12_resource_.Get())
: static_cast<IUnknown*>(d3d11_texture_.Get()),
alpha_type(), color_space())
: nullptr;
}Why does "ProduceDawn" need to create DComp textures?
Don't think it does. Thanks for the catch.
? CreateDCompTexture(static_cast<IUnknown*>(d3d11_texture_.Get()),D3D11 textures inherit from IUnknown. Unless I am missing something, this static cast should not be necessary.
Done
If `BeginAccessD3D12` sets `is_texture_unwrapped` to true below and `EndAccessD3D12` always sets it to false, when will it be true here on line 1600?
Done
if (d3d12_resource && d3d12_resource != d3d12_resource_) {If `d3d12_resource` is nullptr, we should clear out `d3d12_resource_` and return false.
Done
if (d3d12_resource && d3d12_resource != d3d12_resource_) {If `EndAccessCommon` always clears out `d3d12_resource_`, why are we assuming it is non-nullptr here? Is the `EndAccessCommon` code which clears the D3D resource not necessary?
Done
```suggestion
d3d12_resource_ = std::move(d3d12_resource);
```
Done
? CreateDCompTexture(static_cast<IUnknown*>(d3d12_resource_.Get()),Similarly here, D3D12 resources derive from IUnknown so this cast shouldn't be necessary.
Done
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(d3d11_device);Why can't we rely on existing code in GetPendingWaitFences to add signaled fences to `d3d11_signaled_fence_map_` when needed? Is there a bug in GetPendingWaitFences?
GetPendingWaitFences only creates a fence on the texture's device. When there is WebGL on the page, that fence's value is current because work is being done on the ANGLE device. Here, we create a fence on the device that is being used for the current D3D11 access. I think it could be a bug, but I haven't yet looked into why there are no problems in regular D3D11 mode.
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11On12Device2: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}Per if checks which run earlier, shouldn't this QI for ID3D11on12Device2 always succeed?
No, actually GetOrCreateCommandQueueFor11On12 is called regardless of the mode we're in.
CHECK_EQ(hr, S_OK) << ", CreateCommandQueue failed: "
<< logging::SystemErrorCodeToString(hr);CreateCommandQueue, on the other hand, is something we should handle gracefully and not CHECK. If the device has become removed, it will fail.
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 16 comments![]( "Open in Gerrit")
bool BeginAccessD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,
bool write_access,
bool is_overlay_access = false);
void EndAccessD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,
bool is_overlay_access = false);Will Begin/EndAccessD3D12 ever be called from outside of D3DImageBacking? If not, can we make them private to the class?
bool ShouldUseD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,```suggestion
bool ShouldUseD3D12(ID3D11Device* d3d11_device,
```
D3D11_TEXTURE2D_DESC d3d11_texture_desc) {```suggestion
const D3D11_TEXTURE2D_DESC& d3d11_texture_desc) {
```
// TODO(savella) Look into why dawn_d3d11_device == texture_d3d11_device_ if
// the resource has a keyed mutex flag. This happens during copy scenariosWhat were the results of your investigation?
// ProduceDawn can be called during D3D12 access. Therefore if the texture
// is already unwrapped, use the underlying resource to create sharedUnless I am missing something, this comment doesn't appear to match what the code is doing. The comment says "use the underlying resource" but the code sets d3d12_resource to nullptr.
// texture memory for the D3D12 resource. Since it was not already in an
// unwrapped state, return the resource back to the 11On12 translation
// layer.This code seems counterintuitive to me. Since the purpose of "ProduceDawn" is to prepare to render to the texture (with Dawn) with D3D12, we shouldn't need to give the resource back to 11on12. We just called PrepareTextureForD3D12 earlier in this same function.
return false;Please LOG(ERROR) for this false condition so we can troubleshoot bad rendering with users.
if (d3d12_resource && d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = std::move(d3d12_resource);
} else if (!d3d12_resource) {
d3d12_resource_.Reset();
return false;
}```suggestion
if (d3d12_resource) {
if (d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = std::move(d3d12_resource);
}
} else {
d3d12_resource_.Reset();
return false;
}
```
if (d3d11_signal_fence) {
if (d3d11_signal_fence->IncrementAndSignalD3D11()) {
signaled_fence.insert(d3d11_signal_fence);
} else {
LOG(ERROR) << "Failed to signal D3D11 device fence on EndAccess";
}
} else if (!is_texture_device && in_write_access_) {Since the code seems necessary, we should be able to reuse the IncrementAndSignal code instead of copy/pasting it.
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(d3d11_device);Sahir VellaniWhy can't we rely on existing code in GetPendingWaitFences to add signaled fences to `d3d11_signaled_fence_map_` when needed? Is there a bug in GetPendingWaitFences?
GetPendingWaitFences only creates a fence on the texture's device. When there is WebGL on the page, that fence's value is current because work is being done on the ANGLE device. Here, we create a fence on the device that is being used for the current D3D11 access. I think it could be a bug, but I haven't yet looked into why there are no problems in regular D3D11 mode.
Acknowledged
if (d3d11_signal_fence->IncrementAndSignalD3D11()) {Does this code (now) need the same treatment you added to EndAccessD3D11 with regard to calling IncrementAndSignal when we're in write access and the devices do not match? Why or why not?
if (is_texture_unwrapped_) {
PrepareTextureForD3D11(d3d11_texture_.Get());
is_texture_unwrapped_ = false;
}Please add a comment that describes why we're not clearing out `d3d12_resource_` after we've prepared for D3D11.
dcomp_device_6_ = std::move(dcomp_device6);Please improve the code so that we don't end up in a partially initialized state. Suggest that g_d3d12_command_queue is only set when we have IDCompositionDevice6. Then, we can CHECK_EQ(..., S_OK) the QI here.
CHECK_EQ(hr, S_OK) << "Failed to present composition textures."Similar to Commit, PresentCompositionTextures can fail if the device has been removed or we have presentation loss. We want those cases to be handled in the caller. Please add a new "CommitError::Reason" for PresentCompositionTextures.
CHECK_EQ(hr, S_OK) << "Commit failed with error 0x" << std::hex << hr;Since we do FAILED(hr) right above, hr will never be S_OK to satisfy this CHECK.
Commit can fail if the device has been removed or we have presentation loss. We want those cases to be handled in the caller.
}
if (g_d3d12_command_queue) {
g_d3d12_command_queue->Release();
g_d3d12_command_queue = nullptr;
}```suggestion
if (g_d3d12_command_queue) {
g_d3d12_command_queue->Release();
g_d3d12_command_queue = nullptr;
}
}
```
Related details
- Michael Tang
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tang (Gerrit)
Michael Tang added 22 comments![]( "Open in Gerrit")
Do you anticipate that D3D12+DComp support will be something that we need to query for? Or is it something that we want to just happen automatically when supported without our knowledge? If the former, please add a `OutputSurface::DCSupportLevel::kDCompTextureD3D12` and set it [in SkiaOutputDeviceDComp](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display_embedder/skia_output_device_dcomp.cc;l=54-68;drc=75786f4af376776d0078a6d500a7134172922e59).
// True if the backing's texture has been unwrapped for use as a D3D12
// resource. Used to prevent unwraps of already unwrapped texture.
bool is_texture_unwrapped_ GUARDED_BY(lock_) = false;Can we add a note that this is referring to `d3d11_texture_` and if `is_texture_unwrapped_ = true` means that the texture _cannot_ be used by D3D11?
Maybe theres a better name for that that is higher level than the 11on12 terminology?
const Microsoft::WRL::ComPtr<ID3D11Texture2D> d3d11_texture_;Can this be made `const` again? I don't see any places that reset it.
// Unwraps a D3D11 texture to its underlying D3D12 resource.Given the usage, it seems like `UnwrapUnderlyingResource` can return different results on the same input texture. Please add a note in this comment if that's so.
ID3D11Resource* d3d11_texture,Does this texture have to have been created on 11on12? What happens if I call this without D3D12 initialized (or from a non-11on12 texture)?
HRESULT hr = d3d11_device.As(&d3d11on12_device);We're dropping errors from `hr` here.
// UnwrapUnderlyingResource doesn't flush, and it may schedule GPU work. We
// should therefore flush the D3D11 device context here. See:
// https://learn.microsoft.com/en-us/windows/win32/api/d3d11on12/nf-d3d11on12-id3d11on12device2-unwrapunderlyingresourceIs the reason we need to flush is to ensure that any fence signals get realized _now_?
LOG(ERROR) << "Failed to get D3D11 device from D3D12 device. hr="
<< std::hex << hr;```suggestion
LOG(ERROR) << "Failed to get D3D11 device from D3D12 device: "
<< logging::SystemErrorCodeToString(hr);
```
}```suggestion
} else {
NOTREACHED();
}
```
Should we guard against unexpected backend types? Or rename this to something like `TryGetDawnDeviceAsD3D11Device`?
auto d3d12_resource =
is_texture_unwrapped_
? nullptr
: PrepareTextureForD3D12(d3d11_texture_.Get(),
GetOrCreateCommandQueueFor11On12());Is there ever a time when `d3d11_texture_` and `d3d12_resource_` are both valid to use at the same time? Could we (reasonably) replace `is_texture_unwrapped_` with a `std::optional<ID3D12Resource>` (or `std::variant`?) to ensure safety in that case?
} else {Should we guard against unexpected dawn backend types?
CHECK(dawn_d3d11_device);Not sure if intentional, but we are dropping this check. Should it be in `GetD3D11Device`?
CHECK(!is_texture_unwrapped_) << "Texture is unwrapped";Maybe something with a bit more context: "11on12 texture is unwrapped for D3D12 access"? I'm imagining what it would be like to see this crash message after forgetting about this change 😊
AutoLock auto_lock(this);
FlushGraphiteCommandsIfNeeded();
if (!ValidateBeginAccess(write_access)) {
return false;
}
// Defer clearing fences until later to handle failure to synchronize.
std::vector<scoped_refptr<gfx::D3DSharedFence>> wait_fences =
GetPendingWaitFences(d3d11_device, /*dawn_device=*/nullptr, write_access);
for (auto& wait_fence : wait_fences) {
if (!wait_fence->WaitD3D11(d3d11_device)) {
LOG(ERROR) << "Failed to wait for fence";
return false;
}
}
Can this be hoisted up into `BeginAccessD3D`?
if (!dcomp_texture_) {
CHECK(is_texture_unwrapped_);
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space())
: nullptr;
}
if (is_overlay_access && dcomp_texture_) {
CHECK(!write_access);
BeginDCompTextureAccess();
}
// Clear fences and update state iff D3D12 BeginAccess succeeds.
BeginAccessCommon(write_access);
return true;Can this be hoisted up into `BeginAccessD3D`?
// If the accessing device is not the texture's original device, createnit: wording, since the access has finished at this point?
```suggestion
// If the accessing device was not the texture's original device, create
```
if (!d3d11_signal_fence && (!is_texture_device && in_write_access_)) {Could/should we find a way to put the accessing device into `d3d11_signaled_fence_map_` on begin access instead of special casing it here? This map seems to be the general place to sychronize "after D3D11 access".
if (is_overlay_access && dcomp_texture_) {
EndDCompTextureAccess();
}
if (in_write_access_) {
NotifyGraphiteAboutInitializedStatus();
}
EndAccessCommon(signaled_fence);Can this be hoisted up into `EndAccessD3D`?
Microsoft::WRL::ComPtr<ID3D12Fence> d3d12_fence;
if (ShouldUseD3D12(texture_d3d11_device_.Get(), d3d11_texture_desc_)) {
uint64_t fence_value = 0;
HRESULT hr = dcomp_texture_->GetAvailableFence(&fence_value,
IID_PPV_ARGS(&d3d12_fence));
CHECK_EQ(hr, S_OK) << ", GetAvailableFence failed: "
<< logging::SystemErrorCodeToString(hr);
CHECK(d3d12_fence) << "Overlay access is still in use by DWM.";
// If the fence is already passed the wait value, we don't need to wait on
// it.
if (d3d12_fence->GetCompletedValue() >= fence_value) {
return;
}
dcomp_texture_available_fence_ = gfx::D3DSharedFence::CreateFromD3D12Fence(
std::move(d3d12_fence), fence_value);
return;
}
Theres a lot of duplicated code here vs the D3D11 version-- is there a way to deduplicate, so both versions to have to stay as much in sync? Maybe store off `ShouldUseD3D12` and interleave checks to it?
// An extension of composition device interface that enables support of d3d12
// for composition textures.
Microsoft::WRL::ComPtr<EXPERIMENTAL_IDCompositionDevice6> dcomp_device_6_;
// A d3d12 command queue from Dawn that is used to present composition
// textures prior to DComp commit.
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;nit: Can we group this with the other `d3d11_device_`, `dcomp_device_`, etc above?
IUnknown* command_queue_unk = dc_layer_tree_->command_queue_.Get();q: Does this need to be `IUnknown*`? Can it be `ID3D12CommandQueue*` which is converted at the `PresentCompositionTextures` callsite?
// Initialize direct composition with the given d3d11 device and d3d12 command
// queue.nit: Can we note that this can be null?
Related details
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani voted and added 38 comments![]( "Open in Gerrit")
Votes added by Sahir Vellani
| Commit-Queue | +1 |
38 comments
// True if the backing's texture has been unwrapped for use as a D3D12
// resource. Used to prevent unwraps of already unwrapped texture.
bool is_texture_unwrapped_ GUARDED_BY(lock_) = false;Can we add a note that this is referring to `d3d11_texture_` and if `is_texture_unwrapped_ = true` means that the texture _cannot_ be used by D3D11?
Maybe theres a better name for that that is higher level than the 11on12 terminology?
Done
const Microsoft::WRL::ComPtr<ID3D11Texture2D> d3d11_texture_;Can this be made `const` again? I don't see any places that reset it.
Done
bool BeginAccessD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,
bool write_access,
bool is_overlay_access = false);
void EndAccessD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,
bool is_overlay_access = false);Will Begin/EndAccessD3D12 ever be called from outside of D3DImageBacking? If not, can we make them private to the class?
We can make them private for now.
return dcomp_texture_ || (dxgi_shared_handle_state_ &&Sahir VellaniIs this only needed for dx12 comp textures?
Yes
Done
bool ShouldUseD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,Sahir Vellani```suggestion
bool ShouldUseD3D12(ID3D11Device* d3d11_device,
```
Done
D3D11_TEXTURE2D_DESC d3d11_texture_desc) {Sahir Vellani```suggestion
const D3D11_TEXTURE2D_DESC& d3d11_texture_desc) {
```
Done
// Unwraps a D3D11 texture to its underlying D3D12 resource.Given the usage, it seems like `UnwrapUnderlyingResource` can return different results on the same input texture. Please add a note in this comment if that's so.
Done
Does this texture have to have been created on 11on12? What happens if I call this without D3D12 initialized (or from a non-11on12 texture)?
No the texture does not need to be created on 11on12. If this is called without D3D12 initialized, it would be a no-op as the command queue would be null.
We're dropping errors from `hr` here.
Done
d3d11on12_device->ReleaseWrappedResources(&d3d11_texture, 1);Sahir VellaniMight be better to encapsulate this logcin by having the BeginAccessD3D12 and EndAccessD3D12 methods be more explicit about releasing the wrapped resource or unwrapping it based on which one is called. With your current approach, I think you're calling ReleaseWrappedResource when you don't need to.
Did you test the case where media is on a separate device than Chromium?
I think you're right that I'm releasing when I don't have to. We check whether the texture is unwrapped prior to unwrapping anyway.
Done
// UnwrapUnderlyingResource doesn't flush, and it may schedule GPU work. We
// should therefore flush the D3D11 device context here. See:
// https://learn.microsoft.com/en-us/windows/win32/api/d3d11on12/nf-d3d11on12-id3d11on12device2-unwrapunderlyingresourceIs the reason we need to flush is to ensure that any fence signals get realized _now_?
Yes, as UnwrapUnderlyingResource can queue work in the D3D11 context IIUC.
LOG(ERROR) << "Failed to get D3D11 device from D3D12 device. hr="
<< std::hex << hr;```suggestion
LOG(ERROR) << "Failed to get D3D11 device from D3D12 device: "
<< logging::SystemErrorCodeToString(hr);
```
Done
```suggestion
} else {
NOTREACHED();
}
```
Should we guard against unexpected backend types? Or rename this to something like `TryGetDawnDeviceAsD3D11Device`?
Done
auto d3d12_resource =
is_texture_unwrapped_
? nullptr
: PrepareTextureForD3D12(d3d11_texture_.Get(),
GetOrCreateCommandQueueFor11On12());Is there ever a time when `d3d11_texture_` and `d3d12_resource_` are both valid to use at the same time? Could we (reasonably) replace `is_texture_unwrapped_` with a `std::optional<ID3D12Resource>` (or `std::variant`?) to ensure safety in that case?
I purposefully don't clear out the D3D12 resource when returning it back to the 11On12 translation layer because I would need to re-create Dawn's shared texture memory. So I'd rather not use an optional for this case.
Should we guard against unexpected dawn backend types?
I don't think it's necessary. There's a check earlier in this function.
// TODO(savella) Look into why dawn_d3d11_device == texture_d3d11_device_ if
// the resource has a keyed mutex flag. This happens during copy scenariosWhat were the results of your investigation?
Device that opens the shared handle is the one that is returned by `GetDevice`.
// ProduceDawn can be called during D3D12 access. Therefore if the texture
// is already unwrapped, use the underlying resource to create sharedUnless I am missing something, this comment doesn't appear to match what the code is doing. The comment says "use the underlying resource" but the code sets d3d12_resource to nullptr.
Done
// texture memory for the D3D12 resource. Since it was not already in an
// unwrapped state, return the resource back to the 11On12 translation
// layer.This code seems counterintuitive to me. Since the purpose of "ProduceDawn" is to prepare to render to the texture (with Dawn) with D3D12, we shouldn't need to give the resource back to 11on12. We just called PrepareTextureForD3D12 earlier in this same function.
We PrepareTextureForD3D12 to get the resource for creating a SharedTextureMemory. My opinion is that we only unwrap when we definitely want to use the resource; and we know that only happens between Begin/End Access. Returning the resource back to D3D11On12 here keeps it in a predictable state.
CHECK(dawn_d3d11_device);Not sure if intentional, but we are dropping this check. Should it be in `GetD3D11Device`?
Done
CHECK(!is_texture_unwrapped_) << "Texture is unwrapped";Maybe something with a bit more context: "11on12 texture is unwrapped for D3D12 access"? I'm imagining what it would be like to see this crash message after forgetting about this change 😊
Done
AutoLock auto_lock(this);
FlushGraphiteCommandsIfNeeded();
if (!ValidateBeginAccess(write_access)) {
return false;
}
// Defer clearing fences until later to handle failure to synchronize.
std::vector<scoped_refptr<gfx::D3DSharedFence>> wait_fences =
GetPendingWaitFences(d3d11_device, /*dawn_device=*/nullptr, write_access);
for (auto& wait_fence : wait_fences) {
if (!wait_fence->WaitD3D11(d3d11_device)) {
LOG(ERROR) << "Failed to wait for fence";
return false;
}
}
Can this be hoisted up into `BeginAccessD3D`?
I can do this as a follow-up once I get PersistentDawnAccess working, as we need to invalidate persistent access in D3D11 mode before validating begin access and dealing with the fences.
if (!dcomp_texture_) {
CHECK(is_texture_unwrapped_);
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space())
: nullptr;
}
if (is_overlay_access && dcomp_texture_) {
CHECK(!write_access);
BeginDCompTextureAccess();
}
// Clear fences and update state iff D3D12 BeginAccess succeeds.
BeginAccessCommon(write_access);
return true;Can this be hoisted up into `BeginAccessD3D`?
Done
Please LOG(ERROR) for this false condition so we can troubleshoot bad rendering with users.
Done
if (d3d12_resource && d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = std::move(d3d12_resource);
} else if (!d3d12_resource) {
d3d12_resource_.Reset();
return false;
}```suggestion
if (d3d12_resource) {
if (d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = std::move(d3d12_resource);
}
} else {
d3d12_resource_.Reset();
return false;
}
```
Done
// If the accessing device is not the texture's original device, createnit: wording, since the access has finished at this point?
```suggestion
// If the accessing device was not the texture's original device, create
```
Done
if (!d3d11_signal_fence && (!is_texture_device && in_write_access_)) {Could/should we find a way to put the accessing device into `d3d11_signaled_fence_map_` on begin access instead of special casing it here? This map seems to be the general place to sychronize "after D3D11 access".
Done
if (d3d11_signal_fence) {
if (d3d11_signal_fence->IncrementAndSignalD3D11()) {
signaled_fence.insert(d3d11_signal_fence);
} else {
LOG(ERROR) << "Failed to signal D3D11 device fence on EndAccess";
}
} else if (!is_texture_device && in_write_access_) {Since the code seems necessary, we should be able to reuse the IncrementAndSignal code instead of copy/pasting it.
Done
if (is_overlay_access && dcomp_texture_) {
EndDCompTextureAccess();
}
if (in_write_access_) {
NotifyGraphiteAboutInitializedStatus();
}
EndAccessCommon(signaled_fence);Can this be hoisted up into `EndAccessD3D`?
Done
if (d3d11_signal_fence->IncrementAndSignalD3D11()) {Does this code (now) need the same treatment you added to EndAccessD3D11 with regard to calling IncrementAndSignal when we're in write access and the devices do not match? Why or why not?
I'm not aware of a use case that would require us to wait on the d3d11_device here. The existing application of GetPendingWaitFences() seems sufficient.
Microsoft::WRL::ComPtr<ID3D12Fence> d3d12_fence;
if (ShouldUseD3D12(texture_d3d11_device_.Get(), d3d11_texture_desc_)) {
uint64_t fence_value = 0;
HRESULT hr = dcomp_texture_->GetAvailableFence(&fence_value,
IID_PPV_ARGS(&d3d12_fence));
CHECK_EQ(hr, S_OK) << ", GetAvailableFence failed: "
<< logging::SystemErrorCodeToString(hr);
CHECK(d3d12_fence) << "Overlay access is still in use by DWM.";
// If the fence is already passed the wait value, we don't need to wait on
// it.
if (d3d12_fence->GetCompletedValue() >= fence_value) {
return;
}
dcomp_texture_available_fence_ = gfx::D3DSharedFence::CreateFromD3D12Fence(
std::move(d3d12_fence), fence_value);
return;
}
Theres a lot of duplicated code here vs the D3D11 version-- is there a way to deduplicate, so both versions to have to stay as much in sync? Maybe store off `ShouldUseD3D12` and interleave checks to it?
It's a different fence data structure for each version, so we would still have the same amount of code right? My thought is that with the separate block it is easier to read.
if (is_texture_unwrapped_) {
PrepareTextureForD3D11(d3d11_texture_.Get());
is_texture_unwrapped_ = false;
}Please add a comment that describes why we're not clearing out `d3d12_resource_` after we've prepared for D3D11.
Done
// An extension of composition device interface that enables support of d3d12
// for composition textures.
Microsoft::WRL::ComPtr<EXPERIMENTAL_IDCompositionDevice6> dcomp_device_6_;
// A d3d12 command queue from Dawn that is used to present composition
// textures prior to DComp commit.
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;nit: Can we group this with the other `d3d11_device_`, `dcomp_device_`, etc above?
Done
Please improve the code so that we don't end up in a partially initialized state. Suggest that g_d3d12_command_queue is only set when we have IDCompositionDevice6. Then, we can CHECK_EQ(..., S_OK) the QI here.
Done
IUnknown* command_queue_unk = dc_layer_tree_->command_queue_.Get();q: Does this need to be `IUnknown*`? Can it be `ID3D12CommandQueue*` which is converted at the `PresentCompositionTextures` callsite?
Yeah it has to be null. The code fails to compile if an ID3D12CommandQueue* is supplied to PresetCompositionTextures.
CHECK_EQ(hr, S_OK) << "Failed to present composition textures."Similar to Commit, PresentCompositionTextures can fail if the device has been removed or we have presentation loss. We want those cases to be handled in the caller. Please add a new "CommitError::Reason" for PresentCompositionTextures.
Done
CHECK_EQ(hr, S_OK) << "Commit failed with error 0x" << std::hex << hr;Since we do FAILED(hr) right above, hr will never be S_OK to satisfy this CHECK.
Commit can fail if the device has been removed or we have presentation loss. We want those cases to be handled in the caller.
This was from a debugging exercise, missed it.
// Initialize direct composition with the given d3d11 device and d3d12 command
// queue.nit: Can we note that this can be null?
if (g_d3d12_command_queue) {
g_d3d12_command_queue->Release();
g_d3d12_command_queue = nullptr;
}```suggestion
if (g_d3d12_command_queue) {
g_d3d12_command_queue->Release();
g_d3d12_command_queue = nullptr;
}
}
```
- Michael Tang
- Rafael Cintron
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job win-11-perf/motionmark1.3.1.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13c92f68310000
Related details
- Michael Tang
- Rafael Cintron
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/motionmark1.3.1.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11bff6b0310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job win-11-perf/rendering.desktop failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/105e32dfd10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job win-11-perf/rendering.desktop failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/105ec9a6310000
Related details
- Michael Tang
- Rafael Cintron
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job win-11-perf/motionmark1.3.1.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15e78ade310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job win-11-perf/rendering.desktop failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1163e316310000
Sahir Vellani (Gerrit)
Sahir Vellani added 1 comment![]( "Open in Gerrit")
if (!dcomp_texture_) {
CHECK(is_texture_unwrapped_);
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space())
: nullptr;
}
if (is_overlay_access && dcomp_texture_) {
CHECK(!write_access);
BeginDCompTextureAccess();
}
// Clear fences and update state iff D3D12 BeginAccess succeeds.
BeginAccessCommon(write_access);
return true;Sahir VellaniCan this be hoisted up into `BeginAccessD3D`?
Done
Had to undo, because BeginAccessD3D11 is called separately as well; for GLTexturePassthrough ops
Related details
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/motionmark1.3.1.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17a2efd6310000
Related details
- Michael Tang
- Rafael Cintron
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job win-11-perf/rendering.desktop failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/152f6df5310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-10-perf/system_health.common_desktop complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16a322e2310000
Rafael Cintron (Gerrit)
Rafael Cintron added 20 comments![]( "Open in Gerrit")
// will bridge the gap in places where code has not been updated to use
// D3D12 APIs.```suggestion
// will bridge the gap in places SharedImage code uses D3D11 for
// internal operations like copying to staging textures. In the
// future, these places will transition to use Dawn for internal
// operations.
```
if (d3d11on12_device) {Since `d3d11on12_device` will only be nullptr when `QueryInterface` fails, no need for this pointer check after the early return.
CHECK_EQ(hr, S_OK) << ", QueryInterface failed: "
<< logging::SystemErrorCodeToString(hr);An D3D11on12 device should always QI to D3D11 device. We can skip the additional debug spew here. Please do this for the other places where you QI for an 11 device from an 11on12 device.
// queue is used for interop with D3D11; primarily when the backing unwraps
// the D3D11 texture for use as a D3D12 resource.```suggestion
// queue is used for interop with D3D11 when the backing unwraps
// the D3D11 texture for use as a D3D12 resource.
```
if (d3d12_resource && d3d12_resource != d3d12_resource_) {Please add a comment that `d3d12_resource` may not equal `d3d12_resource_` in the case where D3D11 has decided to allocate a new object for the resource. Example: Developer calls UpdateSubresource to modify a resource that is currently being read or written to by the GPU. We want to avoid churning DCompTextures as much as possible.
if (!ShouldUseD3D12(d3d11_device.Get(), d3d11_texture_desc_) &&`DXGISharedHandleState` has a `has_keyed_mutex` method. Can use use that to determine whether we should call `ReleaseKeyedMutex` instead of `ShouldUseD3D12`?
if (!dcomp_texture_) {
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d11_texture_.Get(),
alpha_type(), color_space())
: nullptr;
}```suggestion
if (want_dcomp_texture && !dcomp_texture_) {
dcomp_texture_ = CreateDCompTexture(d3d11_texture_.Get(),
alpha_type(), color_space());
}
```
if (d3d12_resource && d3d12_resource != d3d12_resource_) {Similarly here, please add a comment about 11on12 returning a different d3d12 resource than we might have previously saved away.
CHECK(is_texture_unwrapped_);You JUST set `is_texture_unwrapped_` to true two lines above. No need to CHECK it here as well.
if (!dcomp_texture_) {
CHECK(is_texture_unwrapped_);
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space())
: nullptr;
}```suggestion
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ = CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space());
}
```
if (d3d11_signal_fence->IncrementAndSignalD3D11()) {Sahir VellaniDoes this code (now) need the same treatment you added to EndAccessD3D11 with regard to calling IncrementAndSignal when we're in write access and the devices do not match? Why or why not?
I'm not aware of a use case that would require us to wait on the d3d11_device here. The existing application of GetPendingWaitFences() seems sufficient.
What about the case where we must wait on the ANGLE device's command queue before composing its output with the Dawn command queue? Or the case where WebGPU's output is to be used as WebGL's input?
// texture memory. This will prevent the need to create a new shared texture
// memory on next Dawn access.Please add a comment that we do not clear out the D3D12 resource to avoid re-creating the DComp texture as well.
hr = d3d12_device->CreateCommandQueue(&command_queue_desc,Since `CreateCommandQueue` can return an error when the device is removed, please ensure that all callers of `GetOrCreateCommandQueueForD3D12` check for nullptr and bubble up failures so we can exit the process gracefully instead of crashing.
if (!base::FeatureList::IsEnabled(features::kAllowDCompOnD3D12)) {Why does DCompOnD3D12 flag impact the supported usage we return from this function?
// An extension of composition device interface that enables support of d3d12
// for composition textures.```suggestion
// IDCompositionDevice6 enables support for D3D12 composition textures.
```
// First present composition textures.Please expand the comment to state why `PresentCompositionTextures` must be called before `Commit.`
<< logging::SystemErrorCodeToString(hr);`logging::SystemErrorCodeToString(hr)` prints a friendly string for the error. So `E_INVALIDARG` turns into something like "Invalid argument". It's weird to concatenate that after the string "0x". The "0x" was a holdover from existing code where the hex value of the error was outputted instead. So E_SOME_ERROR would turn into 0x1123ab or whatever the hex valid was.
Better would be for this to be "Present failed with error: " << logging:SystemErrorCodeToString(hr)"
// IDCompositionTexture is not implemented on an 11on12, even though the
// device will claim support for it.Please add to the comment to explain why 11on12 is allowed when DComp runs on D3D12.
BASE_FEATURE(kAllowDCompOnD3D12, base::FEATURE_DISABLED_BY_DEFAULT);```suggestion
BASE_FEATURE(kDCompOnD3D12, base::FEATURE_DISABLED_BY_DEFAULT);
```
BASE_FEATURE(kAllowDCompOnD3D12, base::FEATURE_DISABLED_BY_DEFAULT);Since this is the entrypoint for all of the code, please provide a multi-sentence explanation for the flag and what it does. In the explanation, please add any additional flags that must also be enabled in order for the flag to work correctly.
Related details
- Michael Tang
- Sahir Vellani
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 21 comments![]( "Open in Gerrit")
// will bridge the gap in places where code has not been updated to use
// D3D12 APIs.```suggestion
// will bridge the gap in places SharedImage code uses D3D11 for
// internal operations like copying to staging textures. In the
// future, these places will transition to use Dawn for internal
// operations.
```
Done
Since `d3d11on12_device` will only be nullptr when `QueryInterface` fails, no need for this pointer check after the early return.
Done
CHECK_EQ(hr, S_OK) << ", QueryInterface failed: "
<< logging::SystemErrorCodeToString(hr);An D3D11on12 device should always QI to D3D11 device. We can skip the additional debug spew here. Please do this for the other places where you QI for an 11 device from an 11on12 device.
Done
// queue is used for interop with D3D11; primarily when the backing unwraps
// the D3D11 texture for use as a D3D12 resource.```suggestion
// queue is used for interop with D3D11 when the backing unwraps
// the D3D11 texture for use as a D3D12 resource.
```
Done
if (d3d12_resource && d3d12_resource != d3d12_resource_) {Please add a comment that `d3d12_resource` may not equal `d3d12_resource_` in the case where D3D11 has decided to allocate a new object for the resource. Example: Developer calls UpdateSubresource to modify a resource that is currently being read or written to by the GPU. We want to avoid churning DCompTextures as much as possible.
Done
// texture memory for the D3D12 resource. Since it was not already in an
// unwrapped state, return the resource back to the 11On12 translation
// layer.Sahir VellaniThis code seems counterintuitive to me. Since the purpose of "ProduceDawn" is to prepare to render to the texture (with Dawn) with D3D12, we shouldn't need to give the resource back to 11on12. We just called PrepareTextureForD3D12 earlier in this same function.
We PrepareTextureForD3D12 to get the resource for creating a SharedTextureMemory. My opinion is that we only unwrap when we definitely want to use the resource; and we know that only happens between Begin/End Access. Returning the resource back to D3D11On12 here keeps it in a predictable state.
Done
if (!ShouldUseD3D12(d3d11_device.Get(), d3d11_texture_desc_) &&`DXGISharedHandleState` has a `has_keyed_mutex` method. Can use use that to determine whether we should call `ReleaseKeyedMutex` instead of `ShouldUseD3D12`?
Done
if (!dcomp_texture_) {
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d11_texture_.Get(),
alpha_type(), color_space())
: nullptr;
}```suggestion
if (want_dcomp_texture && !dcomp_texture_) {
dcomp_texture_ = CreateDCompTexture(d3d11_texture_.Get(),
alpha_type(), color_space());
}
```
Done
if (d3d12_resource && d3d12_resource != d3d12_resource_) {Similarly here, please add a comment about 11on12 returning a different d3d12 resource than we might have previously saved away.
Done
You JUST set `is_texture_unwrapped_` to true two lines above. No need to CHECK it here as well.
Done
if (!dcomp_texture_) {
CHECK(is_texture_unwrapped_);
dcomp_texture_ = want_dcomp_texture_
? CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space())
: nullptr;
}```suggestion
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ = CreateDCompTexture(d3d12_resource_.Get(),
alpha_type(), color_space());
}
```
Done
if (d3d11_signal_fence->IncrementAndSignalD3D11()) {Sahir VellaniDoes this code (now) need the same treatment you added to EndAccessD3D11 with regard to calling IncrementAndSignal when we're in write access and the devices do not match? Why or why not?
Rafael CintronI'm not aware of a use case that would require us to wait on the d3d11_device here. The existing application of GetPendingWaitFences() seems sufficient.
What about the case where we must wait on the ANGLE device's command queue before composing its output with the Dawn command queue? Or the case where WebGPU's output is to be used as WebGL's input?
Addressed.
// texture memory. This will prevent the need to create a new shared texture
// memory on next Dawn access.Please add a comment that we do not clear out the D3D12 resource to avoid re-creating the DComp texture as well.
Done
hr = d3d12_device->CreateCommandQueue(&command_queue_desc,Since `CreateCommandQueue` can return an error when the device is removed, please ensure that all callers of `GetOrCreateCommandQueueForD3D12` check for nullptr and bubble up failures so we can exit the process gracefully instead of crashing.
Yes it's already handled. `GetOrCreateCommandQueueForD3D12` is used when calling `PrepareTextureForD3D12`. If `GetOrCreateCommandQueueForD3D12` returns nullptr so does `PrepareTextureForD3D12` which is considered in ensuing logic.
if (!base::FeatureList::IsEnabled(features::kAllowDCompOnD3D12)) {Why does DCompOnD3D12 flag impact the supported usage we return from this function?
If we add the below flags to supported usage, SharedImage opts for a vulkan based backing when rendering webgl, causing a bunch of errors.
// An extension of composition device interface that enables support of d3d12
// for composition textures.```suggestion
// IDCompositionDevice6 enables support for D3D12 composition textures.
```
Done
Please expand the comment to state why `PresentCompositionTextures` must be called before `Commit.`
Done
`logging::SystemErrorCodeToString(hr)` prints a friendly string for the error. So `E_INVALIDARG` turns into something like "Invalid argument". It's weird to concatenate that after the string "0x". The "0x" was a holdover from existing code where the hex value of the error was outputted instead. So E_SOME_ERROR would turn into 0x1123ab or whatever the hex valid was.
Better would be for this to be "Present failed with error: " << logging:SystemErrorCodeToString(hr)"
Done
// IDCompositionTexture is not implemented on an 11on12, even though the
// device will claim support for it.Please add to the comment to explain why 11on12 is allowed when DComp runs on D3D12.
Done
BASE_FEATURE(kAllowDCompOnD3D12, base::FEATURE_DISABLED_BY_DEFAULT);Sahir Vellani```suggestion
BASE_FEATURE(kDCompOnD3D12, base::FEATURE_DISABLED_BY_DEFAULT);
```
Done
BASE_FEATURE(kAllowDCompOnD3D12, base::FEATURE_DISABLED_BY_DEFAULT);Since this is the entrypoint for all of the code, please provide a multi-sentence explanation for the flag and what it does. In the explanation, please add any additional flags that must also be enabled in order for the flag to work correctly.
- Michael Tang
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 6 comments![]( "Open in Gerrit")
@Quyen, @Vasiliy and @Sunny, Sahir is still work on getting all of the scenarios to light up but what he has so far should be ready for review. PTAL.
// D3D11 has decided to allocate a new object for the resource. Example:```suggestion
// D3D11on12 has decided to allocate a new object for the resource. Example:
```
// D3D11 has decided to allocate a new object for the resource. Example:```suggestion
// D3D11on12 has decided to allocate a new object for the resource. Example:
```
// IDCompositionDevice6 enables support for D3D12 composition textures.```suggestion
// IDCompositionDevice6 enables support for D3D12 composition textures.
```
// - WebGL and certain SharedImage functionality such as copies to staging
// textures rely on D3D11 resources. These code paths will continue using
// D3D11 even when this feature is enabled, with interop handled via
// D3D11On12.The comment is not quite correct.
WebGL will continue to use the D3D11 runtime backed by D3D11 drivers with ANGLE's D3D11 device. SharedImage (copies to staging textures) on the other hand, will use D3D11on12 which is the D3D11 runtime backed by a special driver that emulates the D3D11 DDI on top of D3D12. I wouldn't combine these scenarios together as "interop is handled by D3D11on12" because that is not the case.
// This feature requires SkiaGraphite with a dawn-d3d12 backend, BufferQueue toPlease detail the full set of feature flags that need to be enabled so people can easily try it out themselves.
Related details
- Michael Tang
- Quyen Le
- Sahir Vellani
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quyen Le (Gerrit)
Quyen Le added 4 comments![]( "Open in Gerrit")
return dcomp_texture_ || (dxgi_shared_handle_state_ &&`use_cross_device_fence_synchronization()` is used to create an additional fence.
For D3D11, we don't need an additional fence for dcomp. `dcomp_texture_available_fence_` is enough. I don't know if it's needed for D3D11on12 or not. But to avoid regressing existing code's performance, consider use `dcomp_texture_ ` flag here only if D3D11on12 is used.
We want to avoid fences as much as possible for Graphite/D3D11 and it showed it could improve performance a lot.
bool ShouldUseD3D12(ID3D11Device* d3d11_device,Can this result be cached? Maybe at least we should cache it for the texture's original device.
auto& d3d11_signal_fence = d3d11_signaled_fence_map_[d3d11_device];
if (!d3d11_signal_fence &&
(!(d3d11_device == texture_d3d11_device_) && write_access)) {
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(d3d11_device);
}Isn't this already done in `GetPendingWaitFences`? if the existing code in `GetPendingWaitFences` is not enough, maybe we should update it instead?
Microsoft::WRL::ComPtr<ID3D12Fence> d3d12_fence;
uint64_t fence_value = 0;
HRESULT hr = dcomp_texture_->GetAvailableFence(&fence_value,
IID_PPV_ARGS(&d3d12_fence));
CHECK_EQ(hr, S_OK) << ", GetAvailableFence failed: "
<< logging::SystemErrorCodeToString(hr);
CHECK(d3d12_fence) << "Overlay access is still in use by DWM.";
// If the fence is already passed the wait value, we don't need to wait on
// it.
if (d3d12_fence->GetCompletedValue() >= fence_value) {
return;
}
dcomp_texture_available_fence_ = gfx::D3DSharedFence::CreateFromD3D12Fence(
std::move(d3d12_fence), fence_value);
return;This is almost the same as the code below. Maybe we should move it to a template function.
Related details
- Michael Tang
Sahir Vellani (Gerrit)
Sahir Vellani voted and added 10 comments![]( "Open in Gerrit")
Votes added by Sahir Vellani
| Commit-Queue | +1 |
10 comments
`use_cross_device_fence_synchronization()` is used to create an additional fence.
For D3D11, we don't need an additional fence for dcomp. `dcomp_texture_available_fence_` is enough. I don't know if it's needed for D3D11on12 or not. But to avoid regressing existing code's performance, consider use `dcomp_texture_ ` flag here only if D3D11on12 is used.We want to avoid fences as much as possible for Graphite/D3D11 and it showed it could improve performance a lot.
I think this change can be eliminated entirely for now. The already existing check for `dcomp_texture_` in `D3DImageBacking::GetPendingWaitFences` is good enough.
Can this result be cached? Maybe at least we should cache it for the texture's original device.
Caching it for the texture's original device.
// D3D11 has decided to allocate a new object for the resource. Example:```suggestion
// D3D11on12 has decided to allocate a new object for the resource. Example:
```
Done
auto& d3d11_signal_fence = d3d11_signaled_fence_map_[d3d11_device];
if (!d3d11_signal_fence &&
(!(d3d11_device == texture_d3d11_device_) && write_access)) {
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(d3d11_device);
}Isn't this already done in `GetPendingWaitFences`? if the existing code in `GetPendingWaitFences` is not enough, maybe we should update it instead?
Done
// D3D11 has decided to allocate a new object for the resource. Example:```suggestion
// D3D11on12 has decided to allocate a new object for the resource. Example:
```
Done
Microsoft::WRL::ComPtr<ID3D12Fence> d3d12_fence;
uint64_t fence_value = 0;
HRESULT hr = dcomp_texture_->GetAvailableFence(&fence_value,
IID_PPV_ARGS(&d3d12_fence));
CHECK_EQ(hr, S_OK) << ", GetAvailableFence failed: "
<< logging::SystemErrorCodeToString(hr);
CHECK(d3d12_fence) << "Overlay access is still in use by DWM.";
// If the fence is already passed the wait value, we don't need to wait on
// it.
if (d3d12_fence->GetCompletedValue() >= fence_value) {
return;
}
dcomp_texture_available_fence_ = gfx::D3DSharedFence::CreateFromD3D12Fence(
std::move(d3d12_fence), fence_value);
return;This is almost the same as the code below. Maybe we should move it to a template function.
I've abstracted most of the highlighted code into a template function.
// IDCompositionDevice6 enables support for D3D12 composition textures.```suggestion
// IDCompositionDevice6 enables support for D3D12 composition textures.
```
Done
// - WebGL and certain SharedImage functionality such as copies to staging
// textures rely on D3D11 resources. These code paths will continue using
// D3D11 even when this feature is enabled, with interop handled via
// D3D11On12.The comment is not quite correct.
WebGL will continue to use the D3D11 runtime backed by D3D11 drivers with ANGLE's D3D11 device. SharedImage (copies to staging textures) on the other hand, will use D3D11on12 which is the D3D11 runtime backed by a special driver that emulates the D3D11 DDI on top of D3D12. I wouldn't combine these scenarios together as "interop is handled by D3D11on12" because that is not the case.
Done
// This feature requires SkiaGraphite with a dawn-d3d12 backend, BufferQueue toPlease detail the full set of feature flags that need to be enabled so people can easily try it out themselves.
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quyen Le (Gerrit)
Quyen Le added 6 comments![]( "Open in Gerrit")
Can you add some tests to `gpu/command_buffer/service/shared_image/d3d_image_backing_factory_unittest.cc` to verify the new D3D11on12 path works correctly.
// internal operations like copying to staging textures. In the
// future, these places will transition to use Dawn for internal
// operations.nit: This should be placed in a TODO.
bool texture_device_can_use_d3d12_ = false;nit: this can be const since it's initialized in the constructor.
auto& d3d11_signal_fence = d3d11_signaled_fence_map_[wait_d3d11_device];
if (!d3d11_signal_fence && wait_d3d11_device &&
(wait_d3d11_device != texture_d3d11_device_) && write_access) {
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(wait_d3d11_device);
}This seems good to me but we might need to wait for Sunny's review on this part.
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ =
CreateDCompTexture(d3d11_texture_.Get(), alpha_type(), color_space());
}You might need to move this to before calling `GetPendingWaitFences`. Because `GetPendingWaitFences` checks whether dcomp_texture_ is null or not to decide whether to early return.
CreateDCompTexture(d3d12_resource_.Get(), alpha_type(), color_space());ditto
Related details
- Michael Tang
- Rafael Cintron
- Sahir Vellani
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 6 comments![]( "Open in Gerrit")
Can you add some tests to `gpu/command_buffer/service/shared_image/d3d_image_backing_factory_unittest.cc` to verify the new D3D11on12 path works correctly.
Are you comfortable with the tests being added in a follow-up dependent change? The draft CL is in the relation chain.
// internal operations like copying to staging textures. In the
// future, these places will transition to use Dawn for internal
// operations.nit: This should be placed in a TODO.
Done
nit: this can be const since it's initialized in the constructor.
The texture device and its descriptor are obtained in the constructor body, so I think we need to keep it non-const so that the member can be assigned a value in the constructor body.
auto& d3d11_signal_fence = d3d11_signaled_fence_map_[wait_d3d11_device];
if (!d3d11_signal_fence && wait_d3d11_device &&
(wait_d3d11_device != texture_d3d11_device_) && write_access) {
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(wait_d3d11_device);
}This seems good to me but we might need to wait for Sunny's review on this part.
Sounds good. @sun...@chromium.org Tagging you for visibility.
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ =
CreateDCompTexture(d3d11_texture_.Get(), alpha_type(), color_space());
}You might need to move this to before calling `GetPendingWaitFences`. Because `GetPendingWaitFences` checks whether dcomp_texture_ is null or not to decide whether to early return.
Done
CreateDCompTexture(d3d12_resource_.Get(), alpha_type(), color_space());Sahir Vellaniditto
Done
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunny Sachanandani (Gerrit)
Sunny Sachanandani added 10 comments![]( "Open in Gerrit")
Apologies for the delay - just back from vacation this week.
I'm still working through reviewing d3d_image_backing.h/cc which is really the bulk of the CL.
auto d3d11on12_device =nit: explicitly list the type here
// Keyed mutex resources can not be unwrapped by D3D11On12.Are we sure about this? Because we unwrap and use keyed mutex D3D11 resources (from camera capture process) in Dawn and it works: https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/dawn/native/d3d12/DeviceD3D12.cpp;l=641
In fact, Dawn interop used to exclusively rely on keyed mutex synchronization until around 2022 or so. The code in question has been around for a long time (it was removed and reintroduced in an identical form in a different place).
// TODO: Get command queue from Dawn once the 11On12 bug is fixed.nit: link the TODO to a bug
GetD3D11Device(device, backend_type);nit: move this call to inside the if block below
gl::InitializeDirectComposition(d3d11_device_, nullptr);nit: `/*d3d12_command_queue=*/nullptr`
if (!base::FeatureList::IsEnabled(features::kDCompOnD3D12)) {I think we still want to support D3D12 even if we're not using DComp with it. It's useful for testing the D3D12 backend with Graphite.
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;nit: d3d12_command_queue_
InitializeDirectComposition(d3d11_device.Get(), nullptr);nit: `/*d3d12_command_queue=*/nullptr`
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sahir Vellani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani voted and added 9 comments![]( "Open in Gerrit")
Votes added by Sahir Vellani
| Commit-Queue | +1 |
9 comments
nit: explicitly list the type here
Done
// Keyed mutex resources can not be unwrapped by D3D11On12.Are we sure about this? Because we unwrap and use keyed mutex D3D11 resources (from camera capture process) in Dawn and it works: https://source.chromium.org/chromium/chromium/src/+/main:third_party/dawn/src/dawn/native/d3d12/DeviceD3D12.cpp;l=641
In fact, Dawn interop used to exclusively rely on keyed mutex synchronization until around 2022 or so. The code in question has been around for a long time (it was removed and reintroduced in an identical form in a different place).
Yeah I'm sure. 11On12 throws an error when unwrapping a d3d11 texture that's marked as a keyed mutex. However, it does not seem to have problems with wrapping a D3D12 resource into a keyed mutex D3D11 texture, which it looks like that's all Dawn does. I've specified the comment a bit more to state that we can't go from d3d11 to d3d12 if the d3d11 resource is a keyed mutex.
Here's some info: https://learn.microsoft.com/en-us/windows/win32/api/d3d11on12/nf-d3d11on12-id3d11on12device2-unwrapunderlyingresource
// TODO: Get command queue from Dawn once the 11On12 bug is fixed.nit: link the TODO to a bug
Done
nit: move this call to inside the if block below
Done
gl::InitializeDirectComposition(d3d11_device_, nullptr);Sahir Vellaninit: `/*d3d12_command_queue=*/nullptr`
Done
if (!base::FeatureList::IsEnabled(features::kDCompOnD3D12)) {I think we still want to support D3D12 even if we're not using DComp with it. It's useful for testing the D3D12 backend with Graphite.
Done
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue_;Sahir Vellaninit: d3d12_command_queue_
Done
InitializeDirectComposition(d3d11_device.Get(), nullptr);Sahir Vellaninit: `/*d3d12_command_queue=*/nullptr`
Done
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 1 comment![]( "Open in Gerrit")
Hi @sun...@chromium.org, just checking in to see if you had a chance to look at d3d_image_backing.cc :) Any thoughts?
Sunny Sachanandani (Gerrit)
Sunny Sachanandani added 14 comments![]( "Open in Gerrit")
Still reviewing the fence logic, but I thought there was sufficient feedback to send out.
bool is_texture_unwrapped_ GUARDED_BY(lock_) = false;nit: `is_texture_unwrapped_for_d3d12_`?
Microsoft::WRL::ComPtr<ID3D12CommandQueue> d3d12_none_command_queue_;nit: `d3d12_texture_unwrap_command_queue_`?
bool BeginAccessD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,nit: `d3d11on12_device` maybe?
// Unwraps a D3D11 texture to its underlying D3D12 resource. Note that```suggestion
// Unwraps a D3D11on12 texture to its underlying D3D12 resource. Note that
```
Microsoft::WRL::ComPtr<ID3D12Resource> PrepareTextureForD3D12(nit: `PrepareD3D11on12TextureForD3D12` maybe? Or maybe `UnwrapD3D11on12TextureForD3D12`? Naming is hard so I'll leave it up to you (this is a nit after all).
// TODO(crbug.com/474325209): Get command queue from Dawn once the 11On12 bug
// is fixed.nit: maybe this TODO should be at the call-site of `PrepareTextureForD3D12` since that's where we pass it it the non-Dawn command queue, right?
// Returns the D3D11 texture back to the 11On12 layer.```suggestion
// Returns the D3D11on12 texture back to the 11On12 layer.
```
void PrepareTextureForD3D11(ID3D11Resource* d3d11_texture) {nit: `PrepareD3D11on12TextureForD3D11` or perhaps `ReturnD3D11on12TextureToD3D11`? Naming is hard so I'll leave it up to you (this is a nit after all).
GetD3D11Device(device, backend_type);This will cause the 11on12 device to be unnecessarily created on all WebGPU D3D12 devices when the `DCompOnD3D12` feature is enabled.
I think we only want the 11on12 logic for the Graphite Dawn D3D12 device so we should make it conditional on `is_graphite_device` somehow.
shared_texture_memory =
CreateDawnSharedTextureMemory(device, d3d12_resource_);This caches the `SharedTextureMemory` on the first access, but we don't want that since a new `d3d12_resource_` could be set after every `UnwrapUnderlyingResource` call, right?
I'm starting to think that the D3D12 resource unwrapping and returning should be moved to Dawn's `SharedTextureMemory` `Begin/EndAccess` implementation. The rationale is that the only user of the texture via D3D12 is Dawn. This would be done by making a variant of `SharedTextureMemory` on the D3D12 backend that takes a D3D11 texture which Dawn assumes is a D3D11on12 texture from the same device. Then Dawn can handle the unwrapping and returning internally (including the special unwrap command queue). Dawn validation can even assert that the texture doesn't have a keyed mutex.
if (!is_texture_unwrapped_ && d3d12_resource_) {
// The D3D11 texture had to be unwrapped in order to create a shared
// texture memory for the D3D12 resource. Since it was not already in an
// unwrapped state, return the resource back to the 11On12 translation
// layer.
PrepareTextureForD3D11(d3d11_texture_.Get());
}Is this correct? The texture was unwrapped for D3D12 usage by Dawn which will happen soon after `ProduceDawn` in `BeginAccessDawn`. Do we somehow recreate the `SharedTextureMemory` there or signal to Dawn the new `d3d12_resource_` in some other way?
if (!d3d_image_backing->BeginAccessD3D11(d3d11_device_, write_access)) {I believe this is one of the two remaining callers of `BeginAccessD3D11`. We could instead call `BeginAccessD3D` here and it should dispatch to `BeginAccessD3D11` internally. After that, we can make `BeginAccessD3D11` private to `D3DImageBacking`.
if (!d3d_backing->BeginAccessD3D11(texture_device, /*write_access=*/false,This is the other one.
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sahir Vellani
- Vasiliy Telezhnikov
Rafael Cintron (Gerrit)
Rafael Cintron added 1 comment![]( "Open in Gerrit")
shared_texture_memory =
CreateDawnSharedTextureMemory(device, d3d12_resource_);This caches the `SharedTextureMemory` on the first access, but we don't want that since a new `d3d12_resource_` could be set after every `UnwrapUnderlyingResource` call, right?
I'm starting to think that the D3D12 resource unwrapping and returning should be moved to Dawn's `SharedTextureMemory` `Begin/EndAccess` implementation. The rationale is that the only user of the texture via D3D12 is Dawn. This would be done by making a variant of `SharedTextureMemory` on the D3D12 backend that takes a D3D11 texture which Dawn assumes is a D3D11on12 texture from the same device. Then Dawn can handle the unwrapping and returning internally (including the special unwrap command queue). Dawn validation can even assert that the texture doesn't have a keyed mutex.
@sun...@chromium.org, code in SharedImage submits its own D3D11 work via various helpers in `d3d_image_backing.cc` for creating creating staging textures, copying texture and calling UpdateSubresource. If Dawn manages the transition as you suggest, would that code continue to work?
Related details
- Michael Tang
- Quyen Le
- Sahir Vellani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani voted and added 13 comments![]( "Open in Gerrit")
Votes added by Sahir Vellani
| Commit-Queue | +1 |
13 comments
bool is_texture_unwrapped_ GUARDED_BY(lock_) = false;Sahir Vellaninit: `is_texture_unwrapped_for_d3d12_`?
Done
Microsoft::WRL::ComPtr<ID3D12CommandQueue> d3d12_none_command_queue_;Sahir Vellaninit: `d3d12_texture_unwrap_command_queue_`?
Done
bool BeginAccessD3D12(Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device,Sahir Vellaninit: `d3d11on12_device` maybe?
Done
// Unwraps a D3D11 texture to its underlying D3D12 resource. Note that```suggestion
// Unwraps a D3D11on12 texture to its underlying D3D12 resource. Note that
```
Done
Microsoft::WRL::ComPtr<ID3D12Resource> PrepareTextureForD3D12(nit: `PrepareD3D11on12TextureForD3D12` maybe? Or maybe `UnwrapD3D11on12TextureForD3D12`? Naming is hard so I'll leave it up to you (this is a nit after all).
Done
// TODO(crbug.com/474325209): Get command queue from Dawn once the 11On12 bug
// is fixed.nit: maybe this TODO should be at the call-site of `PrepareTextureForD3D12` since that's where we pass it it the non-Dawn command queue, right?
I'm moved it to the declaration of GetOrCreateCommandQueueFor11On12
// Returns the D3D11 texture back to the 11On12 layer.```suggestion
// Returns the D3D11on12 texture back to the 11On12 layer.
```
Done
void PrepareTextureForD3D11(ID3D11Resource* d3d11_texture) {nit: `PrepareD3D11on12TextureForD3D11` or perhaps `ReturnD3D11on12TextureToD3D11`? Naming is hard so I'll leave it up to you (this is a nit after all).
Done
GetD3D11Device(device, backend_type);This will cause the 11on12 device to be unnecessarily created on all WebGPU D3D12 devices when the `DCompOnD3D12` feature is enabled.
I think we only want the 11on12 logic for the Graphite Dawn D3D12 device so we should make it conditional on `is_graphite_device` somehow.
This function is also called in `BeginAccessDawn`. To address this comment, I've added the `SharedContextState` to D3DImageRepresentation so that it is accessible during `BeginAccessDawn`. Should this be done in a follow-up change?
shared_texture_memory =
CreateDawnSharedTextureMemory(device, d3d12_resource_);This caches the `SharedTextureMemory` on the first access, but we don't want that since a new `d3d12_resource_` could be set after every `UnwrapUnderlyingResource` call, right?
I'm starting to think that the D3D12 resource unwrapping and returning should be moved to Dawn's `SharedTextureMemory` `Begin/EndAccess` implementation. The rationale is that the only user of the texture via D3D12 is Dawn. This would be done by making a variant of `SharedTextureMemory` on the D3D12 backend that takes a D3D11 texture which Dawn assumes is a D3D11on12 texture from the same device. Then Dawn can handle the unwrapping and returning internally (including the special unwrap command queue). Dawn validation can even assert that the texture doesn't have a keyed mutex.
The DComp texture also uses the unwrapped resource, so we would need to still do the unwrapping in shared image code. It is a good idea though, and would make the code cleaner. I'm hoping this is a temporary thing and can be removed once we can propagate the 12 resource throughout Chromium. In order to address your concern, I have made the change to unwrap the D3D11on12 texture in BeginAccessDawn (if it isn't already unwrapped). If it is a new resource, I erase and then re-create the SharedTextureMemory.
if (!is_texture_unwrapped_ && d3d12_resource_) {
// The D3D11 texture had to be unwrapped in order to create a shared
// texture memory for the D3D12 resource. Since it was not already in an
// unwrapped state, return the resource back to the 11On12 translation
// layer.
PrepareTextureForD3D11(d3d11_texture_.Get());
}Is this correct? The texture was unwrapped for D3D12 usage by Dawn which will happen soon after `ProduceDawn` in `BeginAccessDawn`. Do we somehow recreate the `SharedTextureMemory` there or signal to Dawn the new `d3d12_resource_` in some other way?
No you're right, this is not necessary. We return the texture in EndAccessCommon.
if (!d3d_image_backing->BeginAccessD3D11(d3d11_device_, write_access)) {I believe this is one of the two remaining callers of `BeginAccessD3D11`. We could instead call `BeginAccessD3D` here and it should dispatch to `BeginAccessD3D11` internally. After that, we can make `BeginAccessD3D11` private to `D3DImageBacking`.
Done
if (!d3d_backing->BeginAccessD3D11(texture_device, /*write_access=*/false,This is the other one.
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Quyen Le (Gerrit)
Quyen Le added 1 comment![]( "Open in Gerrit")
shared_texture_memory =
CreateDawnSharedTextureMemory(device, d3d12_resource_);Sahir VellaniThis caches the `SharedTextureMemory` on the first access, but we don't want that since a new `d3d12_resource_` could be set after every `UnwrapUnderlyingResource` call, right?
I'm starting to think that the D3D12 resource unwrapping and returning should be moved to Dawn's `SharedTextureMemory` `Begin/EndAccess` implementation. The rationale is that the only user of the texture via D3D12 is Dawn. This would be done by making a variant of `SharedTextureMemory` on the D3D12 backend that takes a D3D11 texture which Dawn assumes is a D3D11on12 texture from the same device. Then Dawn can handle the unwrapping and returning internally (including the special unwrap command queue). Dawn validation can even assert that the texture doesn't have a keyed mutex.
The DComp texture also uses the unwrapped resource, so we would need to still do the unwrapping in shared image code. It is a good idea though, and would make the code cleaner. I'm hoping this is a temporary thing and can be removed once we can propagate the 12 resource throughout Chromium. In order to address your concern, I have made the change to unwrap the D3D11on12 texture in BeginAccessDawn (if it isn't already unwrapped). If it is a new resource, I erase and then re-create the SharedTextureMemory.
I think if we want to use D3D12 as the main API (for Skia and Dawn), we should create the D3D12 texture and pass them to this backing. Then wrap it in the D3D11 texture. Not passing D3D11 texture and unwrap. I presume Skia/Dawn should use the D3D12 texture more than DComp. If it requires too much work then it's fine to do in follow up CLs. Maybe adding a TODO?
Related details
- Michael Tang
- Rafael Cintron
- Sahir Vellani
- Sunny Sachanandani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani added 2 comments![]( "Open in Gerrit")
@sun...@chromium.org We reached out to the D3D folks and it turns out that the D3D11 texture can be read from while being checked out for D3D12 use. Regarding the media device however, I think that we should stick to using the D3D12 video decoder when this feature is enabled. A follow up work item would be to bypass the shared handle creation and D3D11-12 resource transfer when the D3D12 video decoder is being created, and directly supply our unwrapped D3D12 resource.
auto& d3d11_signal_fence = d3d11_signaled_fence_map_[wait_d3d11_device];
if (!d3d11_signal_fence && wait_d3d11_device &&
(wait_d3d11_device != texture_d3d11_device_) && write_access) {
d3d11_signal_fence = gfx::D3DSharedFence::CreateForD3D11(wait_d3d11_device);
}Sahir VellaniThis seems good to me but we might need to wait for Sunny's review on this part.
Sounds good. @sun...@chromium.org Tagging you for visibility.
@sun...@chromium.org I've restored this to EndAccessD3D, as discussed offline.
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 1 comment![]( "Open in Gerrit")
Sunny Sachanandani (Gerrit)
Sunny Sachanandani added 14 comments![]( "Open in Gerrit")
const bool want_dcomp_texture_;nit:
```suggestion
const bool want_dcomp_texture_ = false;
```
Microsoft::WRL::ComPtr<ID3D12Resource> d3d12_resource_;nit: can we separate this out from the `d3d12_buffer_` block since otherwise it seems the comment applies to this as well?
} else if (backend_type == wgpu::BackendType::D3D12 &&
base::FeatureList::IsEnabled(features::kDCompOnD3D12) &&
is_graphite_device) {nit:
```suggestion
} else if (base::FeatureList::IsEnabled(features::kDCompOnD3D12) &&
backend_type == wgpu::BackendType::D3D12 &&
is_graphite_device) {
```
use_update_subresource1_(false),
want_dcomp_texture_(false),nit: unnecessary with the suggested change in the header
```suggestion
use_update_subresource1_(false),
```
// Unwrapping is not allowed if the texture is used with a keyed mutex.nit: can you add a comment about what the plan is for keyed mutex textures e.g. those originating from the camera/capture process or at least a TODO to that effect?
// TODO(481916492) Consider creating a D3D12 resource and wrapping itnit:
```suggestion
// TODO(crbug.com/481916492) Consider creating a D3D12 resource and wrapping it
```
auto d3d12_resource =
is_texture_unwrapped_for_d3d12_
? nullptr
: PrepareD3D11on12TextureForD3D12(
d3d11_texture_.Get(), GetOrCreateCommandQueueFor11On12());
if (d3d12_resource) {
is_texture_unwrapped_for_d3d12_ = true;
}
// We want to avoid churning DCompTextures as much as possible. However,
// `d3d12_resource` may not equal `d3d12_resource_` in the case where
// D3D11on12 has decided to allocate a new object for the resource.
// Example: Developer calls UpdateSubresource to modify a resource that
// is currently being read or written to by the GPU.
if (d3d12_resource && d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = d3d12_resource;
}```suggestion
auto d3d12_resource = d3d12_resource_;
if (!is_texture_unwrapped_for_d3d12) {
d3d12_resource = PrepareD3D11on12TextureForD3D12(
d3d11_texture_.Get(), GetOrCreateCommandQueueFor11On12());
is_texture_unwrapped_for_d3d12 = !!d3d12_resource;
}
// We want to avoid churning DCompTextures as much as possible. However,
// `d3d12_resource` may not equal `d3d12_resource_` in the case where
// D3D11on12 has decided to allocate a new object for the resource.
// Example: Developer calls UpdateSubresource to modify a resource that
// is currently being read or written to by the GPU.
if (d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = d3d12_resource;
}
```
if (dawn_d3d11_device == texture_d3d11_device_ && !use_keyed_mutex &&
backend_type == wgpu::BackendType::D3D12) {I think this can make the keyed mutex case also work correctly in the sense that it won't be unwrapped into a D3D12 resource but it will still take the shared handle path correctly. WDYT?
```suggestion
if (dawn_d3d11_device == texture_d3d11_device_ && d3d12_resource_ &&
backend_type == wgpu::BackendType::D3D12) {
```
// backing is created from an existing DXGISharedHandleState. The STM
// is stored inside DXGISharedHandleState but not in the 2nd backing'snit: was this unintentional - not that I mind it per se, but I also wouldn't want to break git blame output unnecessarily
if (backend_type == wgpu::BackendType::D3D11 ||
(backend_type == wgpu::BackendType::D3D12 &&
base::FeatureList::IsEnabled(features::kDCompOnD3D12))) {
auto* dawn_context_provider =
context_state ? context_state->dawn_context_provider() : nullptr;
const bool is_graphite_device =
dawn_context_provider &&
dawn_context_provider->GetDevice().Get() == device.Get();
dawn_d3d11_device =
GetD3D11Device(device, backend_type, is_graphite_device);
}
I think we can get away without passing in a SharedContextState here by relying on the presence of `d3d12_resource_` with `wgpu::BackendType::D3D12` since that implies `ProduceDawn` took the graphite D3D11on12 path already. WDYT?
// If `ProduceDawn` was recently invoked, the texture may already be
// unwrapped. If so, go straight to using `d3d12_resource_` to create
// shared texture memory. If not, unwrap it.
auto d3d12_resource =
is_texture_unwrapped_for_d3d12_
? nullptr
: PrepareD3D11on12TextureForD3D12(
d3d11_texture_.Get(), GetOrCreateCommandQueueFor11On12());
if (d3d12_resource) {
is_texture_unwrapped_for_d3d12_ = true;
if (d3d12_resource != d3d12_resource_) {
d3d12_resource_ = d3d12_resource;
// Erase shared texture memory that would have been created for the
// previous resource.
if (dxgi_shared_handle_state_) {
dxgi_shared_handle_state_->EraseDawnSharedTextureMemory(device);
auto shared_texture_memory =
CreateDawnSharedTextureMemory(device, d3d12_resource_);
dxgi_shared_handle_state_->MaybeCacheSharedTextureMemory(
device, shared_texture_memory);
}
// Reset dcomp texture so that it will be recreated with the new
// resource.
dcomp_texture_.Reset();
}
}Can we factor out this code which is basically the same as what's in ProduceDawn: if texture is not unwrapped for D3D12, unwrap it, if the unwrapped texture doesn't match `d3d12_resource_` also reset `dcomp_texture_`, handle errors along the way
// to be completed on `EndAccess`. This is necessary in the case where DAWN
// may need to access the backing while WebGL is still using it, and
// therefore must wait for WebGL's work to be completed before starting the
// access.nit:
```suggestion
// to be completed on `EndAccess`. This is necessary in the case where Dawn
// may need to access the backing after WebGL has accessed it for write, and
// therefore must wait for WebGL work to be completed.
```
if (d3d12_resource && d3d12_resource != d3d12_resource_) {nit: redundant check
```suggestion
if (d3d12_resource != d3d12_resource_) {
```
d3d12_resource_.Reset();Should we also reset the `dcomp_texture_` here?
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sahir Vellani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 15 comments![]( "Open in Gerrit")
nit:
```suggestion
const bool want_dcomp_texture_ = false;
```
Done
Microsoft::WRL::ComPtr<ID3D12Resource> d3d12_resource_;nit: can we separate this out from the `d3d12_buffer_` block since otherwise it seems the comment applies to this as well?
Done
} else if (backend_type == wgpu::BackendType::D3D12 &&
base::FeatureList::IsEnabled(features::kDCompOnD3D12) &&
is_graphite_device) {nit:
```suggestion
} else if (base::FeatureList::IsEnabled(features::kDCompOnD3D12) &&
backend_type == wgpu::BackendType::D3D12 &&
is_graphite_device) {
```
Done
use_update_subresource1_(false),
want_dcomp_texture_(false),nit: unnecessary with the suggested change in the header
```suggestion
use_update_subresource1_(false),
```
Done
GetD3D11Device(device, backend_type);This will cause the 11on12 device to be unnecessarily created on all WebGPU D3D12 devices when the `DCompOnD3D12` feature is enabled.
I think we only want the 11on12 logic for the Graphite Dawn D3D12 device so we should make it conditional on `is_graphite_device` somehow.
This function is also called in `BeginAccessDawn`. To address this comment, I've added the `SharedContextState` to D3DImageRepresentation so that it is accessible during `BeginAccessDawn`. Should this be done in a follow-up change?
Done
// Unwrapping is not allowed if the texture is used with a keyed mutex.nit: can you add a comment about what the plan is for keyed mutex textures e.g. those originating from the camera/capture process or at least a TODO to that effect?
I've moved the TODO below up to this location. It will solve this problem.
// TODO(481916492) Consider creating a D3D12 resource and wrapping itnit:
```suggestion
// TODO(crbug.com/481916492) Consider creating a D3D12 resource and wrapping it
```
Done
auto d3d12_resource =
is_texture_unwrapped_for_d3d12_
? nullptr
: PrepareD3D11on12TextureForD3D12(
d3d11_texture_.Get(), GetOrCreateCommandQueueFor11On12());
if (d3d12_resource) {
is_texture_unwrapped_for_d3d12_ = true;
}
// We want to avoid churning DCompTextures as much as possible. However,
// `d3d12_resource` may not equal `d3d12_resource_` in the case where
// D3D11on12 has decided to allocate a new object for the resource.
// Example: Developer calls UpdateSubresource to modify a resource that
// is currently being read or written to by the GPU.
if (d3d12_resource && d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = d3d12_resource;
}```suggestion
auto d3d12_resource = d3d12_resource_;
if (!is_texture_unwrapped_for_d3d12) {
d3d12_resource = PrepareD3D11on12TextureForD3D12(
d3d11_texture_.Get(), GetOrCreateCommandQueueFor11On12());
is_texture_unwrapped_for_d3d12 = !!d3d12_resource;
}
// We want to avoid churning DCompTextures as much as possible. However,
// `d3d12_resource` may not equal `d3d12_resource_` in the case where
// D3D11on12 has decided to allocate a new object for the resource.
// Example: Developer calls UpdateSubresource to modify a resource that
// is currently being read or written to by the GPU.
if (d3d12_resource != d3d12_resource_) {
dcomp_texture_.Reset();
d3d12_resource_ = d3d12_resource;
}
```
Done
if (dawn_d3d11_device == texture_d3d11_device_ && !use_keyed_mutex &&
backend_type == wgpu::BackendType::D3D12) {I think this can make the keyed mutex case also work correctly in the sense that it won't be unwrapped into a D3D12 resource but it will still take the shared handle path correctly. WDYT?
```suggestion
if (dawn_d3d11_device == texture_d3d11_device_ && d3d12_resource_ &&
backend_type == wgpu::BackendType::D3D12) {
```
Done
// backing is created from an existing DXGISharedHandleState. The STM
// is stored inside DXGISharedHandleState but not in the 2nd backing'snit: was this unintentional - not that I mind it per se, but I also wouldn't want to break git blame output unnecessarily
Unfortunately it is the result of `git cl format`. I don't know how to avoid a presubmit error otherwise.
if (backend_type == wgpu::BackendType::D3D11 ||
(backend_type == wgpu::BackendType::D3D12 &&
base::FeatureList::IsEnabled(features::kDCompOnD3D12))) {
auto* dawn_context_provider =
context_state ? context_state->dawn_context_provider() : nullptr;
const bool is_graphite_device =
dawn_context_provider &&
dawn_context_provider->GetDevice().Get() == device.Get();
dawn_d3d11_device =
GetD3D11Device(device, backend_type, is_graphite_device);
}
I think we can get away without passing in a SharedContextState here by relying on the presence of `d3d12_resource_` with `wgpu::BackendType::D3D12` since that implies `ProduceDawn` took the graphite D3D11on12 path already. WDYT?
Done
// If `ProduceDawn` was recently invoked, the texture may already be
// unwrapped. If so, go straight to using `d3d12_resource_` to create
// shared texture memory. If not, unwrap it.
auto d3d12_resource =
is_texture_unwrapped_for_d3d12_
? nullptr
: PrepareD3D11on12TextureForD3D12(
d3d11_texture_.Get(), GetOrCreateCommandQueueFor11On12());
if (d3d12_resource) {
is_texture_unwrapped_for_d3d12_ = true;
if (d3d12_resource != d3d12_resource_) {
d3d12_resource_ = d3d12_resource;
// Erase shared texture memory that would have been created for the
// previous resource.
if (dxgi_shared_handle_state_) {
dxgi_shared_handle_state_->EraseDawnSharedTextureMemory(device);
auto shared_texture_memory =
CreateDawnSharedTextureMemory(device, d3d12_resource_);
dxgi_shared_handle_state_->MaybeCacheSharedTextureMemory(
device, shared_texture_memory);
}
// Reset dcomp texture so that it will be recreated with the new
// resource.
dcomp_texture_.Reset();
}
}Can we factor out this code which is basically the same as what's in ProduceDawn: if texture is not unwrapped for D3D12, unwrap it, if the unwrapped texture doesn't match `d3d12_resource_` also reset `dcomp_texture_`, handle errors along the way
Done
// to be completed on `EndAccess`. This is necessary in the case where DAWN
// may need to access the backing while WebGL is still using it, and
// therefore must wait for WebGL's work to be completed before starting the
// access.nit:
```suggestion
// to be completed on `EndAccess`. This is necessary in the case where Dawn
// may need to access the backing after WebGL has accessed it for write, and
// therefore must wait for WebGL work to be completed.
```
Done
if (d3d12_resource && d3d12_resource != d3d12_resource_) {nit: redundant check
```suggestion
if (d3d12_resource != d3d12_resource_) {
```
Done
Should we also reset the `dcomp_texture_` here?
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sunny Sachanandani (Gerrit)
Sunny Sachanandani added 3 comments![]( "Open in Gerrit")
EnsureD3D12Resource();You should check for success of EnsureD3D12Resource() and early out based on that.
if (is_graphite_device) {You might want to skip caching the STM for the D3D11on12 path here - see comment below first.
dxgi_shared_handle_state_->EraseDawnSharedTextureMemory(device);Q: do we even have a `dxgi_shared_handle_state_` when we use the D3D11on12 path for a backing?
We actually might need to clear the cached STM on `dawn_shared_texture_cache_` instead of `dxgi_shared_handle_state_`. You'll need to add a method to `DawnSharedTextureCache` to clear the old entry - there's no existing method to do this because we never remove from the cache currently (it's only used for Graphite's STM which is long lived).
Alternatively, you could skip caching the STM in `ProduceDawn` with a TODO about adding it back if you prefer that instead.
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sahir Vellani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani added 3 comments![]( "Open in Gerrit")
You should check for success of EnsureD3D12Resource() and early out based on that.
Done
You might want to skip caching the STM for the D3D11on12 path here - see comment below first.
Acknowledged
dxgi_shared_handle_state_->EraseDawnSharedTextureMemory(device);Q: do we even have a `dxgi_shared_handle_state_` when we use the D3D11on12 path for a backing?
We actually might need to clear the cached STM on `dawn_shared_texture_cache_` instead of `dxgi_shared_handle_state_`. You'll need to add a method to `DawnSharedTextureCache` to clear the old entry - there's no existing method to do this because we never remove from the cache currently (it's only used for Graphite's STM which is long lived).
Alternatively, you could skip caching the STM in `ProduceDawn` with a TODO about adding it back if you prefer that instead.
Thanks for catching the mistake. I don't think it would be a common scenario to erase and recreate the STM. Therefore I'd rather address this comment than not have any caching at all. I realized that the resource can change during other accesses and render the STM invalid. I've added a bit that tracks when we need to create a new STM. I thought that might be a bit more memory friendly than maintaining a copy of the backing resource of the old STM.
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 13 comments![]( "Open in Gerrit")
void PrepareD3D11on12TextureForD3D11(ID3D11Resource* d3d11_texture) {Please add a CHECK at the beginning of the method that `is_texture_unwrapped_for_d3d12_` is false.
CHECK_EQ(hr, S_OK) << ", QueryInterface failed: "
<< logging::SystemErrorCodeToString(hr);By the time you get to this code, QI to `ID3D11On12Device2` should always succeed. No need to waste DLL bytes on this string, or the call to SystemErrorCodeToString.
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11 device from D3D12 device. hr="
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}If Dawn creates an 11on12 device for us, it should always QI to a D3D11 device. Callers should assume all codepaths to this function succeed.
Microsoft::WRL::ComPtr<IDCompositionTexture> dcomp_texture) {Since `ShouldWaitForDCompTextureFence` does not take ownership of `dcomp_texture`, please pass by bald pointer to avoid unnecessary AddRef/Release.
d3d12_shared_texture_memory_is_current_ = false;Instead of having `d3d12_shared_texture_memory_is_current_`, why not clear out the shared texture memory here?
auto old_d3d12_resource = d3d12_resource_;Why are we caching `d3d12_resource_` into `old_d3d12_resource`? Please add comment to explain or delete.
// Do not release the D3D12 resource here since it may back Dawn's shared
// texture memory. This will prevent the need to create a new shared texture
// memory on next Dawn access and avoid re-creating the DComp texture as
// well.```suggestion
// Do not release the D3D12 resource here since it may back Dawn's shared
// texture memory or a DComp texture. This will prevent the need to create a
// new shared texture or DComp texture the next time we need them.
```
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11On12Device2: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}IIUC, this QI should always succeed. Please CHECK_EQ that it is S_OK.
hr = d3d12_device->CreateCommandQueue(&command_queue_desc,
IID_PPV_ARGS(&command_queue));
if (FAILED(hr)) {
LOG(ERROR) << ", CreateCommandQueue failed: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}D3D dev tells me that calling CreateCommandQueue with "FLAG_NONE" should aways succeed, even if the device has been removed. Should be safe to CHECK_EQ(.., S_OK) here.
Since the QI above should also always succeed, please remove all of the nullptr error handling in callers of GetOrCreateCommandQueueFor11on12.
resource_desc.resource = resource;```suggestion
resource_desc.resource = std::move(resource);
```
// Retrieves the global d3d12 command queue created by Dawn used by SharedImage```suggestion
// Retrieves the global d3d12 command queue created by Chromium's Dawn instance and used by SharedImage
```
// Global d3d12 command queue created by Dawn used by SharedImage code to queue```suggestion
// Global d3d12 command queue created by Chromium's Dawn instance and used by SharedImage code to queue
```
create_device3_function(dxgi_device.Get(), IID_PPV_ARGS(&desktop_device));Passing a device to DCompositionCreateDevice3 is optional. We should factor this code such that InitializeDirectComposition has two overloads, one which takes a D3D11 device and one which takes a D3D12 command queue.
With the current setup, passing DCompositionCreateDevice3 an 11on12 device sets up a dangerous scenario where you can ask it for DCompSurfaces and give those to 12. Since 12 does not respect the scissor rect imposed by BeginDraw/EndDraw, we could end up with hard to debug issues where content can get jumbled up. By not giving DComp any DXGI device in 12 mode, we prevent it from being able to hand out DCompSurfaces altogether and prevent dangerous scenarios.
Related details
- Michael Tang
- Quyen Le
- Sahir Vellani
- Sunny Sachanandani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani voted and added 13 comments![]( "Open in Gerrit")
Votes added by Sahir Vellani
| Commit-Queue | +1 |
13 comments
void PrepareD3D11on12TextureForD3D11(ID3D11Resource* d3d11_texture) {Please add a CHECK at the beginning of the method that `is_texture_unwrapped_for_d3d12_` is false.
It's not a member function. Would you rather it become one to add the CHECK?
CHECK_EQ(hr, S_OK) << ", QueryInterface failed: "
<< logging::SystemErrorCodeToString(hr);By the time you get to this code, QI to `ID3D11On12Device2` should always succeed. No need to waste DLL bytes on this string, or the call to SystemErrorCodeToString.
Done
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11 device from D3D12 device. hr="
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}If Dawn creates an 11on12 device for us, it should always QI to a D3D11 device. Callers should assume all codepaths to this function succeed.
Done
Microsoft::WRL::ComPtr<IDCompositionTexture> dcomp_texture) {Since `ShouldWaitForDCompTextureFence` does not take ownership of `dcomp_texture`, please pass by bald pointer to avoid unnecessary AddRef/Release.
Done
Instead of having `d3d12_shared_texture_memory_is_current_`, why not clear out the shared texture memory here?
We need the WGPU Device to clear out the STM.
Why are we caching `d3d12_resource_` into `old_d3d12_resource`? Please add comment to explain or delete.
Done
// Do not release the D3D12 resource here since it may back Dawn's shared
// texture memory. This will prevent the need to create a new shared texture
// memory on next Dawn access and avoid re-creating the DComp texture as
// well.```suggestion
// Do not release the D3D12 resource here since it may back Dawn's shared
// texture memory or a DComp texture. This will prevent the need to create a
// new shared texture or DComp texture the next time we need them.
```
Done
if (FAILED(hr)) {
LOG(ERROR) << "Failed to get D3D11On12Device2: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}IIUC, this QI should always succeed. Please CHECK_EQ that it is S_OK.
Done
hr = d3d12_device->CreateCommandQueue(&command_queue_desc,
IID_PPV_ARGS(&command_queue));
if (FAILED(hr)) {
LOG(ERROR) << ", CreateCommandQueue failed: "
<< logging::SystemErrorCodeToString(hr);
return nullptr;
}D3D dev tells me that calling CreateCommandQueue with "FLAG_NONE" should aways succeed, even if the device has been removed. Should be safe to CHECK_EQ(.., S_OK) here.
Since the QI above should also always succeed, please remove all of the nullptr error handling in callers of GetOrCreateCommandQueueFor11on12.
Done
resource_desc.resource = resource;```suggestion
resource_desc.resource = std::move(resource);
```
Done
// Retrieves the global d3d12 command queue created by Dawn used by SharedImage```suggestion
// Retrieves the global d3d12 command queue created by Chromium's Dawn instance and used by SharedImage
```
Done
// Global d3d12 command queue created by Dawn used by SharedImage code to queue```suggestion
// Global d3d12 command queue created by Chromium's Dawn instance and used by SharedImage code to queue
```
Done
create_device3_function(dxgi_device.Get(), IID_PPV_ARGS(&desktop_device));Passing a device to DCompositionCreateDevice3 is optional. We should factor this code such that InitializeDirectComposition has two overloads, one which takes a D3D11 device and one which takes a D3D12 command queue.
With the current setup, passing DCompositionCreateDevice3 an 11on12 device sets up a dangerous scenario where you can ask it for DCompSurfaces and give those to 12. Since 12 does not respect the scissor rect imposed by BeginDraw/EndDraw, we could end up with hard to debug issues where content can get jumbled up. By not giving DComp any DXGI device in 12 mode, we prevent it from being able to hand out DCompSurfaces altogether and prevent dangerous scenarios.
I propose that we do this refactor in a separate change. g_d3d11_device is used in other parts of the code; for example in DCompPresenter::Present. I've created a todo.
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 8 comments![]( "Open in Gerrit")
bool d3d12_shared_texture_memory_is_current_ GUARDED_BY(lock_) = false;Please declare `texture_device_can_use_d3d12_ ` next to the shared texture memory variables write a comment that clearly describes how the two are kept in sync.
CHECK_EQ(hr, S_OK) << "Failed to retrieve 11On12 device: "
<< logging::SystemErrorCodeToString(hr);QI from ID3D11Device to ID3D11On12Device should always succeed. Please remove unused debug output.
hr = d3d11on12_device->UnwrapUnderlyingResource(Reading through the 11on12 code, `UnwrapUnderlyingResource` should always return S_OK unless we've passed it invalid arguments. Please CHECK_EQ the return value and fix up the callers to always assume a valid ID3D12Resource is returned.
void PrepareD3D11on12TextureForD3D11(ID3D11Resource* d3d11_texture) {Sahir VellaniPlease add a CHECK at the beginning of the method that `is_texture_unwrapped_for_d3d12_` is false.
It's not a member function. Would you rather it become one to add the CHECK?
Acknowledged
backend_type == wgpu::BackendType::D3D12 && is_graphite_device) {Given that this feature requires Graphite to be enabled, shouldn't `is_graphite_device` always be true if the feature flag is set? If so, please remove the parameter.
GetD3D11Device(device, backend_type, is_graphite_device);Many of the above checks seem redundant with the checks GetD3D11Device already makes. Can we simplify some of this logic a bit more?
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ =
CreateDCompTexture(d3d11_texture_.Get(), alpha_type(), color_space());
}
I think it would be better to move the call to `CreateDCompTexture` until right before `BeginAccessCommon`? This way, we keep the object in a consistent state if we counter errors between here and there.
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ =
CreateDCompTexture(d3d12_resource_.Get(), alpha_type(), color_space());
}Similar to previous feedback, please move the `CreateDCompTexture` call until right before the `BeginAccessCommon` call.
Related details
- Michael Tang
- Quyen Le
- Sahir Vellani
- Sunny Sachanandani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani added 6 comments![]( "Open in Gerrit")
bool d3d12_shared_texture_memory_is_current_ GUARDED_BY(lock_) = false;Please declare `texture_device_can_use_d3d12_ ` next to the shared texture memory variables write a comment that clearly describes how the two are kept in sync.
I think you meant d3d12_shared_texture_memory_is_current_?
CHECK_EQ(hr, S_OK) << "Failed to retrieve 11On12 device: "
<< logging::SystemErrorCodeToString(hr);QI from ID3D11Device to ID3D11On12Device should always succeed. Please remove unused debug output.
Done
Reading through the 11on12 code, `UnwrapUnderlyingResource` should always return S_OK unless we've passed it invalid arguments. Please CHECK_EQ the return value and fix up the callers to always assume a valid ID3D12Resource is returned.
Done
backend_type == wgpu::BackendType::D3D12 && is_graphite_device) {Given that this feature requires Graphite to be enabled, shouldn't `is_graphite_device` always be true if the feature flag is set? If so, please remove the parameter.
We only want to run 11On12 logic for the Graphite Dawn D3D12 device. I believe we can run into this codepath from a non graphite device even if Graphite is enabled.
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ =
CreateDCompTexture(d3d11_texture_.Get(), alpha_type(), color_space());
}
I think it would be better to move the call to `CreateDCompTexture` until right before `BeginAccessCommon`? This way, we keep the object in a consistent state if we counter errors between here and there.
I've moved it to before we call BeginDCompTextureAccess.
if (want_dcomp_texture_ && !dcomp_texture_) {
dcomp_texture_ =
CreateDCompTexture(d3d12_resource_.Get(), alpha_type(), color_space());
}Similar to previous feedback, please move the `CreateDCompTexture` call until right before the `BeginAccessCommon` call.
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 1 comment![]( "Open in Gerrit")
GetD3D11Device(device, backend_type, is_graphite_device);Many of the above checks seem redundant with the checks GetD3D11Device already makes. Can we simplify some of this logic a bit more?
Yeah good point.
Related details
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 1 comment![]( "Open in Gerrit")
Rafael Cintron (Gerrit)
Rafael Cintron added 3 comments![]( "Open in Gerrit")
bool d3d12_shared_texture_memory_is_current_ GUARDED_BY(lock_) = false;Sahir VellaniPlease declare `texture_device_can_use_d3d12_ ` next to the shared texture memory variables write a comment that clearly describes how the two are kept in sync.
I think you meant d3d12_shared_texture_memory_is_current_?
Apologies, yes, I meant `d3d12_shared_texture_memory_is_current_`.
backend_type == wgpu::BackendType::D3D12 && is_graphite_device) {Sahir VellaniGiven that this feature requires Graphite to be enabled, shouldn't `is_graphite_device` always be true if the feature flag is set? If so, please remove the parameter.
We only want to run 11On12 logic for the Graphite Dawn D3D12 device. I believe we can run into this codepath from a non graphite device even if Graphite is enabled.
I presume the scenario you're referring to is WebGPU? If so, please add a comment here describing why is_graphite_device is necessary and why we need to check it in the D3D12 else clause but not the if clause. I presume callers handle this being nullptr?
DLOG(ERROR) << "PresentCompositionTextures failed with error: "```suggestion
LOG(ERROR) << "PresentCompositionTextures failed with error: "
```
Related details
- Michael Tang
- Quyen Le
- Sahir Vellani
- Sunny Sachanandani
- Vasiliy Telezhnikov
Sahir Vellani (Gerrit)
Sahir Vellani added 2 comments![]( "Open in Gerrit")
backend_type == wgpu::BackendType::D3D12 && is_graphite_device) {Sahir VellaniGiven that this feature requires Graphite to be enabled, shouldn't `is_graphite_device` always be true if the feature flag is set? If so, please remove the parameter.
Rafael CintronWe only want to run 11On12 logic for the Graphite Dawn D3D12 device. I believe we can run into this codepath from a non graphite device even if Graphite is enabled.
I presume the scenario you're referring to is WebGPU? If so, please add a comment here describing why is_graphite_device is necessary and why we need to check it in the D3D12 else clause but not the if clause. I presume callers handle this being nullptr?
Yeah, the callers handle this being nullptr.
DLOG(ERROR) << "PresentCompositionTextures failed with error: "```suggestion
LOG(ERROR) << "PresentCompositionTextures failed with error: "
```
- Michael Tang
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sahir Vellani (Gerrit)
Sahir Vellani added 4 comments![]( "Open in Gerrit")
Sahir VellaniDo you anticipate that D3D12+DComp support will be something that we need to query for? Or is it something that we want to just happen automatically when supported without our knowledge? If the former, please add a `OutputSurface::DCSupportLevel::kDCompTextureD3D12` and set it [in SkiaOutputDeviceDComp](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display_embedder/skia_output_device_dcomp.cc;l=54-68;drc=75786f4af376776d0078a6d500a7134172922e59).
Acknowledged
Sahir VellaniCan you add some tests to `gpu/command_buffer/service/shared_image/d3d_image_backing_factory_unittest.cc` to verify the new D3D11on12 path works correctly.
Are you comfortable with the tests being added in a follow-up dependent change? The draft CL is in the relation chain.
Done
if (Microsoft::WRL::ComPtr<PREVIEW_IDCompositionDevice5> dcomp_device5;
SUCCEEDED(dcomp_device.As(&dcomp_device5))) {
return OutputSurface::DCSupportLevel::kDCompDynamicTexture;
}Sahir Vellani```suggestion
if (Microsoft::WRL::ComPtr<PREVIEW_IDCompositionDevice6> dcomp_device6;
SUCCEEDED(dcomp_device.As(&dcomp_device6))) {
return OutputSurface::DCSupportLevel::kDCompTextureD3D12;
}if (Microsoft::WRL::ComPtr<PREVIEW_IDCompositionDevice5> dcomp_device5;
SUCCEEDED(dcomp_device.As(&dcomp_device5))) {
return OutputSurface::DCSupportLevel::kDCompDynamicTexture;
}
```When you clean up this CL, this will be useful to gate your feature on API support. This can possibly replace `kDCompD3D12`.
Acknowledged
Related details
- Quyen Le
- Rafael Cintron
- Sunny Sachanandani
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |