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