[D3D12, Enc] Use shared fence for D3D11/D3D12 interop scenarios sync [chromium/src : main]
Qiu, Jianlin (Gerrit)
Qiu, Jianlin added 4 comments![]( "Open in Gerrit")
// A helper for synchronization between D3D11 and D3D12 in interop scenarios.```suggestion
// Helpers for sychronizing same underlying resource between D3D11 and D3D12.
```
BASE_FEATURE(kSharedFence, base::FEATURE_ENABLED_BY_DEFAULT);kD3D12VideoEncodeAccleratorUseSharedFence
if (fence_and_value) {
command_queue_->Wait(fence_and_value->first.Get(), fence_and_value->second);
}
Similar as in VP, you should first record commands and then wait on GPU queue, before executing the commands.
if (fence_and_value) {
command_queue_->Wait(fence_and_value->first.Get(), fence_and_value->second);
}
You should first record the commands and then wait on GPU queue before executing the command list, though the difference is trivial.
Related details
- Ma, Yingying
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. |
Ma, Yingying (Gerrit)
Ma, Yingying added 4 comments![]( "Open in Gerrit")
// A helper for synchronization between D3D11 and D3D12 in interop scenarios.```suggestion
// Helpers for sychronizing same underlying resource between D3D11 and D3D12.
```
Done
BASE_FEATURE(kSharedFence, base::FEATURE_ENABLED_BY_DEFAULT);Ma, YingyingkD3D12VideoEncodeAccleratorUseSharedFence
Done
if (fence_and_value) {
command_queue_->Wait(fence_and_value->first.Get(), fence_and_value->second);
}
Similar as in VP, you should first record commands and then wait on GPU queue, before executing the commands.
Done
if (fence_and_value) {
command_queue_->Wait(fence_and_value->first.Get(), fence_and_value->second);
}
You should first record the commands and then wait on GPU queue before executing the command list, though the difference is trivial.
- Qiu, Jianlin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 5 comments![]( "Open in Gerrit")
D3D11FenceAndValue GetD3D11FenceWithNextValue() {```suggestion
D3D11FenceAndValue GetD3D11FenceAndIncrementValue() {
```
if (SUCCEEDED(context4->Signal(fence_and_value->first.Get(),
fence_and_value->second))) {I think we should simplify the complexity of this work by:
-Treat failures from Signal as equivalent to device removed.
-Use D3D11 video decoder if ID3D11DeviceContext4 is not present. Check this early in the lifetime of the decoder, not here in the middle of sync token generation.
Trying to limp along if we get failures in Signal isn't worth the complexity, especially since I do not see any included tests that verify this code path. To be clear, I'm not suggesting you write them.
if (!d3d11_device || FAILED(d3d11_device.As(&device5))) {Can we check for D3D11Device5 availability when the decoder is created instead of attempting to gracefully handle failures during the middle of operation?
interop_fence_ = TryCreateInteropFence(Code here does not check the return value of `TryCreateInteropFence` in case of failure. Does ResolveQueuedSharedImages handle this? How do we prevent the code getting into a continual state of fail? Seems like we want to create the interop fence early in the decoder's lifetime instead of doing it on demand.
fence_and_value.reset();Please add a comment describing why we want to reset fence_and_value here.
Related details
- Eugene Zemtsov
- Ma, Yingying
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. |
Ma, Yingying (Gerrit)
Ma, Yingying added 3 comments![]( "Open in Gerrit")
D3D11FenceAndValue GetD3D11FenceWithNextValue() {Ma, Yingying```suggestion
D3D11FenceAndValue GetD3D11FenceAndIncrementValue() {
```
Done
if (!d3d11_device || FAILED(d3d11_device.As(&device5))) {Can we check for D3D11Device5 availability when the decoder is created instead of attempting to gracefully handle failures during the middle of operation?
You probably mean encoder rather than decoder? If D3D11Device5 is not supported, or if the creation of `interop_fence_`(which is optional) fails, we can fall back to the current solution and wait on the CPU side. I prefer to regard this as a tolerable error. However, if `interop_fence_` has already been created but an error occurs in `GenerateResourceOnSynTokenReleased`, this should be treated as a fatal error（updated in the latest patchset），WDYT？
Please add a comment describing why we want to reset fence_and_value here.
- 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 1 comment![]( "Open in Gerrit")
if (!d3d11_device || FAILED(d3d11_device.As(&device5))) {Ma, YingyingCan we check for D3D11Device5 availability when the decoder is created instead of attempting to gracefully handle failures during the middle of operation?
You probably mean encoder rather than decoder? If D3D11Device5 is not supported, or if the creation of `interop_fence_`(which is optional) fails, we can fall back to the current solution and wait on the CPU side. I prefer to regard this as a tolerable error. However, if `interop_fence_` has already been created but an error occurs in `GenerateResourceOnSynTokenReleased`, this should be treated as a fatal error（updated in the latest patchset），WDYT？
Yes, I meant encoder.
Are there machines where we can't use D3D12 encoder for reasons besides lack of fences? If so, I think we can simplify further and use that fallback when there is no fence support.
Once your change is enabled, few people are going to test the "current solution" as the code evolves and there are no tests to ensure it stays working. The fewer fallbacks we have to maintain, the better.
Related details
- Ma, Yingying
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. |
Ma, Yingying (Gerrit)
Ma, Yingying added 3 comments![]( "Open in Gerrit")
if (SUCCEEDED(context4->Signal(fence_and_value->first.Get(),
fence_and_value->second))) {I think we should simplify the complexity of this work by:
-Treat failures from Signal as equivalent to device removed.
-Use D3D11 video decoder if ID3D11DeviceContext4 is not present. Check this early in the lifetime of the decoder, not here in the middle of sync token generation.Trying to limp along if we get failures in Signal isn't worth the complexity, especially since I do not see any included tests that verify this code path. To be clear, I'm not suggesting you write them.
Done
if (!d3d11_device || FAILED(d3d11_device.As(&device5))) {Ma, YingyingCan we check for D3D11Device5 availability when the decoder is created instead of attempting to gracefully handle failures during the middle of operation?
Rafael CintronYou probably mean encoder rather than decoder? If D3D11Device5 is not supported, or if the creation of `interop_fence_`(which is optional) fails, we can fall back to the current solution and wait on the CPU side. I prefer to regard this as a tolerable error. However, if `interop_fence_` has already been created but an error occurs in `GenerateResourceOnSynTokenReleased`, this should be treated as a fatal error（updated in the latest patchset），WDYT？
Yes, I meant encoder.
Are there machines where we can't use D3D12 encoder for reasons besides lack of fences? If so, I think we can simplify further and use that fallback when there is no fence support.
Once your change is enabled, few people are going to test the "current solution" as the code evolves and there are no tests to ensure it stays working. The fewer fallbacks we have to maintain, the better.
I've added UMA to track interop fence creation failures and simplified the fallback path logic in the latest patchset. I prefer to keep the fallback path for now to avoid block D3D12 VEA tests. If UMA shows no fallback events, we’ll remove both the UMA metric and fallback path in a follow-up update.
Code here does not check the return value of `TryCreateInteropFence` in case of failure. Does ResolveQueuedSharedImages handle this? How do we prevent the code getting into a continual state of fail? Seems like we want to create the interop fence early in the decoder's lifetime instead of doing it on demand.
- Eugene Zemtsov
- 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. |
Ma, Yingying (Gerrit)
Ma, Yingying added 1 comment![]( "Open in Gerrit")
Related details
- Eugene Zemtsov
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")
std::optional<uint64_t> resolved_fence_value;What does it mean for a fence value to be "resolved"?
hr = shared_texture.As(&dxgi_resource);
RETURN_ON_FAILURE_WITH_CALLBACK(
hr, "Failed to query DXGI resource from shared texture");ID3D11Texture2D => IDXGIResource QI should always succeed.
if (fence_and_value) {With multiple textures at play in this method, please add comments that describe what this fence and value is used for. Is this texture going from D3D12 command queue => D3D11 context or the other way around?
Microsoft::WRL::ComPtr<ID3D11DeviceContext4> context4;
hr = d3d11_context.As(&context4);
RETURN_ON_FAILURE_WITH_CALLBACK(hr, "Failed to query context4");If `fence_and_value` is valid, shouldn't the QI to `ID3D11DeviceContext4` always succeed?
if (!d3d11_device || FAILED(d3d11_device.As(&device5))) {Ma, YingyingCan we check for D3D11Device5 availability when the decoder is created instead of attempting to gracefully handle failures during the middle of operation?
Rafael CintronYou probably mean encoder rather than decoder? If D3D11Device5 is not supported, or if the creation of `interop_fence_`(which is optional) fails, we can fall back to the current solution and wait on the CPU side. I prefer to regard this as a tolerable error. However, if `interop_fence_` has already been created but an error occurs in `GenerateResourceOnSynTokenReleased`, this should be treated as a fatal error（updated in the latest patchset），WDYT？
Ma, YingyingYes, I meant encoder.
Are there machines where we can't use D3D12 encoder for reasons besides lack of fences? If so, I think we can simplify further and use that fallback when there is no fence support.
Once your change is enabled, few people are going to test the "current solution" as the code evolves and there are no tests to ensure it stays working. The fewer fallbacks we have to maintain, the better.
I've added UMA to track interop fence creation failures and simplified the fallback path logic in the latest patchset. I prefer to keep the fallback path for now to avoid block D3D12 VEA tests. If UMA shows no fallback events, we’ll remove both the UMA metric and fallback path in a follow-up update.
When you say "... to avoid block D3D12 VEA tests", does that imply there are no tests for the fence code path? When will those be added?
Even if the UMA shows fallback events, I still inclined to have the fallback behavior be to use D3D11 encoder instead of D3D12 + EnqueueSetEvent. The fewer codepaths we need to keep in our minds when working with the code, the better. Right now, everything is std::optional and it's difficult to reason about when things are truly optional vs. a place to create objects when they don't exist.
Which of these two methods is better for performance? How does that change if the browser is D3D12 but we need to fallback to D3D11 encoding?
Microsoft::WRL::ComPtr<ID3D12Device> d3d12_device,
Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device) {Since TryCreateInteropFence does not take ownership of these objects, please pass them by raw/bald pointer to avoid unnecessary AddRef/Release.
return std::make_unique<FenceInteropHelper>(d3d11_fence, d3d12_fence);```suggestion
return std::make_unique<FenceInteropHelper>(std::move(d3d11_fence), std::move(d3d12_fence));
```
interop_fence_ = TryCreateInteropFence(Instead of trying to create the interop fence here in `OnCommandBufferHelperAvailable`, can we create it when `D3D12VideoEncodeAccelerator` is created?
Related details
- Eugene Zemtsov
- Ma, Yingying
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. |
Ma, Yingying (Gerrit)
Ma, Yingying added 8 comments![]( "Open in Gerrit")
What does it mean for a fence value to be "resolved"?
Updated to interop_fence_value.
hr = shared_texture.As(&dxgi_resource);
RETURN_ON_FAILURE_WITH_CALLBACK(
hr, "Failed to query DXGI resource from shared texture");ID3D11Texture2D => IDXGIResource QI should always succeed.
Done
With multiple textures at play in this method, please add comments that describe what this fence and value is used for. Is this texture going from D3D12 command queue => D3D11 context or the other way around?
Done
Microsoft::WRL::ComPtr<ID3D11DeviceContext4> context4;
hr = d3d11_context.As(&context4);
RETURN_ON_FAILURE_WITH_CALLBACK(hr, "Failed to query context4");If `fence_and_value` is valid, shouldn't the QI to `ID3D11DeviceContext4` always succeed?
Done
if (!d3d11_device || FAILED(d3d11_device.As(&device5))) {Ma, YingyingCan we check for D3D11Device5 availability when the decoder is created instead of attempting to gracefully handle failures during the middle of operation?
Rafael CintronYou probably mean encoder rather than decoder? If D3D11Device5 is not supported, or if the creation of `interop_fence_`(which is optional) fails, we can fall back to the current solution and wait on the CPU side. I prefer to regard this as a tolerable error. However, if `interop_fence_` has already been created but an error occurs in `GenerateResourceOnSynTokenReleased`, this should be treated as a fatal error（updated in the latest patchset），WDYT？
Ma, YingyingYes, I meant encoder.
Are there machines where we can't use D3D12 encoder for reasons besides lack of fences? If so, I think we can simplify further and use that fallback when there is no fence support.
Once your change is enabled, few people are going to test the "current solution" as the code evolves and there are no tests to ensure it stays working. The fewer fallbacks we have to maintain, the better.
Rafael CintronI've added UMA to track interop fence creation failures and simplified the fallback path logic in the latest patchset. I prefer to keep the fallback path for now to avoid block D3D12 VEA tests. If UMA shows no fallback events, we’ll remove both the UMA metric and fallback path in a follow-up update.
When you say "... to avoid block D3D12 VEA tests", does that imply there are no tests for the fence code path? When will those be added?
Even if the UMA shows fallback events, I still inclined to have the fallback behavior be to use D3D11 encoder instead of D3D12 + EnqueueSetEvent. The fewer codepaths we need to keep in our minds when working with the code, the better. Right now, everything is std::optional and it's difficult to reason about when things are truly optional vs. a place to create objects when they don't exist.
Which of these two methods is better for performance? How does that change if the browser is D3D12 but we need to fallback to D3D11 encoding?
Updated! The test in question is finch test for D3D12 VEA with shared image encoding enabled. I agree that fewer codepaths are better for readability and maintainability and will not introduce extra confusion around "optional".
Microsoft::WRL::ComPtr<ID3D12Device> d3d12_device,
Microsoft::WRL::ComPtr<ID3D11Device> d3d11_device) {Since TryCreateInteropFence does not take ownership of these objects, please pass them by raw/bald pointer to avoid unnecessary AddRef/Release.
Done
return std::make_unique<FenceInteropHelper>(d3d11_fence, d3d12_fence);```suggestion
return std::make_unique<FenceInteropHelper>(std::move(d3d11_fence), std::move(d3d12_fence));
```
Done
Instead of trying to create the interop fence here in `OnCommandBufferHelperAvailable`, can we create it when `D3D12VideoEncodeAccelerator` is created?
interop fence is only required for shared image encoding. And the creation of a shared interop fence depends on `command_buffer_helper`, which is not available when D3D12VideoEncodeAccelerator.
Related details
- Eugene Zemtsov
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron added 4 comments![]( "Open in Gerrit")
CHECK(SUCCEEDED(d3d11_context.As(&context4)));Please assign the return value of `As` to a local HRESULT and use "CHECK_EQ(hr, S_OK)" to ensure it is exactly the value we're expecting.
interop_fence_->GetD3D11FenceAndIncrementValue(),Today, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
if (!interop_fence_) {If we can't create `interop_fence_` without the command buffer helper, will `interop_fence_` ever be non-null when `OnCommandBufferHelperAvailable` is called? Is the class expected to work without a command buffer helper? If the answer is "No" to both of these, then we should ensure (with CHECK) that `interop_fence_` is nullptr before we do the assignment.
If `OnCommandBufferHelperAvailable` can be called multiple times, could the helper point to a completely different shared image system such that we want always want to make a new interop fence?
return NotifyError(
{EncoderStatus::Codes::kSystemAPICallError,
"Failed to create interop fence for shared image encoding"});If interop fence creation failure always returns an error here, is `D3D12VideoEncodeAccelerator` expected to continue operating without it? If not, we should remove other `interop_fence_` checks in the code and assume it is always valid.
Related details
- Eugene Zemtsov
- Ma, Yingying
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. |
Ma, Yingying (Gerrit)
Ma, Yingying added 6 comments![]( "Open in Gerrit")
Please assign the return value of `As` to a local HRESULT and use "CHECK_EQ(hr, S_OK)" to ensure it is exactly the value we're expecting.
Done
Please assign the return value of `As` to a local HRESULT and use "CHECK_EQ(hr, S_OK)" to ensure it is exactly the value we're expecting.
Done
interop_fence_->GetD3D11FenceAndIncrementValue(),Today, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
Done, Updated to `d3d11_on_d3d12_fence_`.
If we can't create `interop_fence_` without the command buffer helper, will `interop_fence_` ever be non-null when `OnCommandBufferHelperAvailable` is called? Is the class expected to work without a command buffer helper? If the answer is "No" to both of these, then we should ensure (with CHECK) that `interop_fence_` is nullptr before we do the assignment.
If `OnCommandBufferHelperAvailable` can be called multiple times, could the helper point to a completely different shared image system such that we want always want to make a new interop fence?
Done
If we can't create `interop_fence_` without the command buffer helper, will `interop_fence_` ever be non-null when `OnCommandBufferHelperAvailable` is called? Is the class expected to work without a command buffer helper? If the answer is "No" to both of these, then we should ensure (with CHECK) that `interop_fence_` is nullptr before we do the assignment.
If `OnCommandBufferHelperAvailable` can be called multiple times, could the helper point to a completely different shared image system such that we want always want to make a new interop fence?
Done, update to keep align with behavior of `command_buffer_helper_`.
return NotifyError(
{EncoderStatus::Codes::kSystemAPICallError,
"Failed to create interop fence for shared image encoding"});If interop fence creation failure always returns an error here, is `D3D12VideoEncodeAccelerator` expected to continue operating without it? If not, we should remove other `interop_fence_` checks in the code and assume it is always valid.
No, `D3D12VideoEncodeAccelerator` is expected to throw an error.
Related details
- Eugene Zemtsov
- 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 3 comments![]( "Open in Gerrit")
interop_fence_->GetD3D11FenceAndIncrementValue(),Ma, YingyingToday, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
Done, Updated to `d3d11_on_d3d12_fence_`.
The latest patchset is not quite what I had in mind.
I think it is more important to name the variable with the Chromium component that signals the fence to make it more clear that media must wait on the fence. Call it "compositor_fence_", or something similar. CompositorFence will be an 11 fence for now but might be 12 fence in the future so the name of the fence class can remain "InteropFence".
if (FAILED(d3d11_device->QueryInterface(IID_PPV_ARGS(&device5)))) {
LOG(ERROR) << "ID3D11Device5 is not supported.";
return nullptr;
}Why can't we use RETURN_NULLPTR_ON_HR_FAILURE here?
return NotifyError(
{EncoderStatus::Codes::kSystemAPICallError,
"Failed to create interop fence for shared image encoding"});Ma, YingyingIf interop fence creation failure always returns an error here, is `D3D12VideoEncodeAccelerator` expected to continue operating without it? If not, we should remove other `interop_fence_` checks in the code and assume it is always valid.
No, `D3D12VideoEncodeAccelerator` is expected to throw an error.
Since the answer is "No", can we remove the null checks for the fence in all of the code locations except for `OnCommandBufferHelperAvailable`?
Related details
- Eugene Zemtsov
- Ma, Yingying
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. |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin added 1 comment![]( "Open in Gerrit")
interop_fence_->GetD3D11FenceAndIncrementValue(),Ma, YingyingToday, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
Rafael CintronDone, Updated to `d3d11_on_d3d12_fence_`.
The latest patchset is not quite what I had in mind.
I think it is more important to name the variable with the Chromium component that signals the fence to make it more clear that media must wait on the fence. Call it "compositor_fence_", or something similar. CompositorFence will be an 11 fence for now but might be 12 fence in the future so the name of the fence class can remain "InteropFence".
Hi Rafael, I had a discussion with Yingying before on... we're in HW encoder, and incoming shared image VideoFrame is not neccessarily coming from the compositor. It might be from VideoCapture module, or video decoder module, or a MediaStreamTrack, which might never be involved in the composition/rendering pipeline.
Ma, Yingying (Gerrit)
Ma, Yingying added 2 comments![]( "Open in Gerrit")
if (FAILED(d3d11_device->QueryInterface(IID_PPV_ARGS(&device5)))) {
LOG(ERROR) << "ID3D11Device5 is not supported.";
return nullptr;
}Why can't we use RETURN_NULLPTR_ON_HR_FAILURE here?
Done
return NotifyError(
{EncoderStatus::Codes::kSystemAPICallError,
"Failed to create interop fence for shared image encoding"});Ma, YingyingIf interop fence creation failure always returns an error here, is `D3D12VideoEncodeAccelerator` expected to continue operating without it? If not, we should remove other `interop_fence_` checks in the code and assume it is always valid.
Rafael CintronNo, `D3D12VideoEncodeAccelerator` is expected to throw an error.
Since the answer is "No", can we remove the null checks for the fence in all of the code locations except for `OnCommandBufferHelperAvailable`?
- Eugene Zemtsov
- 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 1 comment![]( "Open in Gerrit")
interop_fence_->GetD3D11FenceAndIncrementValue(),Ma, YingyingToday, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
Rafael CintronDone, Updated to `d3d11_on_d3d12_fence_`.
Qiu, JianlinThe latest patchset is not quite what I had in mind.
I think it is more important to name the variable with the Chromium component that signals the fence to make it more clear that media must wait on the fence. Call it "compositor_fence_", or something similar. CompositorFence will be an 11 fence for now but might be 12 fence in the future so the name of the fence class can remain "InteropFence".
Hi Rafael, I had a discussion with Yingying before on... we're in HW encoder, and incoming shared image VideoFrame is not neccessarily coming from the compositor. It might be from VideoCapture module, or video decoder module, or a MediaStreamTrack, which might never be involved in the composition/rendering pipeline.
@jianl...@intel.com, good point about the incoming texture originating from non-compositor sources.
Even so, I think the current "d3d11_on_d3d12" name is confusing because the input component is not always running 11on12. It may be running D3D11 runtime with D3D11 drivers, or D3D12 with D3D12 drivers.
Proposal: Rename the variable to "input_texture_fence", "source_texture_fence" or something similar. This way, it is clear where the fence originates and, therefore, which system is responsible for signaling it.
[In the future the originating system should be responsible for signaling the fence instead of having media do it on its behalf. Today, signaling for D3D11 is easy because there is always an immediate context but in the future, the originating command queue may not be straightforward for media to obtain. We can do this in a future CL]
WDYT?
Related details
- Eugene Zemtsov
- Ma, Yingying
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. |
Ma, Yingying (Gerrit)
Ma, Yingying voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ma, Yingying
| Commit-Queue | +1 |
1 comment
interop_fence_->GetD3D11FenceAndIncrementValue(),Ma, YingyingToday, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
Rafael CintronDone, Updated to `d3d11_on_d3d12_fence_`.
Qiu, JianlinThe latest patchset is not quite what I had in mind.
I think it is more important to name the variable with the Chromium component that signals the fence to make it more clear that media must wait on the fence. Call it "compositor_fence_", or something similar. CompositorFence will be an 11 fence for now but might be 12 fence in the future so the name of the fence class can remain "InteropFence".
Rafael CintronHi Rafael, I had a discussion with Yingying before on... we're in HW encoder, and incoming shared image VideoFrame is not neccessarily coming from the compositor. It might be from VideoCapture module, or video decoder module, or a MediaStreamTrack, which might never be involved in the composition/rendering pipeline.
@jianl...@intel.com, good point about the incoming texture originating from non-compositor sources.
Even so, I think the current "d3d11_on_d3d12" name is confusing because the input component is not always running 11on12. It may be running D3D11 runtime with D3D11 drivers, or D3D12 with D3D12 drivers.
Proposal: Rename the variable to "input_texture_fence", "source_texture_fence" or something similar. This way, it is clear where the fence originates and, therefore, which system is responsible for signaling it.
[In the future the originating system should be responsible for signaling the fence instead of having media do it on its behalf. Today, signaling for D3D11 is easy because there is always an immediate context but in the future, the originating command queue may not be straightforward for media to obtain. We can do this in a future CL]
WDYT?
- Qiu, Jianlin
- 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. |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin added 1 comment![]( "Open in Gerrit")
interop_fence_->GetD3D11FenceAndIncrementValue(),Ma, YingyingToday, it is true that Chromium's compositor runs with D3D11. But in the (hopefully near term) future, it will run with D3D12. We have a dev on the Edge team [working on this](https://chromium-review.googlesource.com/c/chromium/src/+/6733314). Depending on driver maturity and bugs, it may be the case that, on certain machines, the compositor runs on D3D12 but media runs on D3D11.
The above makes this "GetD3D11FenceAndIncrementValue" name confusing. The name "interop fence" is also confusing because it doesn't tell me which work queue owns the signaling of the fence. When I was first reviewing the change, I thought the media engine was the owner of the signaling and that Chromium would be the receiver.
Aren't we going to need a fence that the reader of the texture signals when it is finished reading and writing is permitted? What is that one going to be called?
At a minimum, I suggest we rename interop_fence_ to compositor_fence_.
What can we do to make GetD3D11FenceAndIncrementValue more meaningful? I guess we can add GetD3D12FenceAndIncrementValue in the future? 11on12 may save us in the short term but I'd like to remove that dependency in the future.
Rafael CintronDone, Updated to `d3d11_on_d3d12_fence_`.
Qiu, JianlinThe latest patchset is not quite what I had in mind.
I think it is more important to name the variable with the Chromium component that signals the fence to make it more clear that media must wait on the fence. Call it "compositor_fence_", or something similar. CompositorFence will be an 11 fence for now but might be 12 fence in the future so the name of the fence class can remain "InteropFence".
Rafael CintronHi Rafael, I had a discussion with Yingying before on... we're in HW encoder, and incoming shared image VideoFrame is not neccessarily coming from the compositor. It might be from VideoCapture module, or video decoder module, or a MediaStreamTrack, which might never be involved in the composition/rendering pipeline.
Ma, Yingying@jianl...@intel.com, good point about the incoming texture originating from non-compositor sources.
Even so, I think the current "d3d11_on_d3d12" name is confusing because the input component is not always running 11on12. It may be running D3D11 runtime with D3D11 drivers, or D3D12 with D3D12 drivers.
Proposal: Rename the variable to "input_texture_fence", "source_texture_fence" or something similar. This way, it is clear where the fence originates and, therefore, which system is responsible for signaling it.
[In the future the originating system should be responsible for signaling the fence instead of having media do it on its behalf. Today, signaling for D3D11 is easy because there is always an immediate context but in the future, the originating command queue may not be straightforward for media to obtain. We can do this in a future CL]
WDYT?
Updated to `source_texture_fence_`.
>In the future the originating system should be responsible for signaling the fence instead of having media do it on its behalf.
- Ma, Yingying
- Rafael Cintron
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ma, Yingying
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Ma, Yingying
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ma, Yingying
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Ma, Yingying
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rafael Cintron (Gerrit)
Rafael Cintron voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ma, Yingying
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ma, Yingying (Gerrit)
Ma, Yingying added 1 comment![]( "Open in Gerrit")
- Eugene Zemtsov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Eugene Zemtsov added 4 comments![]( "Open in Gerrit")
class MEDIA_GPU_EXPORT InteropFenceHelper {I find "Helper" suffix somewhat awkward. Let's rename it to `D3D11To12Fence`
// TODO: This CPU-side wait is a temporary solution. The plan is to replace
// this with proper GPU-side synchronization in CL7219003 (for example, making
// the D3D12 VP/encoder command queue wait on the shared fence).please refer to the chromium issue here (crbug.com/XXX) rather than a CL number
auto fence = base::MakeRefCounted<D3D12Fence>(can we reuse this object instead of creating it for every frame?
hr = d3d11_fence->CreateSharedHandle(nullptr, DXGI_SHARED_RESOURCE_READ,the [doc](https://learn.microsoft.com/en-us/windows/win32/api/d3d11_3/nf-d3d11_3-id3d11fence-createsharedhandle) says only `GENERIC_ALL` is acceptable here.
Related details
- Ma, Yingying
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. |