Change d3d11 video decoder back resource to support key mutex [chromium/src : main]
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani.
Shaobo Yan would like Dale Curtis and Sunny Sachanandani to review this change.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocal that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn: 576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/status_codes.h
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
6 files changed, 71 insertions(+), 6 deletions(-)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani.
1 comment:
Patchset:
Still trying with local sample to verify the correctness for 0-copy but want to send out to collect arch feedback.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan.
Shaobo Yan has uploaded this change for review.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocal that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn: 576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/status_codes.h
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
6 files changed, 71 insertions(+), 6 deletions(-)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan.
9 comments:
Patchset:
Not super familiar with key mutexes, so defer to Sunny for correctness and cc: rafael.
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #3, Line 139:
: D3D11_RESOURCE_MISC_SHARED_NTHANDLE |Add parens here.
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #3, Line 98:
const ComD3D11VideoDecoderOutputView& output_view() const;Must be inline to use hacker_style(). So this needs to be renabled to GetOutputView now.
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #3, Line 91:
if (!success) {Hmm, is it safe to return output view now or do we need to wait to acquire the mutex here?
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #3, Line 61:
if (FAILED(hr)) {return SUCCEEDED(texture.As(&keyed_mutex_)) if you're not going to have a log message.
Patch Set #3, Line 69:
HRESULT hr = keyed_mutex_->AcquireSync(keyed_mutex_acquire_key_, INFINITE);IIRC this is happening on the GPU thread, so INFINITE mutex wait is a bit concerning, but I guess the hang detector will point out issues here.
Patch Set #3, Line 71:
DLOG(ERROR) << "Unable to acquire the key mutex";DPLOG() so that you get the error code. Ditto below.
Patch Set #3, Line 110:
if (!this->EndAccess()) {No parens for single line conditional. Should just be if (!EndAccess()) ?
Patch Set #3, Line 135:
if (texture != nullptr) {&& !InitKeyMutex(texture)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani, Shaobo Yan.
10 comments:
File media/gpu/windows/d3d11_texture_wrapper.h:
bool InitKeyMutex(ComD3D11Texture2D texture);
bool BeginAccess();
bool EndAccess();InitKeyedMutex, BeginAccess and EndAccess are only called in the implementation of class. Hence, we should make them be protected (if we're going to keep them in the base class) or private (if we're going to move them to DefaultTexture2DWrapper).
Patch Set #3, Line 70:
Microsoft::WRL::ComPtr<IDXGIKeyedMutex> keyed_mutex_;Since keyed_mutex_ is only used DefaultTexture2DWrapper, is there a reason we can't put it as a member variable there instead of having it in the base class?
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #3, Line 59:
ComD3D11Texture2DSince InitKeyedMutext does not take ownership of the texture, please pass it by raw ID3D11Texture2D pointer to avoid unnecessary AddRef/Release.
Patch Set #3, Line 69:
keyed_mutex_acquire_key_Since keyed_mutex_acquire_key_ is never modified, I suggest we hardcode 0 here or put zero in a constant at the top of the file to avoid burning an unnecessary member variable.
Patch Set #3, Line 69:
HRESULT hr = keyed_mutex_->AcquireSync(keyed_mutex_acquire_key_, INFINITE);
IIRC this is happening on the GPU thread, so INFINITE mutex wait is a bit concerning, but I guess th […]
Personally, I prefer that we use "infinite" here. That way, the hang detector will tell us when we have bugs. Unfortunately, bugs where users see "flashing" are common so this is one fewer piece of code where we have to wonder about it being the root cause.
Patch Set #3, Line 71:
DLOG(ERROR) << "Unable to acquire the key mutex";DPLOG() so that you get the error code. Ditto below.
+1
// TODO(liberato): make sure that |mailbox_holders_| is zero-initialized in
// case we don't use all the planes.
for (size_t i = 0; i < VideoFrame::kMaxPlanes; i++)
(*mailbox_dest)[i] = mailbox_holders_[i];
// We're just binding, so the output and output color spaces are the same.
*output_color_space = input_color_space;In general, it is good for methods to fill in output variables when we're sure there are no errors. The received_error_ error already works this way. In light of that, please move the call to EndAccess to be before setting mailbox_dest and output_color_space.
Patch Set #3, Line 110:
if (!this->EndAccess()) {
This call to EndAccess is predicated on output_view always being called first. Are we sure these will always be coupled together? Are there frames where the developer does not ask for a texture and we want to process it anyways?
Depending on the usage, might be better for the BeginAccess/EndAccess be something explicit that happens outside of the class and have this code assert if it is not in the state we expect. But I am not sure how everything fits together to say for sure.
Patch Set #3, Line 135:
texture != nullptrNit: Media code tends to favor not having explicit checks for nullptr.
DLOG(ERROR) << "Failed to get key_mutex from output resource";
return Status(StatusCode::kGetKeyMutexFailed);In D3D11DecoderConfigurator::SetUpTextureDescriptor, we may not always make the output texture with the keyed mutex flags. Will that use-case trigger an error here? If so, suggest we either silently fail or query the desc and only emit an error when the keyed mutex flags are set.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
1 comment:
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #3, Line 69:
HRESULT hr = keyed_mutex_->AcquireSync(keyed_mutex_acquire_key_, INFINITE);Personally, I prefer that we use "infinite" here. […]
That's reasonable to me.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
Shaobo Yan has uploaded this change for review.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocal that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn: 576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/status_codes.h
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
6 files changed, 71 insertions(+), 6 deletions(-)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
1 comment:
Patchset:
Thanks Sunny for his work on
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
Shaobo Yan would like Rafael Cintron to review this change.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocal that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn: 576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/status_codes.h
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
6 files changed, 71 insertions(+), 6 deletions(-)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
1 comment:
Patchset:
Thanks Rafael!
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
Shaobo Yan uploaded patch set #5 to this change.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocol that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn: 576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/status_codes.h
M media/gpu/windows/d3d11_av1_accelerator.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.h
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
M media/gpu/windows/d3d11_vp9_accelerator.cc
11 files changed, 87 insertions(+), 11 deletions(-)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani, Rafael Cintron.
15 comments:
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #3, Line 139:
: D3D11_RESOURCE_MISC_SHARED_NTHANDLE |Add parens here.
Done
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #3, Line 98:
const ComD3D11VideoDecoderOutputView& output_view() const;Must be inline to use hacker_style(). So this needs to be renabled to GetOutputView now.
Done
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #3, Line 91:
if (!success) {Hmm, is it safe to return output view now or do we need to wait to acquire the mutex here?
I think we need to wait to acquire the mutex here or the decoder will report an error.
File media/gpu/windows/d3d11_texture_wrapper.h:
bool InitKeyMutex(ComD3D11Texture2D texture);
bool BeginAccess();
bool EndAccess();
InitKeyedMutex, BeginAccess and EndAccess are only called in the implementation of class. […]
Good point. I thought copy texture wrapper will be different when I'm working this CL but actually it still leverage default texture wrapper.
By thinking it more, I kept an API named PrepareTexture()(Do acquire key mutex) in par with ProcessTexture()(Release key mutex) for PictureBuffer usage. WDYT?
Patch Set #3, Line 70:
Microsoft::WRL::ComPtr<IDXGIKeyedMutex> keyed_mutex_;
Since keyed_mutex_ is only used DefaultTexture2DWrapper, is there a reason we can't put it as a memb […]
Move it to DefaultTexture2DWrapper, Done.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #3, Line 59:
ComD3D11Texture2DSince InitKeyedMutext does not take ownership of the texture, please pass it by raw ID3D11Texture2D […]
Remove the helper function. so Still use texture.As(&keyed_mutex).
Patch Set #3, Line 61:
if (FAILED(hr)) {return SUCCEEDED(texture.As(&keyed_mutex_)) if you're not going to have a log message.
Done
Patch Set #3, Line 69:
keyed_mutex_acquire_key_
Since keyed_mutex_acquire_key_ is never modified, I suggest we hardcode 0 here or put zero in a cons […]
Done with the const number
Patch Set #3, Line 71:
DLOG(ERROR) << "Unable to acquire the key mutex";
+1
Done
// TODO(liberato): make sure that |mailbox_holders_| is zero-initialized in
// case we don't use all the planes.
for (size_t i = 0; i < VideoFrame::kMaxPlanes; i++)
(*mailbox_dest)[i] = mailbox_holders_[i];
// We're just binding, so the output and output color spaces are the same.
*output_color_space = input_color_space;
In general, it is good for methods to fill in output variables when we're sure there are no errors. […]
Thanks for explain. Done.
Patch Set #3, Line 110:
if (!this->EndAccess()) {
No parens for single line conditional. […]
Remove this API. Done.
Patch Set #3, Line 110:
if (!this->EndAccess()) {
This call to EndAccess is predicated on output_view always being called first. […]
Good point. I think it is possible for your case(Not quite sure). So I add a flag
to mark whether the keyed_mutex has been acquired. Combined with checking keyed_mutex_ is nullptr, it may work correctly. WDYT?
Patch Set #3, Line 135:
if (texture != nullptr) {&& !InitKeyMutex(texture)
Remove this API. Done.
Patch Set #3, Line 135:
texture != nullptrNit: Media code tends to favor not having explicit checks for nullptr.
Thanks for explain.
DLOG(ERROR) << "Failed to get key_mutex from output resource";
return Status(StatusCode::kGetKeyMutexFailed);
In D3D11DecoderConfigurator::SetUpTextureDescriptor, we may not always make the output texture with […]
Good point. Now adding a check when we do init. If the texture is nullptr or it doesn't containt key mutex flag. The keyed_mutex_ will keep as nullptr. And the following acquire/release keyed_mutex will check whether keyed_mutex_ is nullptr.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
2 comments:
Patchset:
Mostly lg2m, but defer to Sunny and Rafaels expertise to complete review before stamping.
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #6, Line 78:
const ComD3D11VideoDecoderOutputView& GetOutputView() const;I think this should probably return an ID3D11VideoDecoderOutputView* since it's not transferring any ownership.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani, Shaobo Yan.
10 comments:
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #6, Line 78:
const ComD3D11VideoDecoderOutputView& GetOutputView() const;
I think this should probably return an ID3D11VideoDecoderOutputView* since it's not transferring any […]
+1
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #6, Line 99:
return output_view_;Returning the output view even when the keyed mutex acquire returned false will likely lead to black textures rendering. We should consider return nullptr in these cases and having the caller check for it.
File media/gpu/windows/d3d11_texture_wrapper.h:
// If the |texture| has key mutex, it needs to acquire the key mutex
// before any usage.To make this more informative to readers, please explain the importance of always calling PrepareTexture:
1) Before reading or writing to the texture via views on the texture or other means.
2) Before calling ProcessTexture.Patch Set #6, Line 134:
= nullptrNit: ComPtr already sets its internal pointer to nullptr so there's no need to explicitly set it to nullptr here.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 71:
keyed_mutex_acquired_ = false;If there is no keyed mutex, then keyed_mutex_acquired_ should always be false. This should be a DCHECK instead of an explicit assignment.
Patch Set #6, Line 82:
DPLOG(ERROR) << "Unable to acquire the key mutex";Please assign the return value of AcquireSync to a local variable and output the hresult to the log.
Patch Set #6, Line 98:
FAILED(keyed_mutex_->ReleaseSync(keyed_mutex_acquire_key))Please assign the return value of ReleaseSync to a local variable and output it as part of the log.
Patch Set #6, Line 114:
(*mailbox_dest)[i]So that I am clear, the caller who receives these mailboxes is responsible for acquiring the mutex themselves?
Taking a step back for a moment .... if the D3D11 device we use for media is the same as the one used for shared images, why can't you use the BeginAccess/EndAccess APIs that are on the shared image directly? That seems more robust than having code outside of shared images deal with keyed mutexes itself.
Patch Set #6, Line 143:
texture &&It's too late to check the texture for nullptr here if you're already dereferencing it in the line right above.
Patch Set #6, Line 146:
Status(StatusCode::kGetKeyMutexFailed);Please do error checking on inputs before filling out member variables. If we return an error here, mailbox_holders_ ends up being filled with bogus mailboxes.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan.
Shaobo Yan uploaded patch set #7 to this change.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocol that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn:576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/status_codes.h
M media/gpu/windows/d3d11_av1_accelerator.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.h
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
M media/gpu/windows/d3d11_vp9_accelerator.cc
11 files changed, 88 insertions(+), 11 deletions(-)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani, Rafael Cintron.
15 comments:
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #6, Line 78:
const ComD3D11VideoDecoderOutputView& GetOutputView() const;
+1
Done
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #3, Line 91:
if (!success) {
I think we need to wait to acquire the mutex here or the decoder will report an error.
Done
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #6, Line 99:
return output_view_;
Returning the output view even when the keyed mutex acquire returned false will likely lead to black […]
Done
File media/gpu/windows/d3d11_texture_wrapper.h:
// If the |texture| has key mutex, it needs to acquire the key mutex
// before any usage.
To make this more informative to readers, please explain the importance of always calling PrepareTex […]
Done
Patch Set #6, Line 134:
= nullptr
Nit: ComPtr already sets its internal pointer to nullptr so there's no need to explicitly set it to […]
Done
File media/gpu/windows/d3d11_texture_wrapper.h:
bool InitKeyMutex(ComD3D11Texture2D texture);
bool BeginAccess();
bool EndAccess();
Good point. […]
Done
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #3, Line 59:
ComD3D11Texture2D
Remove the helper function. so Still use texture.As(&keyed_mutex).
Done
Patch Set #3, Line 110:
if (!this->EndAccess()) {Good point. I think it is possible for your case(Not quite sure). So I add a flag […]
Done
DLOG(ERROR) << "Failed to get key_mutex from output resource";
return Status(StatusCode::kGetKeyMutexFailed);
Good point. Now adding a check when we do init. […]
Done
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 71:
keyed_mutex_acquired_ = false;
If there is no keyed mutex, then keyed_mutex_acquired_ should always be false. […]
Done
Patch Set #6, Line 82:
DPLOG(ERROR) << "Unable to acquire the key mutex";Please assign the return value of AcquireSync to a local variable and output the hresult to the log.
Done
Patch Set #6, Line 98:
FAILED(keyed_mutex_->ReleaseSync(keyed_mutex_acquire_key))Please assign the return value of ReleaseSync to a local variable and output it as part of the log.
Done
Patch Set #6, Line 114:
(*mailbox_dest)[i]
So that I am clear, the caller who receives these mailboxes is responsible for acquiring the mutex t […]
Good question. The reason why we cannot use BeginAccess/EndAccess APIs is due to the multithread issues.
The creation of the SharedImage happens in gpu thread but the decoder thread doesn't know whether it could use the sharedimage when it needs to call BeginAccess(Although it is not real async, but it will be in future). The solution to resolve this is to make the pictureBuffer creation/release in an async way. In the time when we don't have 0 value protocol, I have a trying on this here https://chromium-review.googlesource.com/c/chromium/src/+/2651665 But it makes the whole arch in a mess.
So with 0 protocol, this could be simplified a lot to handle this multi-thread things.
Patch Set #6, Line 143:
texture &&
It's too late to check the texture for nullptr here if you're already dereferencing it in the line r […]
Done
Patch Set #6, Line 146:
Status(StatusCode::kGetKeyMutexFailed);
Please do error checking on inputs before filling out member variables. […]
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan.
1 comment:
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]Good question. […]
Perhaps I am missing the big picture, but I'm not sure how this will work.
AcquireSync acquires the lock for the device, not for the thread. If two pieces of code call mutex0->AcquireSync(0), one of them will hang until mutex0->ReleaseSync(0) gets called. It is permissible for one thread to call mutex0->AcquireSync(0) and for a different one to call mutex0->ReleaseSync(0).
In your case, I'm assuming both gpuMain and the decoder thread are using the same D3D device. If so, and gpuMain has called AcquireSync(0), then the decoder thread doesn't need to acquire anything; it can just use the resource. Having the decoder thread also call AcquireSync(0) will make it wait until ReleaseSync(0) has been called by some piece of code.
If we want to simplify things, the only time ReleaseSync(0) should get called is when we want to specifically relinquish control over the resource to another device. Today, that happens with WebGPU (which uses D3D12) and will also happen for DrDC (which will use a different D3D11 device). If a resource has not been given to WebGPU for reading or writing, SharedImage should just AcquireSync(0) it for the ANGLE device and leave it acquired for the decoder thread to use.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
1 comment:
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
Perhaps I am missing the big picture, but I'm not sure how this will work. […]
I think you're correct about the extra acquire/release key_mutex. And this CL is for interacting with WebGPU.
The main issue is that if we leverage gpu thread to do all acquire/release key mutex work. Decoder thread doesn't know when/whether the mutex has been acquired
successfully, this will be a more important issue when we considering interact with WebGPU(which means, we need to acquire/release key_mutex correctly for each
frame to ensure both decoder and webgpu work correctly).In current arch, decoder thread and gpuMain thread interact with task runner to post tasks and use callback to notify the work done, which is an async work model. If we adopt this model, the arch will changes a lot. I want to avoid this.
I think the best solution might be: gpu resource control all IDXGIKeyedMutex acquire/release through shared images but decoder thread needs to have a mechanism to wait until the resource can be used(e.g. acquired key mutex successfully).
I'm not quite sure whether media folder can or allow to use things like std::condition_variable, std::unique_lock and std::mutex to have a non-busy wait mechanism. Or we could use std::atomic (which I find in media folder) to have a busy wait system.
WDYT? cc Sunnyps@ for suggestion too.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan.
1 comment:
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
I think you're correct about the extra acquire/release key_mutex. […]
Reading through you comment makes me think you might run into the same issue Sunny is attempting to fix for GMBs in https://chromium-review.googlesource.com/c/chromium/src/+/2983683. Basically, there is a lock inversion problem if two threads attempt to call AcquireSync(0) for the same mutex at the same time. Will that happen for your codepath?
In my comments for his CL, I proposed a couple of solutions to the problem.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
1 comment:
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
Reading through you comment makes me think you might run into the same issue Sunny is attempting to […]
Oh, yes,it is highly possible for me to hit this issue in current patchSet when we have webgpu into the game.
And by reading your discussions in that CL, I think if we could have a std::condition_variable, std::unique_lock and std::mutex to have a non-busy wait mechanism to communicate between two thread so they could use the same shared_image to acquire/release key mutex, it would be better(Possible in my case).
Whatever, if we choose the two IDXGIKeyedMutex as the solution, I'll add the same mechanism to have a seperate device.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Shaobo Yan, Rafael Cintron.
4 comments:
Patchset:
mostly looks good - holding off on +1 for the keyed mutex discussion and whether we should use the shared handle and keyed mutex flag all the time - maybe guard behind USE_DAWN?
File media/base/status_codes.h:
Patch Set #10, Line 98:
KeyMutexnit: KeyedMutex for consistency with IDXGIKeyedMutex name (also below)
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #10, Line 78:
ID3D11VideoDecoderOutputViewnit: Can you make this return ComPtr instead of raw ptr?
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
Oh, yes,it is highly possible for me to hit this issue in current patchSet when we have webgpu into […]
I don't think you'll encounter the GMB copy deadlock here because we don't have concurrent access of the resource from different threads or even recursive BeginAccess calls on the same thread. When the decoder acquires the keyed mutex in PrepareTexture we have a guarantee that it's not being used by the display compositor, webgl, etc. because the VideoFrame is only returned to the pool after the release sync token has been passed. Similarly when we produce the video frame and send it to the renderer, the keyed mutex has been released by the decoder in ProcessTexture.
Also, what we have here are actually two sequences (decoder and gpu) not threads - sequences in the base::SequencedTaskRunner sense not gpu scheduler sense. Splitting the code into sequences is a forward looking architecture to handle moving the decoder into a separate thread in the future (liberato@ can correct me if I'm wrong).
Finally, we could make the shared image backing thread safe so that you can create it on the decoder thread/sequence and register it with the shared image manager later. The way I imagine this working on the decoder thread is to either use the GLImage(D3D) representation (ProduceOverlay) to get the D3D11 texture or introduce a new representation type specifically for D3D11 (maybe also caching the video processor output view). This representation's Begin/EndAccess and the corresponding ScopedAccess class would be responsible for interacting with the keyed mutex (via Begin/EndAccessD3D11 on the backing).
I understand that's a fair bit of work without any real benefit, so I'm not opposed to the current solution, and I would rather focus our efforts on making sure the WebGPU path doesn't impose undue burden on the common video playback path like needing a keyed mutex and shared handle all the time - a feedback mechanism that upgrades the decoder's buffers to be webgpu compatible would be very nice to have. But of course we should check if there are performance issues first.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
2 comments:
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #10, Line 78:
ID3D11VideoDecoderOutputViewnit: Can you make this return ComPtr instead of raw ptr?
Want to double check this. Address previous comments to change this from ComPtr to RawPtr because I agree with that output_view will be used only once and won't be hold by other class(So it returns the reference),
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
I don't think you'll encounter the GMB copy deadlock here because we don't have concurrent access of […]
Sunny@, Thanks for the explain! I think this CL is something like a basic work of how media part works when it interacts with WebGPU to provide 0-copy path. Sunny@ and Rafael@, So my thoughts on this CL is to
- Add macro or flags to open/close back resource with keyed_mutex support and corresponding keyed_mutex mechanism(And the missing shared_handle creation part in this CL). Because feedback system needs this flag and we could try some prototype of 0-copy by opening/close this flag quickly. WDYT?
< When the decoder acquires the keyed mutex in PrepareTexture we have a guarantee
< that it's not being used by the display compositor, webgl, etc. because the
< VideoFrame is only returned to the pool after the release sync token has been
< passed. Similarly when we produce the video frame and send it to the renderer,
< the keyed mutex has been released by the decoder in ProcessTexture.
Is there any steps WebGL/Display needs to do to ensure this? I think we may hit this because I'm trying this CL with our local 0-copy import path in WebGPU(For internal project performance exploring) and hit a hang on issue. So I suspect we're lacking of some logics. I think the mechanism to leverage sync token to control the pool is awesome and I'd like to leverage on it.
< This representation's Begin/EndAccess and the corresponding ScopedAccess class
< would be responsible for interacting with the keyed mutex (via
< Begin/EndAccessD3D11 on the backing).
To make sure I understand this correctly. This still works like: decoder thread has its own keyed_mutex which is different from gpu thread, right? But we should to leverge ScopedAccess class to interact with keyed_mutex.
< a feedback mechanism that upgrades the decoder's buffers to be webgpu
< compatible would be very nice to have.
Absolutely! With many potential acquire/release pairs been introduced, feedback system is the correct direction.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Shaobo Yan, Rafael Cintron.
2 comments:
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #10, Line 78:
ID3D11VideoDecoderOutputView
Want to double check this. […]
I'm ok either way, but I have a slight preference for returning the ComPtr since we don't know what the caller might do, plus it's consistent with Texture() above returning a ComPtr as well. Feel free to defer to Rafael or Frank on this.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
Is there any steps WebGL/Display needs to do to ensure this? I think we may hit this because I'm trying this CL with our local 0-copy import path in WebGPU(For internal project performance exploring) and hit a hang on issue. So I suspect we're lacking of some logics. I think the mechanism to leverage sync token to control the pool is awesome and I'd like to leverage on it.
The VideoFrame's lifecycle should guarantee this - the decoder won't get back the picture buffer until the video frame is returned by the renderer. If you're seeing a hang, it could be something else too - concurrent access by WebGPU and display/WebGL/canvas causing a recursive keyed mutex acquire. If you could get callstacks for the hung GPU process that would help.
To make sure I understand this correctly. This still works like: decoder thread has its own keyed_mutex which is different from gpu thread, right? But we should to leverge ScopedAccess class to interact with keyed_mutex.
Oh yeah, I forgot that we would have a separate D3D device and hence keyed mutex on the decoder thread. I guess ProduceD3D11 would take a D3D11 device as a parameter and open the shared handle and keyed mutex on that device. Making this performant will be tricky. Also I was mistaken that you can use ProduceOverlay here - that won't work because we call BeginAccessD3D11 (ANGLE device) inside the representation's BeginAccess.
I think for the initial version just keeping the keyed mutex logic in the decoder (what you're doing here) will be simple instead of adding more features to the shared image backing.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Rafael Cintron.
1 comment:
Patchset:
Thanks Sunny and Rafael for the comments.
I think the way for the CL to iterate is to guard the decoder resource type with a flag. Only when the flag is set, decoder will create a keyed_mutex guarded resource for further using.
I'll update the CL in this direction.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Shaobo Yan, Rafael Cintron.
3 comments:
Patchset:
Frank, is it ok if we start using keyed mutex and shared handles unconditionally - we were using the D3D11_RESOURCE_MISC_SHARED flag to workaround a bug with old Intel GPUs but not keyed mutexes.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #10, Line 27:
const uint64_t keyed_mutex_acquire_key = 0u;nit: this is defined in //gpu/command_buffer/common/constants.h as kDXGIKeyedMutexAcquireKey
auto shared_image_backings =
gpu::SharedImageBackingD3D::CreateFromVideoTexture(
mailboxes, dxgi_format, size, usage, texture, array_slice);We need to pass the texture's shared handle to CreateFromVideoTexture, otherwise the shared image cannot be imported into Dawn.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Shaobo Yan, Rafael Cintron.
1 comment:
Patchset:
Oh and forgot that we need to set use_single_video_decoder_texture_ to true (i.e. use individual textures instead of texture array) for this to work as described in https://bugs.chromium.org/p/chromium/issues/detail?id=1238943#c3
Given that, I think we should put this behind a flag for now, maybe something like D3D11VideoDecoderUseSharedHandle (or UseKeyedMutex, ResourceSharing, etc.)
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
File media/gpu/windows/d3d11_copying_texture_wrapper.cc:
Patch Set #10, Line 33:
return output_texture_wrapper_->PrepareTexture();We don't need to call PrepareTexture on the output here since it's only used for the VideoProcessorBlt and not for decoding. Calling it in ProcessTexture would be required though.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
Set Ready For Review
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Frank Liberato, Sunny Sachanandani, Rafael Cintron.
Shaobo Yan would like Frank Liberato to review this change.
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocol that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn:576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
---
M media/base/media_switches.cc
M media/base/media_switches.h
M media/base/status_codes.h
M media/gpu/windows/d3d11_av1_accelerator.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.h
M media/gpu/windows/d3d11_copying_texture_wrapper_unittest.cc
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_decoder_configurator.h
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_selector.cc
M media/gpu/windows/d3d11_texture_selector.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
M media/gpu/windows/d3d11_video_decoder.cc
M media/gpu/windows/d3d11_vp9_accelerator.cc
18 files changed, 271 insertions(+), 45 deletions(-)
Shaobo Yan (Gerrit)
Attention is currently required from: Dale Curtis, Frank Liberato, Sunny Sachanandani, Rafael Cintron.
7 comments:
Patchset:
Sunny, Rafael and Frank,
This CL has been updated, pls take another look, thanks!
More Info:
https://bugs.chromium.org/p/chromium/issues/detail?id=1238943&q=cc%3Ame&can=2
File media/base/status_codes.h:
Patch Set #10, Line 98:
KeyMutexnit: KeyedMutex for consistency with IDXGIKeyedMutex name (also below)
Done
File media/gpu/windows/d3d11_copying_texture_wrapper.cc:
Patch Set #10, Line 33:
return output_texture_wrapper_->PrepareTexture();
We don't need to call PrepareTexture on the output here since it's only used for the VideoProcessorB […]
Done
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #10, Line 78:
ID3D11VideoDecoderOutputView
I'm ok either way, but I have a slight preference for returning the ComPtr since we don't know what […]
Revisit the review record. Rafael and Dale prefer this form, so keep this.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #6, Line 114:
(*mailbox_dest)[i]
> Is there any steps WebGL/Display needs to do to ensure this? I think we may hit this because I'm t […]
Update the CL to resolve this by using single texture workaround
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #10, Line 27:
const uint64_t keyed_mutex_acquire_key = 0u;
nit: this is defined in //gpu/command_buffer/common/constants. […]
Done
auto shared_image_backings =
gpu::SharedImageBackingD3D::CreateFromVideoTexture(
mailboxes, dxgi_format, size, usage, texture, array_slice);
We need to pass the texture's shared handle to CreateFromVideoTexture, otherwise the shared image ca […]
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Dale Curtis, Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
10 comments:
Patchset:
thanks for putting this together!
-fl
File media/gpu/windows/d3d11_copying_texture_wrapper_unittest.cc:
this should be a MOCK_METHOD0(), along with changes to the tests to make sure (a) that it's called by ProcessTexture, and (b) Copy::ProcessTexture fails if this does.
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #12, Line 115:
output_viewonce this returns StatusOr, you can attach it as a caused-by to a new Status(kDecoderBeginFrameFailed). then all of it will make it into the log.
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #12, Line 78:
GetOutputViewi don't think this should be called GetOutputView, because it's not a simple getter. it seems like many folks might not expect this to potentially acquire a mutex as a side-effect. doesn't have to be a big change, but just not 'X* GetX()'
AcquireOutputView? SynchronizeAndGetOutputView()? i'm not sure either :)
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #12, Line 89:
ID3D11VideoDecoderOutputViewStatusOr<ID3D11VideoDecoderOutputView*>, so that it can send the error back if one happens.
File media/gpu/windows/d3d11_texture_selector.h:
Patch Set #12, Line 55:
DecoderOutputTextureUseSharedHandlenit: please rename 'DoesDecoderOutputTextureUseSharedHandle()' or similar, to make it clear that this is asking a question rather than setting a preference. from the 'bool () const' sig here, it's fairly clear, but at the call sites it's less so.
wait -- this is identical to UseSharedHandle(), isn't it?
File media/gpu/windows/d3d11_texture_wrapper.h:
Patch Set #12, Line 55:
// before any usage or you'll get an error. This API is required to be called:please add something about what one has to do to cause the mutex to be released -- i think "ProcessTexture" is the answer.
Patch Set #12, Line 59:
PrepareTexturenit: please rename PrepareTexture to something like AcquireMutexIfNeeded(), so it's clearer what it does.
i admit readily that the existing method "ProcessTexture" is already badly named. it was marginally okay when it meant "just do that one thing this class does", but having both "Process" and "Prepare" would be even more confusing. :)
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #12, Line 72:
!use_shared_handle_ || !keyed_mutex_it doesn't look like one wants to be able to get into a state where exactly one of these two can be set -- init should fail.
normally, i'd suggest getting rid of use_shared_handle_ in favor of making it a parameter to Init. however, i see why you didn't. it's nice that TextureSelector handles it via the ctor, so whoever calls Init doesn't care.
so, maybe just put use_shared_handle_ in the if() here, and DCHECK(keyed_mutex_) inside?
Patch Set #12, Line 78:
keyed_mutex_acquired_if i understand, this can happen if an error happens between the accelerator acquiring it, and the resulting call to ProcessTexture when decoding for that texture completes.
does this really happen in practice without the decoder falling over from whatever error happened? are there other cases that i'm missing where this can happen, besides errors?
i'm wondering if this should just be a DCHECK. or, since DCHECKS are off in production builds and a recursive acquire would hang the gpu process, maybe a normal "return failure" would be appropriate. either way, it seems a little questionable that we might acquire the lock, then forget we have it, and then try to acquire it again.
if it's really okay that it's already acquired at this point, then it needs a good comment here about how that can happen and why it's okay. ideally, though, either (a) the lock gets returned (maybe via RAII), or (b) this condition is just an error.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Shaobo Yan.
6 comments:
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 111:
UpdateOutputTextureToUseSharedHandleThis method seems poorly named. It is called UpdateOutputTextureToUseSharedHandle, yet it doesn't change the attributes of the allocated texture, nor its contents. A better name might be UpdateOutputTextureDescToUseSharedHandle.
Further, we're assuming that this method is called before CreateOutputTexture but after SetupTextureDescriptor. Is that always guaranteed? If so, can we add asserts to double check we're doing it correctly.
Patch Set #12, Line 114:
MiscFlagsTo ensure you're not stomping on existing MiscFlags that may have been set, consider asserting that MiscFlags is zero before setting it here.
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #12, Line 78:
GetOutputView
i don't think this should be called GetOutputView, because it's not a simple getter. […]
+1 to this rename.
File media/gpu/windows/d3d11_texture_selector.h:
Patch Set #12, Line 55:
DecoderOutputTextureUseSharedHandle
nit: please rename 'DoesDecoderOutputTextureUseSharedHandle()' or similar, to make it clear that thi […]
+1 to finding a better name here. DecoderOutputTextureUseSharedHandle seems like an action method you call when you want the decoder to use share handles for the output texture.
File media/gpu/windows/d3d11_texture_wrapper.h:
Patch Set #12, Line 59:
PrepareTexturenit: please rename PrepareTexture to something like AcquireMutexIfNeeded(), so it's clearer what it […]
+1 to finding a better name.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #12, Line 147:
!SUCCEEDEDI know existing code uses !SUCCEEDED(hr) but please use FAILED(hr) instead for better readability.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
1 comment:
File media/gpu/windows/d3d11_texture_selector.h:
Patch Set #12, Line 55:
DecoderOutputTextureUseSharedHandle
wait -- this is identical to UseSharedHandle(), isn't it?
Aha, you got my pain. But the answer is No. We need to handle the CopyTextureSelector, which don't require decoder provide texture with shared handle.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
3 comments:
File media/gpu/windows/d3d11_texture_selector.h:
Patch Set #12, Line 55:
DecoderOutputTextureUseSharedHandle
> wait -- this is identical to UseSharedHandle(), isn't it? […]
i don't see any calls to UseSharedHandle, though.
if you keep it around, then consider renaming it to go with this method more explicitly. something like DoesSharedImageTextureUseSharedHandle() for the one @59.
you might want to rename `use_shared_handle_` to something similar, too, though that's less important.
File media/gpu/windows/d3d11_texture_selector.cc:
Patch Set #12, Line 200:
use_shared_handle_please don't access this directly. i have no idea why this class is friended to the base class, but please use DoesSharedImageTextureUseSharedHandle() instead (or whatever name you pick), here and elsewhere.
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #12, Line 245:
decoder_configurator_->SupportsSwapChain()if SupportsSwapChain, is it a problem that we told TextureSelector to use_shared_handle above, then don't bother with it here?
should 'use_shared_handle' just be set with `IsEnabled && !SupportsSwap` instead?
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
5 comments:
Patchset:
Still working on address all the comments. Here are some quick replies.
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 114:
MiscFlagsTo ensure you're not stomping on existing MiscFlags that may have been set, consider asserting that […]
I think we cannot dcheck miscFlags is 0 because if it is encrypt frames, it contains D3D11_RESOURCE_MISC_HW_PROTECTED.
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #12, Line 115:
output_view
once this returns StatusOr, you can attach it as a caused-by to a new Status(kDecoderBeginFrameFaile […]
I'm not quite clear about how to address this. In my local trying,
I think we still need to call RecordFailure, and use the returned error Status object to provide the message and code. Is it correct?
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #12, Line 78:
keyed_mutex_acquired_if i understand, this can happen if an error happens between the accelerator acquiring it, and the r […]
Good catch and you're right. I think this is a legacy code in the time I want to add keyed_mutex without flags. In that time, when resource is used in the same native device, keyed_mutex could be acquire once and the following ops do not need to acquire it again.
But that's not the case for now. Will remove it in latest patchset!
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #12, Line 245:
decoder_configurator_->SupportsSwapChain()
if SupportsSwapChain, is it a problem that we told TextureSelector to use_shared_handle above, then […]
Good point!
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
1 comment:
File media/gpu/windows/d3d11_texture_selector.cc:
Patch Set #12, Line 200:
use_shared_handle_
please don't access this directly. […]
I think this is the place I could describe the DoesSharedImageTextureUseSharedHandle() usage. In here, we should call UseSharedHandle() instead of DoesSharedImageTextureUseSharedHandle().
The reason is that the DefaultTextureWrapper wrapped by CopyingTexture2DWrapper hold the output_texture, which is the dst of VPBlit. This texture use shared handle.But for the texture generated from D3D11VideoDecoder, DoesSharedImageTextureUseSharedHandle() will always return false if texture selector is CopyTextureSelector. This means the decoder resources don't need to use shared handle. Only the final dst texture use shared handle.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
1 comment:
File media/gpu/windows/d3d11_texture_selector.cc:
Patch Set #12, Line 200:
use_shared_handle_
I think this is the place I could describe the DoesSharedImageTextureUseSharedHandle() usage. […]
sorry, i didn't explain my suggestion well.
there are two fns: DoesSharedImageTextureUse... and (virtual) DoesDecoderOutputTextureUse... . DecoderOutput is the one that would always return false for copying case, or `use_shared_handle_` for non-copying. SharedImage would return `use_shared_handle_` in both cases.
this one can be a call to SharedImage.
additionally, `use_shared_handle_` could be renamed to include 'shared image' or similar, to make it clear which of those two flags it is.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
12 comments:
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 111:
UpdateOutputTextureToUseSharedHandleThis method seems poorly named. […]
I removed this method and combine the logic into CreateOutputTexture.
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #12, Line 115:
output_view
I'm not quite clear about how to address this. In my local trying, […]
I think this has been addressed but not 100% sure.
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #12, Line 78:
GetOutputView+1 to this rename.
Chosse AcquireOutputView() as the placeholder, until I have a better idea.
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #12, Line 89:
ID3D11VideoDecoderOutputViewStatusOr<ID3D11VideoDecoderOutputView*>, so that it can send the error back if one happens.
Done
File media/gpu/windows/d3d11_texture_selector.h:
Patch Set #12, Line 55:
DecoderOutputTextureUseSharedHandle
i don't see any calls to UseSharedHandle, though. […]
Done
File media/gpu/windows/d3d11_texture_selector.cc:
Patch Set #12, Line 200:
use_shared_handle_
sorry, i didn't explain my suggestion well. […]
Thanks for the detail description.
Use the shared_image_use_shared_handle as placeholder until I have a better idea. But I think it could show the difference.
File media/gpu/windows/d3d11_texture_wrapper.h:
Patch Set #12, Line 55:
// before any usage or you'll get an error. This API is required to be called:
please add something about what one has to do to cause the mutex to be released -- i think "ProcessT […]
Done
Patch Set #12, Line 59:
PrepareTexture
+1 to finding a better name.
Choose AcquireMutexIfNeeded() since it told the user what this function do.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #12, Line 72:
!use_shared_handle_ || !keyed_mutex_it doesn't look like one wants to be able to get into a state where exactly one of these two can be […]
Done
Patch Set #12, Line 78:
keyed_mutex_acquired_
Good catch and you're right. […]
Done
Patch Set #12, Line 147:
!SUCCEEDEDI know existing code uses !SUCCEEDED(hr) but please use FAILED(hr) instead for better readability.
Done
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #12, Line 245:
decoder_configurator_->SupportsSwapChain()
Good point!
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Shaobo Yan.
6 comments:
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 114:
MiscFlagsI think we cannot dcheck miscFlags is 0 because if it is encrypt frames, it contains D3D11_RESOURCE_ […]
I see. Do you mean that output_texture_desc_ can have flags from a previous call to UpdateOutputTextureToUseSharedHandle?
File media/gpu/windows/d3d11_decoder_configurator.cc:
DCHECK_EQ(supports_swap_chain_, false);
DCHECK_EQ(output_texture_desc_.MiscFlags & D3D11_RESOURCE_MISC_SHARED,
true);Nit: For checking against true or false, should be sufficient to just do DCHECK(<condition>) or DCHECK(!<condition>)
Patch Set #14, Line 107:
output_texture_desc_use_shared_handle_ = true;Can we lose this member variable and simply check output_texture_desc_.MiscFlags for the shared handle ones?
What's the harm in adding them more than once?
What happens if someone calls the function with use_shared_handle==false and the MiscFlags previously had them applied, should we remove it from output_texture_desc?
I think the rationale for output_texture_desc_use_shared_handle_ needs to be explained a bit better in code comments here. The usage seems a bit fragile.
File media/gpu/windows/d3d11_texture_wrapper.h:
bool use_shared_handle_;
Microsoft::WRL::ComPtr<IDXGIKeyedMutex> keyed_mutex_;Reading through the code, use_shared_handle_ seems duplicated information with keyed_mutex_ being nullptr. We should be able to use the fact that keyed_mutex_ is nullptr in place of use_shared_handle_ being true or false. Further, we can use the MiscFlags on the passed in texture to determine whether should QI for the keyed mutex object instead of using the boolean.
Eliminating use_shared_handle_ should simplify the code and reduce the number of if checks and cognitive load on the reader of the code.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #14, Line 100:
keyed_mutex_ && keyed_mutex_acquired_Should it be an error if the texture has a keyed mutex but hasn't been acquired?
Or do we expect calls to ProcessTexture that happened to not be read or written to in this frame? If so, what you have is fine.
Either way, please add a comment stating when we can get here with the keyed mutex not being acquired.
Patch Set #14, Line 141:
desc.MiscFlags & D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEXIf someone passes use_shared_handle_ == false and we receive a texture with D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX, then it is an error to use the texture without acquiring and releasing the keyed mutex. Nothing will render to it.
Per my previous comment, we should always be checking the MiscFlags and using that to QI for the keyed mutex object. Unless I am missing something, there shouldn't be a need for use_shared_handle_
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
5 comments:
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 114:
MiscFlags
I see. […]
(The feedback system is not implement yet and need more infra like recreate the resource, if we choose that direction, we'll have more CLs).
Yes, the idea is that the whole shareable resource is control behind a feedback system. Which means it is possible to be not shareable and been created with flags D3D11_RESOURCE_MISC_SHARED | D3D11_RESOURCE_MISC_HW_PROTECTED(if is_encrypted). If we set the flag use_shared_handle, we want to replace D3D11_RESOURCE_MISC_SHARED with D3D11_RESOURCE_MISC_SHARED_NTHANDLE | D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX. And keep any other flags if it is valid.One thing not clear here: Is it useful/meaningful to share encrypted video frame to WebGPU/Dawn?
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #14, Line 107:
output_texture_desc_use_shared_handle_ = true;Can we lose this member variable and simply check output_texture_desc_. […]
(The feedback system is not implement yet and need more infra like recreate the resource, if we choose that direction, we'll have more CLs).
You got the point.
The design here is that the (possible) feedback system is a once-set-always-work system. Which means, if use_shared_handle has been set once, the output_texture will always be shareable resource. So it won't need to set back to original flags to avoid thrashing (re-creating textures).
I'll add comments for the flag and here.
File media/gpu/windows/d3d11_texture_wrapper.h:
bool use_shared_handle_;
Microsoft::WRL::ComPtr<IDXGIKeyedMutex> keyed_mutex_;
Reading through the code, use_shared_handle_ seems duplicated information with keyed_mutex_ being nu […]
I see. So I think what you mean is that the use_shared_handle is a used-and-go param and not need to be stored and should be limited to decoder confg part. I'll have a try.
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #14, Line 100:
keyed_mutex_ && keyed_mutex_acquired_
Should it be an error if the texture has a keyed mutex but hasn't been acquired? […]
Good point. I think I'll add DCHECK here. I think it shouldn't happen when texture has a keyed mutex but hasn't been acquired.
Patch Set #14, Line 141:
desc.MiscFlags & D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX
If someone passes use_shared_handle_ == false and we receive a texture with D3D11_RESOURCE_MISC_SHAR […]
Yes, seems that use_shared_handle_ may introduce such errors. Per my previous reply, I'll try to get rid of it.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
6 comments:
Patchset:
Rafael@, I'm working on removing most use_shared_handle_ flag. Only TextureSelector keeps one named "shared_image_use_shared_handle" for CopyTextureSelector and DefaultTextureSelector.
Pls take a look, thanks!
File media/gpu/windows/d3d11_decoder_configurator.cc:
DCHECK_EQ(supports_swap_chain_, false);
DCHECK_EQ(output_texture_desc_.MiscFlags & D3D11_RESOURCE_MISC_SHARED,
true);
Nit: For checking against true or false, should be sufficient to just do DCHECK(<condition>) or DCHE […]
Done
Patch Set #14, Line 107:
output_texture_desc_use_shared_handle_ = true;
(The feedback system is not implement yet and need more infra like recreate the resource, if we choo […]
Defer setup MiscFlag to createOutputTexture because in that time we know the proper flags.
Set the same value multiple times to avoid complex if-condition. Don't know whether this is Ok, Rafael@ maybe you have more comments on this. Thanks!
File media/gpu/windows/d3d11_texture_wrapper.h:
bool use_shared_handle_;
Microsoft::WRL::ComPtr<IDXGIKeyedMutex> keyed_mutex_;
I see. […]
Done
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #14, Line 100:
keyed_mutex_ && keyed_mutex_acquired_
Good point. I think I'll add DCHECK here. […]
Done
Patch Set #14, Line 141:
desc.MiscFlags & D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX
Yes, seems that use_shared_handle_ may introduce such errors. […]
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Shaobo Yan.
6 comments:
File media/gpu/windows/d3d11_av1_accelerator.cc:
Patch Set #15, Line 511:
"DecoderBeginFrame"The text for this error should be "AcquireOutputView" instead of "DecoderBeginFrame". This way, we won't get the error confused with the other possible failure in this method where we actually do call DecoderBeginFrame.
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 114:
MiscFlags
Is it useful/meaningful to share encrypted video frame to WebGPU/Dawn?
No. Not only is it not useful, the OS will prevent you from reading a texture with encrypted content.
Is there a scenario where we need both keyed mutexes + encrypted content?
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #15, Line 85:
keyed_mutex_acquired_In a previous iteration of your CL, if keyed_mutex_acquired_ was true at the beginning of this method, then the method returned OkStatus.
Will AcquireKeyedMutexIfNeeded be called more than once before ProcessTexture does? If so, you'll need to restore that logic, otherwise you'll hang here when code calls AcquireSync when the mutex is already acquired.
On the other hand, if AcquireKeyedMutexIfNeeded can only be called once before ProcessTexture gets called, you should DCHECK that keyed_mutex_acquired_ is false at the beginning of this method so that people know in debug builds what the root cause of the hang is. Ultimately, we can get rid of keyed_mutex_acquired_ but I'm fine keeping it in for easier debugging.
Patch Set #15, Line 98:
!FAILED(hr)!FAILED(hr) == success, yet your code returns an error to the caller.
I think you want FAILED(hr) here.
hr = dxgi_resource->CreateSharedHandle(
nullptr, DXGI_SHARED_RESOURCE_READ | DXGI_SHARED_RESOURCE_WRITE,
nullptr, &handle);Apologies for not realizing this earlier but we should check the return value of CreateSharedHandle and flag and error if it fails. I expect you will hit this error more frequently than the QI ones you've already added new errors for in this change.
File media/gpu/windows/d3d11_vp9_accelerator.cc:
Patch Set #15, Line 90:
"DecoderBeginFrame"Should this failure be "AcquireOutputView" instead of "DecoderBeginFrame"?
If we use the latter, we'll get the error confused with the RecordFailure later in this method when we actually do call DecoderBeginFrame.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
4 comments:
File media/gpu/windows/d3d11_av1_accelerator.cc:
Patch Set #15, Line 511:
"DecoderBeginFrame"
The text for this error should be "AcquireOutputView" instead of "DecoderBeginFrame". […]
Done
File media/gpu/windows/d3d11_texture_wrapper.cc:
Patch Set #15, Line 85:
keyed_mutex_acquired_
In a previous iteration of your CL, if keyed_mutex_acquired_ was true at the beginning of this metho […]
You're right. Shared resource is living behind use_single_texture workaround. I think decoder should prevent AcquireKeyedMutexIfNeeded be called more than once before ProcessTexture. Add a DCHECK for it.
Patch Set #15, Line 98:
!FAILED(hr)
!FAILED(hr) == success, yet your code returns an error to the caller. […]
Oops... Thanks!
hr = dxgi_resource->CreateSharedHandle(
nullptr, DXGI_SHARED_RESOURCE_READ | DXGI_SHARED_RESOURCE_WRITE,
nullptr, &handle);
Apologies for not realizing this earlier but we should check the return value of CreateSharedHandle […]
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
2 comments:
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 114:
MiscFlags
> Is it useful/meaningful to share encrypted video frame to WebGPU/Dawn? […]
I don't think there is a sceneario for keyed_mutexes + encrypted content. It's just for the case which resource is created with D3D11_RESOURCE_MISC_SHARED | D3D11_RESOURCE_MISC_HW_PROTECTED. So does this mean if we find the video frame is encrypted, we shouldn't enable use_share_handle?
File media/gpu/windows/d3d11_vp9_accelerator.cc:
Patch Set #15, Line 90:
"DecoderBeginFrame"
Should this failure be "AcquireOutputView" instead of "DecoderBeginFrame"? […]
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Frank Liberato, Sunny Sachanandani, Rafael Cintron.
5 comments:
Patchset:
Frank@, Rafael@ and Sunny@, Please take another look, thanks!
File media/gpu/windows/d3d11_copying_texture_wrapper_unittest.cc:
this should be a MOCK_METHOD0(), along with changes to the tests to make sure (a) that it's called b […]
Done
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #14, Line 107:
output_texture_desc_use_shared_handle_ = true;Defer setup MiscFlag to createOutputTexture because in that time we know the proper flags. […]
Done
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #12, Line 115:
output_view
I think this has been addressed but not 100% sure.
Done
File media/gpu/windows/d3d11_picture_buffer.h:
Patch Set #12, Line 78:
GetOutputView
Chosse AcquireOutputView() as the placeholder, until I have a better idea.
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan, Rafael Cintron.
Patch set 16:Code-Review +1
1 comment:
Patchset:
media/ lgtm. thanks for all the work.
-fl
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Shaobo Yan, Rafael Cintron.
5 comments:
Patchset:
mostly looks good - a few minor nits and one important comment about interaction with decode swap chain support
File media/gpu/windows/d3d11_decoder_configurator.h:
bool supports_swap_chain_;
bool is_encrypted_;nit: const bool since these are only assigned in the constructor
File media/gpu/windows/d3d11_decoder_configurator.cc:
// Update the decoder output texture usage to support shared handle and
// keyed_mutex ifnit: looks like this comment was left incomplete?
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #16, Line 231:
!decoder_configurator_->SupportsSwapChain();note that this will disable shared handles on most Kaby Lake+ systems - we probably want to do the opposite i.e. disable decode swap chain support if shared handle is enabled
// When creating output texture with shared handle supports, we need to enable
// single texture to workaround hang issue. More info here:
// https://crbug.com/1238943nit: Can you mention that we can't use a texture array because the keyed mutex applies on the entire texture array causing a deadlock when multiple threads try to use different slots of the array?
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Attention is currently required from: Sunny Sachanandani, Rafael Cintron.
6 comments:
Patchset:
Sunny@ and Frank@, Thanks for your reviewing.
Sunny@ and Rafael@, would you mind to take another look? Thanks!
File media/gpu/windows/d3d11_decoder_configurator.h:
bool supports_swap_chain_;
bool is_encrypted_;nit: const bool since these are only assigned in the constructor
Done
File media/gpu/windows/d3d11_decoder_configurator.cc:
Patch Set #12, Line 114:
MiscFlagsI don't think there is a sceneario for keyed_mutexes + encrypted content. […]
Solution: disable use_shared_handle when the video frame is encrypted because it is not useful. Could enable it easily in future if we find exceptions.
File media/gpu/windows/d3d11_decoder_configurator.cc:
// Update the decoder output texture usage to support shared handle and
// keyed_mutex ifnit: looks like this comment was left incomplete?
Oops.. Thanks for catching this.
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #16, Line 231:
!decoder_configurator_->SupportsSwapChain();
note that this will disable shared handles on most Kaby Lake+ systems - we probably want to do the o […]
Thanks for the info! Disable the swapChain when we have use_shared_handle == true. But disable use_shared_handle when the video frame is encrypted because it is not useful.
// When creating output texture with shared handle supports, we need to enable
// single texture to workaround hang issue. More info here:
// https://crbug.com/1238943
nit: Can you mention that we can't use a texture array because the keyed mutex applies on the entire […]
Done
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Sunny Sachanandani, Shaobo Yan.
Patch set 17:Code-Review +1
3 comments:
Patchset:
LGTM provided comments are addressed
File media/gpu/windows/d3d11_av1_accelerator.cc:
media::Status error = std::move(result).error();
RecordFailure("AcquireOutputView", error.message(), error.code());Nit: As a potential future improvement, we should have an overload of RecordFailure which takes a string+media::Status and shims to the other version which pulls out the message and code.
File media/gpu/windows/d3d11_texture_wrapper.cc:
if (handle == nullptr) {
DLOG(ERROR) << "CreateSharedHandle returns nullptr ";
std::move(on_error_cb)
.Run(std::move(StatusCode::kCreateSharedHandleFailed));
}I think gracefully handling a nullptr handle on success is not necessary. If CreateSharedHandle succeeds, handle should contain a valid value. At the most, we should turn this into a DCHECK.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Sunny Sachanandani (Gerrit)
Attention is currently required from: Shaobo Yan.
Patch set 17:Code-Review +1
1 comment:
Patchset:
lgtm! Thanks for working on this!
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
3 comments:
Patchset:
Thanks all for your reviewing. Learned a lot from the CL!
File media/gpu/windows/d3d11_av1_accelerator.cc:
media::Status error = std::move(result).error();
RecordFailure("AcquireOutputView", error.message(), error.code());
Nit: As a potential future improvement, we should have an overload of RecordFailure which takes a st […]
Done.
File media/gpu/windows/d3d11_texture_wrapper.cc:
if (handle == nullptr) {
DLOG(ERROR) << "CreateSharedHandle returns nullptr ";
std::move(on_error_cb)
.Run(std::move(StatusCode::kCreateSharedHandleFailed));
}
I think gracefully handling a nullptr handle on success is not necessary. […]
Thanks for telling this. I tend to remove the extra check. Done.
To view, visit change 2973567. To unsubscribe, or for help writing mail filters, visit settings.
Shaobo Yan (Gerrit)
Patch set 18:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
17 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: media/gpu/windows/d3d11_vp9_accelerator.cc
Insertions: 5, Deletions: 0.
@@ -56,6 +56,11 @@
D3D11VP9Accelerator::~D3D11VP9Accelerator() {}
void D3D11VP9Accelerator::RecordFailure(const std::string& fail_type,
+ media::Status error) {
+ RecordFailure(fail_type, error.message(), error.code());
+}
+
+void D3D11VP9Accelerator::RecordFailure(const std::string& fail_type,
const std::string& reason,
StatusCode code) {
MEDIA_LOG(ERROR, media_log_)
```
```
The name of the file: media/gpu/windows/d3d11_texture_wrapper.cc
Insertions: 0, Deletions: 5.
@@ -239,11 +239,6 @@
return;
}
- if (handle == nullptr) {
- DLOG(ERROR) << "CreateSharedHandle returns nullptr ";
- std::move(on_error_cb)
- .Run(std::move(StatusCode::kCreateSharedHandleFailed));
- }
shared_handle.Set(handle);
}
}
```
```
The name of the file: media/gpu/windows/d3d11_av1_accelerator.cc
Insertions: 6, Deletions: 2.
@@ -397,6 +397,11 @@
D3D11AV1Accelerator::~D3D11AV1Accelerator() {}
void D3D11AV1Accelerator::RecordFailure(const std::string& fail_type,
+ media::Status error) {
+ RecordFailure(fail_type, error.message(), error.code());
+}
+
+void D3D11AV1Accelerator::RecordFailure(const std::string& fail_type,
const std::string& message,
StatusCode reason) {
MEDIA_LOG(ERROR, media_log_)
@@ -507,8 +512,7 @@
if (result.has_value()) {
output_view = std::move(result).value();
} else {
- media::Status error = std::move(result).error();
- RecordFailure("AcquireOutputView", error.message(), error.code());
+ RecordFailure("AcquireOutputView", std::move(result).error());
return DecodeStatus::kFail;
}
const auto hr = video_context_->DecoderBeginFrame(video_decoder_.Get(),
```
```
The name of the file: media/gpu/windows/d3d11_av1_accelerator.h
Insertions: 2, Deletions: 0.
@@ -49,6 +49,8 @@
bool SubmitDecoderBuffer(
const DXVA_PicParams_AV1& pic_params,
const libgav1::Vector<libgav1::TileBuffer>& tile_buffers);
+
+ void RecordFailure(const std::string& fail_type, media::Status error);
void RecordFailure(const std::string& fail_type,
const std::string& message,
StatusCode reason);
```
```
The name of the file: media/gpu/windows/d3d11_h264_accelerator.h
Insertions: 2, Deletions: 1.
@@ -13,7 +13,7 @@
#include <vector>
#include "gpu/command_buffer/service/texture_manager.h"
-#include "media/base/status_codes.h"
+#include "media/base/status.h"
#include "media/base/video_frame.h"
#include "media/base/win/mf_helpers.h"
#include "media/gpu/h264_decoder.h"
@@ -89,6 +89,7 @@
void RecordFailure(const std::string& reason,
StatusCode code,
HRESULT hr = S_OK) const;
+ void RecordFailure(media::Status error) const;
D3D11VideoDecoderClient* client_;
MediaLog* media_log_ = nullptr;
```
```
The name of the file: media/gpu/windows/d3d11_vp9_accelerator.h
Insertions: 1, Deletions: 0.
@@ -67,6 +67,7 @@
bool SubmitDecoderBuffer(const DXVA_PicParams_VP9& pic_params,
const D3D11VP9Picture& pic);
+ void RecordFailure(const std::string& fail_type, media::Status error);
void RecordFailure(const std::string& fail_type,
const std::string& reason,
StatusCode code);
```
```
The name of the file: media/gpu/windows/d3d11_h264_accelerator.cc
Insertions: 5, Deletions: 2.
@@ -115,8 +115,7 @@
if (result.has_value()) {
output_view = std::move(result).value();
} else {
- media::Status error = std::move(result).error();
- RecordFailure(error.message(), error.code());
+ RecordFailure(std::move(result).error());
return DecoderStatus::kFail;
}
@@ -626,6 +625,10 @@
base::UmaHistogramSparse("Media.D3D11.H264Status", static_cast<int>(code));
}
+void D3D11H264Accelerator::RecordFailure(media::Status error) const {
+ RecordFailure(error.message(), error.code());
+}
+
void D3D11H264Accelerator::SetVideoDecoder(ComD3D11VideoDecoder video_decoder) {
video_decoder_ = std::move(video_decoder);
}
```
Change d3d11 video decoder back resource to support key mutex
Current d3d11 video decoder works fine because all of the graphics
ops lives in the same native d3d11 context.
But WebGPU, which lives in different d3d context from d3d11 decoder,
requires the ability to share the frame to it.
This CL adds the ability for d3d11 video decoder and relies on the
truth that WebGPU and Decoder resource using a default protocol that
pick 0 as the key value to acquire and release the key mutex
BUG=dawn:576
Change-Id: Id91b83a4bd95c178ed9b59cdbe9455a5f161479b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2973567
Commit-Queue: Shaobo Yan <shaob...@intel.com>
Reviewed-by: Rafael Cintron <rafael....@microsoft.com>
Reviewed-by: Sunny Sachanandani <sun...@chromium.org>
Reviewed-by: Frank Liberato <libe...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#922490}
---
M media/base/media_switches.cc
M media/base/media_switches.h
M media/base/status_codes.h
M media/gpu/windows/d3d11_av1_accelerator.cc
M media/gpu/windows/d3d11_av1_accelerator.h
M media/gpu/windows/d3d11_copying_texture_wrapper.cc
M media/gpu/windows/d3d11_copying_texture_wrapper.h
M media/gpu/windows/d3d11_copying_texture_wrapper_unittest.cc
M media/gpu/windows/d3d11_decoder_configurator.cc
M media/gpu/windows/d3d11_decoder_configurator.h
M media/gpu/windows/d3d11_decoder_configurator_unittest.cc
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_h264_accelerator.h
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_texture_selector.cc
M media/gpu/windows/d3d11_texture_selector.h
M media/gpu/windows/d3d11_texture_wrapper.cc
M media/gpu/windows/d3d11_texture_wrapper.h
M media/gpu/windows/d3d11_video_decoder.cc
M media/gpu/windows/d3d11_vp9_accelerator.cc
M media/gpu/windows/d3d11_vp9_accelerator.h
22 files changed, 330 insertions(+), 63 deletions(-)