Create D3D12 video decoder [chromium/src : main]
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis.
Zhibo Wang would like Dale Curtis to review this change.
Create D3D12 video decoder
Since the rendering side don't accept a D3D12Resource now, the d3d12 video decoder will use D3D11Texture2D as picture buffer, open it in D3D12 by shared handle, do D3D12 video decoding and close the handle, then let the D3D11Texture2D follow the current rendering pipeline.
In this case, D3D12VideoDecoder inherits D3D11VideoDecoder and reuses most of its methods, only replacing the wrapper with a D3D12 one.
Bug: 1348104
Change-Id: I8d9b5274197195242aed320e9fb33a9746e8cf55
---
M media/base/decoder.cc
M media/base/decoder.h
M media/base/media_switches.cc
M media/base/media_switches.h
M media/gpu/BUILD.gn
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_h264_accelerator.h
M media/gpu/windows/d3d11_h265_accelerator.cc
M media/gpu/windows/d3d11_h265_accelerator.h
M media/gpu/windows/d3d11_video_decoder.cc
M media/gpu/windows/d3d11_video_decoder.h
M media/gpu/windows/d3d11_video_decoder_wrapper.cc
A media/gpu/windows/d3d12_video_decoder.cc
A media/gpu/windows/d3d12_video_decoder.h
A media/gpu/windows/d3d12_video_decoder_wrapper.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.h
M media/gpu/windows/d3d_video_decoder_wrapper.h
M media/gpu/windows/init_guid.cc
M media/mojo/mojom/stable/stable_video_decoder_types_mojom_traits.h
M media/mojo/services/BUILD.gn
M media/mojo/services/gpu_mojo_media_client_win.cc
21 files changed, 1,110 insertions(+), 120 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis.
1 comment:
Patchset:
Hello, this is our D3D12VideoDecoder implementation, which is now ready for review.
This patch demonstrates an overall change, and may be split into minor patches.Performance: While we are doing D3D11/12 interops, many D3D12 features cannot be fully utilized, and the performance is not as good as pure D3D11 pipeline (about 0.5x).
Question:
- When and how shall we create D3D12Device? Currently I create it in `media/mojo/services/gpu_mojo_media_client_win.cc` but Checks says
- ```
- Illegal import in chrome.dll: d3d12.dll
- ```
- https://chromium-review.googlesource.com/c/chromium/src/+/4410406?checksPatchset=32&checksRunsSelected=win-rel&tab=checks
- We also want to fallback to D3D11 when D3D12 fails, but we only have one PlatformVideoDecoder implementation. How should we do that?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
Dale Curtis would like Frank Liberato and Ted (Chromium) Meyer to review this change authored by Zhibo Wang.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
6 comments:
Patchset:
I think it'd be better if we just rename d3d11_video_decoder to d3d_video_decoder or just otherwise live with the name and let it support d3d12 as well. You probably need to parameterize / change the GetDevice callback then.
Then instead of having D3D12 inherit from D3D11, we'd move the D3D11 parts into something like `D3D11VideoDecoderCore` first and then you'd add a `D3D12VideoDecoderCore` after the fact.
@liberato @tmathemeyer do you have thoughts?
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #36, Line 203:
bit_depth_ = 0;You'll want to restore this code.
Patch Set #36, Line 437:
bool D3D11VideoDecoder::ReloadD3DVideoDecoder() {`(Recreate|Reset)DecoderAndWrapper` seems clearer.
File media/gpu/windows/d3d12_video_decoder.cc:
Patch Set #36, Line 27:
if (config.profile() == VP9PROFILE_PROFILE0) {switch statement?
Patch Set #36, Line 43:
} else if (config.profile() == HEVCPROFILE_REXT) {All these non-d3d12 ones seem common and could come from a shared function?
Patch Set #36, Line 69:
DXGI_FORMAT GetDXGIFormat(uint8_t bitdepth,This seems like it'd be common.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Dale Curtis, Ted (Chromium) Meyer, Zhibo Wang.
2 comments:
Patchset:
I think it'd be better if we just rename d3d11_video_decoder to d3d_video_decoder or just otherwise […]
that was also my thought. there are a several parts that need to be abstracted away based on 11 vs 12, but having 12 derive from 11 invites having lots of duplicate code (e.g., i commented one such section).
File media/gpu/windows/d3d12_video_decoder.cc:
Patch Set #36, Line 115:
bool D3D12VideoDecoder::ReloadD3DVideoDecoder() {this fn has a lot of duplicated code, or nearly so. i think that it can be split up a bit to combine the duplicate parts.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Ted (Chromium) Meyer.
7 comments:
Patchset:
The case is, since the rendering side don't support D3D12 yet, the D3D12 video decoder still use D3D11Texture2D as the output picture buffer for better memory management, as is recommended by Microsoft expert. https://bugs.chromium.org/p/chromium/issues/detail?id=1434145#c10
So in this CL(PS38 now)'s implementation, the D3D12VideoDecoder will
1. Use D3D11's CreatePictureBuffers logic to create and manage D3D11Texture2D's
2. Open one in D3D12VideoDecoderWrapper via a shared handle
3. Do D3D12 decode command and wait for decoding to be done
4. return to D3D11Texture2D and do the possible video processingApart from 3. being D3D12, the other parts are still identical to D3D11's logic. That is to say current D3D12 video decoding heavily uses D3D11 API.
Then instead of having D3D12 inherit from D3D11, we'd move the D3D11 parts into something like D3D11VideoDecoderCore first and then you'd add a D3D12VideoDecoderCore after the fact.
there are a several parts that need to be abstracted away based on 11 vs 12, but having 12 derive from 11 invites having lots of duplicate code
I don't fully get your point. Where should we put the shared D3D11 API calls while not doing inherit-derive?
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #36, Line 203:
bit_depth_ = 0;You'll want to restore this code.
Done
Patch Set #36, Line 437:
bool D3D11VideoDecoder::ReloadD3DVideoDecoder() {`(Recreate|Reset)DecoderAndWrapper` seems clearer.
Done
File media/gpu/windows/d3d12_video_decoder.cc:
Patch Set #36, Line 27:
if (config.profile() == VP9PROFILE_PROFILE0) {switch statement?
Done
Patch Set #36, Line 43:
} else if (config.profile() == HEVCPROFILE_REXT) {All these non-d3d12 ones seem common and could come from a shared function?
Done
Patch Set #36, Line 69:
DXGI_FORMAT GetDXGIFormat(uint8_t bitdepth,This seems like it'd be common.
Done
Patch Set #36, Line 115:
bool D3D12VideoDecoder::ReloadD3DVideoDecoder() {
this fn has a lot of duplicated code, or nearly so. […]
Reused the creation of `decoder_configurator_` and `texture_selector_`.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
Patchset:
The case is, since the rendering side don't support D3D12 yet, the D3D12 video decoder still use D3D […]
I think Frank and I are just suggesting that instead of creating a new decoder which inherits from D3D11VideoDecoder, you instead have a d3d11 "core" and a d3d12 "core" that the D3D11VideoDecoder (perhaps renamed to D3DVideoDecoder) can use depending on which d3d version level is being targeted.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
Patchset:
I think Frank and I are just suggesting that instead of creating a new decoder which inherits from D […]
E.g., in terms of CL steps:
1. First extract the d3d11 core as is in one CL.
2. Implement a D3d12 core in another CL.
3. Maybe rename D3D11VideoDecoder to D3DVideoDecoder?
4. Add code to D3DVideoDecoder to support multiple cores depending on version level.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Frank Liberato, Ted (Chromium) Meyer.
1 comment:
Patchset:
E.g., in terms of CL steps: […]
I realized d3d12_video_decoder.cc almost overridden nothing. After I move the argument preparation for creating ID3D1xVideoDecoder into _wrapper.cc, I managed to get rid of the d3d12_video_decoder.cc file.
I believe PS42 solve the issues, where there is no inheritance, we don't need 11/12 core since we have nothing to be polymorphic in d3d11_video_decoder.cc, and everything is shared until the creation of the wrapper, then we do #4 in D3D11VideoDecoder::CreateD3DVideoDecoderWrapper().
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
14 comments:
Patchset:
Thanks for reorganizing!
The number of classes in the wrapper impl is starting to get a bit too high. I think it'd probably be helpful to split these out into their own files with testing so that the wrappers aren't so large and untested.
File media/gpu/windows/d3d12_video_decoder_wrapper.h:
Patch Set #42, Line 20:
class D3D12VideoDecoderWrapper : public D3DVideoDecoderWrapper {Documentation.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #42, Line 30:
HANDLE handle,Pass and store `base::win::ScopedHandle` instead?
Patch Set #42, Line 59:
: heap_(heap) {}std::move?
Patch Set #42, Line 71:
if (index >= size_) {I think it's better to just have a single vector with a struct containing these resource elements that you can push_back into if you're going to have to `resize` every time?
Patch Set #42, Line 92:
std::vector<D3D12_RESOURCE_BARRIER> TransitionBarriersForAllPlanes(`CreateTransitionBarriersForAllPlanes`
Patch Set #42, Line 100:
for (UINT8 i = 0; i < num_planes; i++) {Document what's happening and why here.
Patch Set #42, Line 125:
device_(device),All these need std::move?
Patch Set #42, Line 153:
if (FAILED(hr)) {`RETURN_IF_FAILED` might clean up a lot of the boiler plate here.
Patch Set #42, Line 165:
if (output_stream_arguments_.pOutputTexture2D) {Document why.
Patch Set #42, Line 247:
.Size = compressed_bitstream_->GetDesc().Width,`Width` only seems a bit odd. Is that correct? A small comment if so.
Patch Set #42, Line 295:
std::unique_ptr<std::remove_pointer_t<HANDLE>, decltype(&CloseHandle)>base::win::SCopedHandle
Patch Set #42, Line 337:
class ScopedD3D12MemoryBuffer : public ScopedD3DBuffer {Move these above the impl so it's closer to the outer wrapper below.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #42, Line 38:
D3D11VideoDecoder::GetD3D12VideoDeviceCB GetD3D12DeviceCallback() {Can we just parameterize the existing one with a D3D11/D3D12 enum or boolean?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Ted (Chromium) Meyer, Zhibo Wang.
3 comments:
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #42, Line 197:
bool D3D11VideoDecoder::ResetD3DVideoDecoder() {why change the name?
Patch Set #42, Line 229:
NotifyError(D3D11StatusCode::kDecoderUnsupportedCodec);and `return false`.
File media/gpu/windows/d3d11_video_decoder_wrapper.h:
Patch Set #42, Line 26:
bool&i think that this would be 'bool*' for the chromium style guide, with some hint that it's an out parameter.
however, it might be better to put a getter on the decoder wrapper base class to return the value.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Frank Liberato, Ted (Chromium) Meyer.
18 comments:
Patchset:
I realized d3d12_video_decoder.cc almost overridden nothing. […]
Done
Patchset:
Thanks for reorganizing! […]
Done
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #42, Line 197:
bool D3D11VideoDecoder::ResetD3DVideoDecoder() {why change the name?
The creation of `ID3D11VideoDecoder` is moved to `CreateD3DVideoDecoderWrapper()`. So it does not do the things as its old name `CreateD3D11Decoder()` indicates. Note that the return type also changed.
The new method do re-creation of decoder_configurator_, texture_selector_, and create-and-set the wrapper. It is more like a reset to the `media::D3D11VideoDecoder` class.
Patch Set #42, Line 229:
NotifyError(D3D11StatusCode::kDecoderUnsupportedCodec);and `return false`.
Done
File media/gpu/windows/d3d11_video_decoder_wrapper.h:
Patch Set #42, Line 26:
bool&
i think that this would be 'bool*' for the chromium style guide, with some hint that it's an out par […]
Done
File media/gpu/windows/d3d12_video_decoder_wrapper.h:
Patch Set #42, Line 20:
class D3D12VideoDecoderWrapper : public D3DVideoDecoderWrapper {Documentation.
Done
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #42, Line 30:
HANDLE handle,Pass and store `base::win::ScopedHandle` instead?
Done
Patch Set #42, Line 59:
: heap_(heap) {}std::move?
Done
Patch Set #42, Line 71:
if (index >= size_) {I think it's better to just have a single vector with a struct containing these resource elements that
The vectors are for `ToD3D12VideoDecodeReferenceFrames()` returning the exact array pointers that D3D12 API expected without any conversion.
you can push_back into if you're going to have to resize every time?
The indices of the frames coming in are not guaranteed to be incremental. E.g. it would be 0,4,9,13,7,2 instead of 0,1,2,3,4,5,6.
Patch Set #42, Line 92:
std::vector<D3D12_RESOURCE_BARRIER> TransitionBarriersForAllPlanes(`CreateTransitionBarriersForAllPlanes`
Done
Patch Set #42, Line 100:
for (UINT8 i = 0; i < num_planes; i++) {Document what's happening and why here.
Done
Patch Set #42, Line 125:
device_(device),All these need std::move?
Done
Patch Set #42, Line 153:
if (FAILED(hr)) {`RETURN_IF_FAILED` might clean up a lot of the boiler plate here.
Done
Patch Set #42, Line 165:
if (output_stream_arguments_.pOutputTexture2D) {Document why.
Done
Patch Set #42, Line 247:
.Size = compressed_bitstream_->GetDesc().Width,`Width` only seems a bit odd. Is that correct? A small comment if so.
Done
Patch Set #42, Line 295:
std::unique_ptr<std::remove_pointer_t<HANDLE>, decltype(&CloseHandle)>base::win::SCopedHandle
Done
Patch Set #42, Line 337:
class ScopedD3D12MemoryBuffer : public ScopedD3DBuffer {Move these above the impl so it's closer to the outer wrapper below.
`ScopedD3D12MemoryBuffer::Commit()` will use `D3D12VideoDecoderWrapperImpl->input_stream_arguments_` so it must come after the impl class.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #42, Line 38:
D3D11VideoDecoder::GetD3D12VideoDeviceCB GetD3D12DeviceCallback() {Can we just parameterize the existing one with a D3D11/D3D12 enum or boolean?
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
8 comments:
File media/gpu/windows/d3d11_h265_accelerator.cc:
Patch Set #43, Line 556:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);Isn't what we append always smaller than this?
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #43, Line 59:
CHECK(SUCCEEDED(video_decoder_->GetCreationParameters(&desc, &config)));Will this always succeed? Seems like a GPU driver disconnect or similar could cause it to fail?
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #42, Line 71:
if (index >= size_) {> I think it's better to just have a single vector with a struct containing these resource elements […]
Acknowledged
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 18:
#define RETURN_IF_FAILED(message, status_code, hr) \Doesn't mf_helpers.h already have these macros?
Patch Set #43, Line 54:
UINT8 num_planes)`int num_planes`
Patch Set #43, Line 74:
WaitForDecodeComplete();Waiting for decode to complete during `Reset()` is a bit surprising, since it will affect seeking performance if we have to wait for completion.
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;I don't think it's safe to do this on the gpu main thread. Is there a callback interface we can use instead?
File media/gpu/windows/d3d_video_decoder_wrapper.h:
Patch Set #43, Line 55:
// Wait for the submitted decode request to complete.This probably isn't going to be okay on the gpu main thread.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Frank Liberato, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
Patchset:
You might consider landing the D3D11 pieces of this first and then the D3D12 pieces in a followup CL.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
Patchset:
me => cc
thanks
-fl
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Jianlin Qiu (Gerrit)
Attention is currently required from: Dale Curtis, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;I don't think it's safe to do this on the gpu main thread. […]
dalecurtis@, Seems this has to be synchronous. How about changing INFINITE timeout to a limited timeout, i.e, 500ms?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Dale Curtis would like Rafael Cintron to review this change authored by Zhibo Wang.
Create D3D12 video decoder
Since the rendering side don't accept a D3D12Resource now, the d3d12 video decoder will use D3D11Texture2D as picture buffer, open it in D3D12 by shared handle, do D3D12 video decoding and close the handle, then let the D3D11Texture2D follow the current rendering pipeline.
In this case, D3D12VideoDecoder inherits D3D11VideoDecoder and reuses most of its methods, only replacing the wrapper with a D3D12 one.
Bug: 1348104
Change-Id: I8d9b5274197195242aed320e9fb33a9746e8cf55
---
M media/base/media_switches.cc
M media/base/media_switches.h
M media/base/win/BUILD.gn
A media/base/win/d3d12_mocks.cc
A media/base/win/d3d12_mocks.h
M media/gpu/BUILD.gn
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_h264_accelerator.h
M media/gpu/windows/d3d11_h265_accelerator.cc
M media/gpu/windows/d3d11_h265_accelerator.h
M media/gpu/windows/d3d11_video_decoder.cc
M media/gpu/windows/d3d11_video_decoder.h
M media/gpu/windows/d3d11_video_decoder_unittest.cc
M media/gpu/windows/d3d11_video_decoder_wrapper.cc
M media/gpu/windows/d3d11_video_decoder_wrapper.h
M media/gpu/windows/d3d11_vp9_accelerator.cc
A media/gpu/windows/d3d12_helpers.cc
A media/gpu/windows/d3d12_helpers.h
A media/gpu/windows/d3d12_helpers_unittest.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.h
M media/gpu/windows/d3d_video_decoder_wrapper.h
M media/gpu/windows/init_guid.cc
M media/mojo/services/BUILD.gn
M media/mojo/services/gpu_mojo_media_client_win.cc
25 files changed, 1,343 insertions(+), 155 deletions(-)
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
dalecurtis@, Seems this has to be synchronous. […]
Is it possible to wait on this on a background thread and deliver a callback to the main thread when ready? Timeout of 500ms seems like it could lead to sporadic glitching which is always hard to debug. +rafael in case he has advice.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Jianlin Qiu (Gerrit)
Attention is currently required from: Dale Curtis, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
Is it possible to wait on this on a background thread and deliver a callback to the main thread when […]
Yes the event can be wait on another thread. This requires SubmitDecode() to be implemented as async API as well.
However, I believe as of today D3D11's DecoderEndFrame() does the same(though internally in driver) on main thread waiting for decode to finish, and you don't have a chance to make it async.
We may need to align d3d11 and d3d12 to both accept a oncecallback param for SubmitDecode() API, though D3D11 invokes it with null callback?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
Yes the event can be wait on another thread. […]
Ah, I didn't realize this was already happening in the driver; that's unfortunate. It would certainly be better to do an async wait that triggers the callback, but this CL is already fairly complicated, so I don't mind unifying that in a followup before we enable the d3d12 path.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #44 to this change.
Create D3D12 video decoder
Since the rendering side don't accept a D3D12Resource now, the d3d12 video decoder will use D3D11Texture2D as picture buffer, open it in D3D12 by shared handle, do D3D12 video decoding and close the handle, then let the D3D11Texture2D follow the current rendering pipeline.
In this case, D3D12VideoDecoder inherits D3D11VideoDecoder and reuses most of its methods, only replacing the wrapper with a D3D12 one.
Bug: 1348104
Change-Id: I8d9b5274197195242aed320e9fb33a9746e8cf55
---
M media/base/media_switches.cc
M media/base/media_switches.h
M media/base/win/BUILD.gn
A media/base/win/d3d12_mocks.cc
A media/base/win/d3d12_mocks.h
M media/gpu/BUILD.gn
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_h264_accelerator.h
M media/gpu/windows/d3d11_h265_accelerator.cc
M media/gpu/windows/d3d11_h265_accelerator.h
M media/gpu/windows/d3d11_status.h
M media/gpu/windows/d3d11_video_decoder.cc
M media/gpu/windows/d3d11_video_decoder.h
M media/gpu/windows/d3d11_video_decoder_unittest.cc
M media/gpu/windows/d3d11_video_decoder_wrapper.cc
M media/gpu/windows/d3d11_video_decoder_wrapper.h
M media/gpu/windows/d3d11_vp9_accelerator.cc
A media/gpu/windows/d3d12_helpers.cc
A media/gpu/windows/d3d12_helpers.h
A media/gpu/windows/d3d12_helpers_unittest.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.h
M media/gpu/windows/d3d_video_decoder_wrapper.h
M media/gpu/windows/init_guid.cc
M media/mojo/services/BUILD.gn
M media/mojo/services/gpu_mojo_media_client_win.cc
26 files changed, 1,363 insertions(+), 154 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
5 comments:
File media/gpu/windows/d3d11_h265_accelerator.cc:
Patch Set #43, Line 556:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);Isn't what we append always smaller than this?
Yes. In D3D12 we need to submit the whole bitstream buffer in one go. So we get the size of the stream at below and allocate a buffer of sufficient size (it is harder to get the exact size). In D3D11 this argument has no effect, the driver decides the buffer size, and the bitstream can be chopped.
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #43, Line 59:
CHECK(SUCCEEDED(video_decoder_->GetCreationParameters(&desc, &config)));Will this always succeed? Seems like a GPU driver disconnect or similar could cause it to fail?
Done
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 18:
#define RETURN_IF_FAILED(message, status_code, hr) \Doesn't mf_helpers. […]
mf-helpers version don't use `media_log_`. Do you mean we should reuse that or rename this?
Patch Set #43, Line 54:
UINT8 num_planes)`int num_planes`
Done
Patch Set #43, Line 74:
WaitForDecodeComplete();
since it will affect seeking performance
Could you point out at which position Reset() will be called and the performance is affected? My search only shows it is called in the constructor of H265Decoder where WaitForDecodeComplete() will do nothing, and in the WaitForFrameBegins() below.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
3 comments:
File media/gpu/windows/d3d11_h265_accelerator.cc:
Patch Set #43, Line 556:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);
Yes. In D3D12 we need to submit the whole bitstream buffer in one go. […]
Acknowledged
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 18:
#define RETURN_IF_FAILED(message, status_code, hr) \
mf-helpers version don't use `media_log_`. […]
Ah, right; what you have is fine.
Patch Set #43, Line 74:
WaitForDecodeComplete();
> since it will affect seeking performance […]
That call comes from https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/windows/d3d11_video_decoder.cc;l=713;drc=a6bdc8f2993883fc55eb9cb0945694299b056675 right?
Which is the operation we use to drop all pending decodes before initiating a seek; maybe we have to wait for them, but if there's a way to abort them or drop them without waiting that would be preferred here.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #45 to this change.
26 files changed, 1,364 insertions(+), 154 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 74:
WaitForDecodeComplete();That call comes from https://source.chromium. […]
I see. We may need to look deeper to make sure whether the waiting is necessary and when the buffer can be cleared.
Since we always do synchronized waiting in SubmitDecode(), we can assume the decode is always completed, in the current code (without the cross-thread wait). Removing this line for now.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
6 comments:
Patchset:
Rafael, did you want to weigh in on any of this?
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #45, Line 385:
current_frame_size_ = stream.size();If all we need is the frame size, should we just save it on the base class instead of making multiple implementations override this just to get it?
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #45, Line 299:
if (!video_decoder_wrapper) {This is going to leave us with "use_shared_handle_" if D3D12 fails to create. I think you shouldn't fallback to D3D11 for the purposes of experimenting. It makes it hard to disambiguate issues with the implementation.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #42, Line 337:
class ScopedD3D12MemoryBuffer : public ScopedD3DBuffer {`ScopedD3D12MemoryBuffer::Commit()` will use `D3D12VideoDecoderWrapperImpl->input_stream_arguments_` […]
Acknowledged
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 74:
WaitForDecodeComplete();
I see. […]
Acknowledged
File media/gpu/windows/d3d_video_decoder_wrapper.h:
Patch Set #43, Line 55:
// Wait for the submitted decode request to complete.
This probably isn't going to be okay on the gpu main thread.
Can we just remove this for now if nothing is calling it?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #46 to this change.
26 files changed, 1,346 insertions(+), 154 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
4 comments:
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #45, Line 385:
current_frame_size_ = stream.size();
If all we need is the frame size, should we just save it on the base class instead of making multipl […]
Do you mean merge the implementations for h264 and h265? No, they don't share a common base class. D3D11H264Accelerator inherits H264Decoder::H264Accelerator while D3D11H265Accelerator inherits H265Decoder::H265Accelerator.
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #45, Line 299:
if (!video_decoder_wrapper) {
This is going to leave us with "use_shared_handle_" if D3D12 fails to create. […]
Done
File media/gpu/windows/d3d_video_decoder_wrapper.h:
Patch Set #43, Line 55:
// Wait for the submitted decode request to complete.
Can we just remove this for now if nothing is calling it?
Done
Patch Set #43, Line 55:
// Wait for the submitted decode request to complete.
Can we just remove this for now if nothing is calling it?
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 46:Code-Review +1
1 comment:
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #45, Line 385:
current_frame_size_ = stream.size();
Do you mean merge the implementations for h264 and h265? No, they don't share a common base class. […]
Ah right. Nevermind
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #47 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Dale Curtis
The change is no longer submittable: Code-Review is unsatisfied now.
M media/mojo/services/gpu_mojo_media_client_win.cc
25 files changed, 1,353 insertions(+), 154 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 48:Code-Review +1
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 48:-Code-Review
The change is no longer submittable: Code-Owners and Code-Review are unsatisfied now.
1 comment:
Patchset:
"DelayloadsTest.ChromeDllDelayloadsCheck failed." looks legit actually.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #49 to this change.
Create D3D12 video decoder
Since the rendering side don't accept a D3D12Resource now, the d3d12 video decoder will use D3D11Texture2D as picture buffer, open it in D3D12 by shared handle, do D3D12 video decoding and close the handle, then let the D3D11Texture2D follow the current rendering pipeline.
In this case, D3D12VideoDecoder inherits D3D11VideoDecoder and reuses most of its methods, only replacing the wrapper with a D3D12 one.
Bug: 1348104
Change-Id: I8d9b5274197195242aed320e9fb33a9746e8cf55
---
M build/config/win/BUILD.gn
26 files changed, 1,354 insertions(+), 154 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Patch set 49:Auto-Submit +1Commit-Queue +1
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
1 comment:
Patchset:
"DelayloadsTest.ChromeDllDelayloadsCheck failed." looks legit actually.
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 49:Code-Review +1Commit-Queue +2
Dale Curtis (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 49:Commit-Queue +1
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;Ah, I didn't realize this was already happening in the driver; that's unfortunate. […]
Drive by comment: Unless I am missing something, we should not need to do this synchronous waiting. If you're trying to use D3D12 resources on the D3D11 implicit queue or vice versa, you can do GPU timeline waits on either queue with fences, which are available with both APIs.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Jianlin Qiu (Gerrit)
Attention is currently required from: Dale Curtis, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
Drive by comment: Unless I am missing something, we should not need to do this synchronous waiting. […]
rafael.cintron@ Could you elaborate more on this?
We're waiting for the submitted D3D12 decode command to finish execution on the HW queue. Common practice is to set a completion event on the fence, and wait on that event(that we're doing here). If there is a way to avoid that WaitForSingleObject() call, we'll be happy to experiment with it.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Arthur Eubanks, Rafael Cintron, Ted (Chromium) Meyer.
Zhibo Wang would like Arthur Eubanks to review this change.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Arthur Eubanks (Gerrit)
Attention is currently required from: Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
Patchset:
(removed myself as reviewer, please find somebody with more background on the build/config/win/BUILD.gn change)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Ted (Chromium) Meyer, Zhibo Wang.
17 comments:
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #49, Line 275:
CHECK(SUCCEEDED(d3d_device.As(&device)));To CHECK that a function succeeded, I prefer to use `CHECK_EQ(..., S_OK)`. This way, the logs will tell us what the return value ended up being on failure.
if (!ResetD3DVideoDecoder()) {
return;
}One problem with attempting to reset an existing object instead of creating a brand new one is the object can end up in a half baked state if initialization the second time around fails part way through. I see several places in the implementation of `ResetD3DVideoDecoder` where we early out on error, leaving the the class with a random assortment of old values and new values.
To make the reset pattern work, you must be militant about first clearing out all of the old state before creating the new state. When you do create the new state, you need to create it into local variables one by one and only commit them to member variables when the entire initialization process finishes.
If you can pull it off, a better pattern would be to re-create the whole object when state becomes invalid. That way, you don't have to obsess about which member variables could be valid at any given point when you add new code in the future.
Patch Set #49, Line 1097:
CHECK(SUCCEEDED(d3d_device.As(&d3d11_device)));Similarly here, please do `CHECK_EQ(..., S_OK)` so we can see failure HRESULTs in log files.
File media/gpu/windows/d3d12_helpers.h:
std::vector<ID3D12Resource*> resources_;
std::vector<UINT> subresources_;
std::vector<ID3D12VideoDecoderHeap*> heaps_;I worry about storing raw pointers in these members. Who is meant to own these and how sure are we that the caller will not free them out from under this class? Any objections to making these be smart pointers just in case?
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #49, Line 14:
deviceNit: Since the constructor does not take ownership of `device`, best to pass `ID3D12Device` by raw pointer so we avoid unnecessary AddRef/Release.
Patch Set #49, Line 17:
handle_(handle)Strongly prefer passing `handle` via `ScopedHandle` so that you can `std::move` it to the member variable. Makes the ownership transfer more clear and you don't end up with bugs where you close the handle out from under the caller, leak the handle or double close it. All of these lead to difficult to track down issues.
HRESULT hr =
device->OpenSharedHandle(handle_.get(), IID_PPV_ARGS(&resource_));Consider having a static `Create` method which calls `OpenSharedHandle` and, only if it succeeds, creates the class with the object. No objects will half baked state if we do it that way.
If you pass `SharedResourceD3D12` by value and `std::move` it in the caller, you do not need the double `&&`.
Patch Set #49, Line 64:
barriersTo avoid unnecessary re-allocation of vectors, reserve the correct number of entries in `barriers` ahead or time. If the number of planes is always known, use `std::array` to avoid heap allocation altogether.
subresource + i * desc.MipLevels *
desc.DepthOrArraySize,Nit: Use `D3D12CalcSubresource` helper instead of doing this math yourself.
https://learn.microsoft.com/en-us/windows/win32/direct3d12/d3d12calcsubresource.
Patch Set #49, Line 122:
EnableDebugLayerIf two components in the process call `EnableDebugLayer`, the device will become removed out from under the other one. This will bite you in the future when those other components (Dawn, WebNN, etc) start to get turned on by default.
I recommend not doing this unconditionally and, instead, use DXCPL.exe to enable the debug layer locally. The D3D12 debug layer can really slow things down compared to the D3D11 one so it might not be a good idea to enable unconditionally in debug builds for non-graphics people on the team.
Patch Set #49, Line 131:
D3D_FEATURE_LEVEL_12_0D3D12 supports feature levels starting with 11_0. Any reason we can't start with 11_0 instead of 12? If not, please add a comment explaining why.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
rafael.cintron@ Could you elaborate more on this? […]
@Jianlin, is the goal of this work to use the decoded bits on the CPU? If so, your code is fine.
I presume that you want to display the decoded bits with D3D11 or D3D12? If so, then waiting for the completion event on the CPU ties up the CPU thread until the GPU work completes. A better approach would be to create fences on the D3D11 and D3D12 side, signal the fence on the D3D12 command queue and instruct the D3D11 command queue to wait on the fence. With this approach, no CPU thread waits for work to complete, it's all taken care of by the GPU scheduler.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
HANDLE handle;
hr = dxgi_resource->CreateSharedHandle(nullptr, GENERIC_ALL, nullptr,
&handle);Creating a share handle can be expensive since it makes a kernel transition. I know that you cache it in `SharedD3D12Resource` which is cached in `reference_frame_list_` but I don't see code which reduces the size of `reference_frame_list_`. Is that going to appear in a subsequent change? How does caching fit in the overall design of your solution?
std::vector<uint8_t> picture_parameters_buffer;
std::vector<uint8_t> inverse_quantization_matrix_buffer;
std::vector<uint8_t> slice_control_buffer;Nit: Please append `_` to all member variables.
Patch Set #49, Line 284:
resourceNit: Please use `std::move` to avoid unnecessary AddRef/Release
media_log, device, command_queue, command_allocator, fence, video_device,
video_decoder, video_decoder_heap, command_list, format_info.PlaneCount);Nit: Please `std::move` all of the smartpointers into the constructor arguments.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #49, Line 17:
handle_(handle)Strongly prefer passing `handle` via `ScopedHandle` so that you can `std::move` it to the member var […]
Yes +1, I thought I had caught all these, sorry I missed some.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #50 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Dale Curtis
Create D3D12 video decoder
Since the rendering side don't accept a D3D12Resource now, the d3d12 video decoder will use D3D11Texture2D as picture buffer, open it in D3D12 by shared handle, do D3D12 video decoding and close the handle, then let the D3D11Texture2D follow the current rendering pipeline.
In this case, D3D12VideoDecoder inherits D3D11VideoDecoder and reuses most of its methods, only replacing the wrapper with a D3D12 one.
Bug: 1348104
Change-Id: I8d9b5274197195242aed320e9fb33a9746e8cf55
---
M build/config/win/BUILD.gn
M media/base/media_switches.cc
M media/base/media_switches.h
M media/base/win/BUILD.gn
A media/base/win/d3d12_mocks.cc
A media/base/win/d3d12_mocks.h
M media/gpu/BUILD.gn
M media/gpu/windows/d3d11_h264_accelerator.cc
M media/gpu/windows/d3d11_h264_accelerator.h
M media/gpu/windows/d3d11_h265_accelerator.cc
M media/gpu/windows/d3d11_h265_accelerator.h
M media/gpu/windows/d3d11_picture_buffer.cc
M media/gpu/windows/d3d11_picture_buffer.h
M media/gpu/windows/d3d11_status.h
M media/gpu/windows/d3d11_video_decoder.cc
M media/gpu/windows/d3d11_video_decoder.h
M media/gpu/windows/d3d11_video_decoder_unittest.cc
M media/gpu/windows/d3d11_video_decoder_wrapper.cc
M media/gpu/windows/d3d11_video_decoder_wrapper.h
M media/gpu/windows/d3d11_vp9_accelerator.cc
A media/gpu/windows/d3d12_helpers.cc
A media/gpu/windows/d3d12_helpers.h
A media/gpu/windows/d3d12_helpers_unittest.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.cc
A media/gpu/windows/d3d12_video_decoder_wrapper.h
M media/gpu/windows/d3d_video_decoder_wrapper.h
M media/gpu/windows/init_guid.cc
M media/mojo/services/gpu_mojo_media_client_win.cc
28 files changed, 1,411 insertions(+), 164 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Zhibo Wang would like Bruce Dawson to review this change.
Create D3D12 video decoder
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Patch Set #49, Line 275:
CHECK(SUCCEEDED(d3d_device.As(&device)));
To CHECK that a function succeeded, I prefer to use `CHECK_EQ(..., S_OK)`. […]
Done
if (!ResetD3DVideoDecoder()) {
return;
}
create it into local variables one by one and only commit them to member variables when the entire initialization process finishes.
Done.
Patch Set #49, Line 1097:
CHECK(SUCCEEDED(d3d_device.As(&d3d11_device)));Similarly here, please do `CHECK_EQ(..., S_OK)` so we can see failure HRESULTs in log files.
Done
File media/gpu/windows/d3d12_helpers.h:
std::vector<ID3D12Resource*> resources_;
std::vector<UINT> subresources_;
std::vector<ID3D12VideoDecoderHeap*> heaps_;
I worry about storing raw pointers in these members. […]
Their life cycle are now(PS50) managed by D3D11PictureBuffer. If the underlying D3D11PictureBuffer is deleted, the such reference index won't be used by the decoder.
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #49, Line 14:
deviceNit: Since the constructor does not take ownership of `device`, best to pass `ID3D12Device` by raw p […]
Done
Patch Set #49, Line 17:
handle_(handle)
Yes +1, I thought I had caught all these, sorry I missed some.
Done
HRESULT hr =
device->OpenSharedHandle(handle_.get(), IID_PPV_ARGS(&resource_));
Consider having a static `Create` method which calls `OpenSharedHandle` and, only if it succeeds, cr […]
Done
If you pass `SharedResourceD3D12` by value and `std::move` it in the caller, you do not need the dou […]
Done
Patch Set #49, Line 64:
barriers
To avoid unnecessary re-allocation of vectors, reserve the correct number of entries in `barriers` a […]
Done
subresource + i * desc.MipLevels *
desc.DepthOrArraySize,
Nit: Use `D3D12CalcSubresource` helper instead of doing this math yourself. […]
D3D12CalcSubresource is defined in `d3dx12.h`. We don't have it yet.
@dalec...@chromium.org Shall we include `d3dx12.h`? IIUC it is not in the SDK so we may need to add a header file in the source tree.
Patch Set #49, Line 122:
EnableDebugLayer
If two components in the process call `EnableDebugLayer`, the device will become removed out from un […]
Done
Patch Set #49, Line 131:
D3D_FEATURE_LEVEL_12_0
D3D12 supports feature levels starting with 11_0. […]
Done. I didn't realize that.
> D3D12 supports feature levels starting with 11_0.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
@Jianlin, is the goal of this work to use the decoded bits on the CPU? If so, your code is fine. […]
I see. Let me do it in a followup CL.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
HANDLE handle;
hr = dxgi_resource->CreateSharedHandle(nullptr, GENERIC_ALL, nullptr,
&handle);Creating a share handle can be expensive since it makes a kernel transition.
PS50 makes SharedD3D12Resource live with D3D11PictureBuffer, meaning creating shared handle will only be called once after each picture buffer is created.
but I don't see code which reduces the size of `reference_frame_list_`.
The items in `reference_frame_list_` will be replaced by pictures in the following gop, the size won't exceed a small fixed number which I guess is 16.
std::vector<uint8_t> picture_parameters_buffer;
std::vector<uint8_t> inverse_quantization_matrix_buffer;
std::vector<uint8_t> slice_control_buffer;Nit: Please append `_` to all member variables.
Done
Patch Set #49, Line 284:
resourceNit: Please use `std::move` to avoid unnecessary AddRef/Release
Done
media_log, device, command_queue, command_allocator, fence, video_device,
video_decoder, video_decoder_heap, command_list, format_info.PlaneCount);Nit: Please `std::move` all of the smartpointers into the constructor arguments.
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Zhibo Wang uploaded patch set #51 to this change.
Create D3D12 video decoder
Since the rendering side don't accept a D3D12Resource now, the d3d12 video decoder will use D3D11Texture2D as picture buffer, open it in D3D12 by shared handle, do D3D12 video decoding and close the handle, then let the D3D11Texture2D follow the current rendering pipeline.
In this case, the D3D12 video decoder backend reuses most of
media::D3D11VideoDecoder's methods, only replacing the wrapper with a
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Zhibo Wang uploaded patch set #52 to this change.
Bruce Dawson (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 52:Code-Review +1
2 comments:
Patchset:
build/config/win/BUILD.gn lgtm
Commit Message:
Patch Set #52, Line 9:
Since the rendering side don't accept a D3D12Resource now, the d3d12don't -> won't
Also, what rendering side are you talking about?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
Patchset:
Will stamp after Rafael is happy.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Patch set 52:Auto-Submit +1
2 comments:
Patchset:
Will stamp after Rafael is happy.
Dale, this is question is for you.
https://chromium-review.googlesource.com/c/chromium/src/+/4410406/comment/c365c45f_a2ff35a8/
Commit Message:
Patch Set #52, Line 9:
Since the rendering side don't accept a D3D12Resource now, the d3d12
don't -> won't […]
https://bugs.chromium.org/p/chromium/issues/detail?id=1434145 may be informative.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer.
Since the rendering side don't accept a D3D12Resource now, the d3d12
video decoder will use D3D11Texture2D as picture buffer, open it in
D3D12 by shared handle, do D3D12 video decoding and close the handle,
then let the D3D11Texture2D follow the current rendering pipeline.
In this case, the D3D12 video decoder backend reuses most of
media::D3D11VideoDecoder's methods, only replacing the wrapper with a
D3D12 one.
Bug: 1348104, 1434145
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Bruce Dawson, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
1 comment:
File media/gpu/windows/d3d12_helpers.cc:
subresource + i * desc.MipLevels *
desc.DepthOrArraySize,
D3D12CalcSubresource is defined in `d3dx12.h`. We don't have it yet. […]
Hmm, @bruce...@chromium.org -- I would have thought we'd already have that SDK somewhere in the tree.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Bruce Dawson (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Rafael Cintron, Ted (Chromium) Meyer, Zhibo Wang.
Patch set 53:Code-Review +1
1 comment:
File media/gpu/windows/d3d12_helpers.cc:
subresource + i * desc.MipLevels *
desc.DepthOrArraySize,
Hmm, @brucedawson@chromium. […]
We have the latest Windows SDK (22H2) but apparently that doesn't come with d3dx12.h. That is, we have d3d12.h, but not d3dx12.h. I think all of the d3dx*.h files come from other places, like this:
https://github.com/microsoft/DirectX-Headers/blob/main/include/directx/d3dx12.h
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Bruce Dawson, Dale Curtis, Jianlin Qiu, Ted (Chromium) Meyer, Zhibo Wang.
13 comments:
Patchset:
I am not quite finished with my review and will not be in the office next week. But here is what I have so far.
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #52, Line 350:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);All of the other code I see which calls `GetBitStreamBuffer` uses the return value for something. Do we need to call it here? If so, why?
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #52, Line 104:
Microsoft::WRL::ComPtr<ID3D12Device> deviceSince `ToSharedD3D12Resource` does not take ownership of the SmartPointer, would be better to pass a raw interface pointer. Avoids unnecessary AddRef/Release.
File media/gpu/windows/d3d11_picture_buffer.cc:
shared_d3d12_resource_ =
SharedD3D12Resource::Create(device.Get(), std::move(handle_holder));If `SharedD3D12Resource::Create` fails, it will return null here. We'll want to propagate the "status" out of the factory method.
Since we don't need to keep the handle around, consider just storing the ID3D12Resource in the object itself. Less memory consumption than storing a pointer to a SharedD3D12Resource and, in that object, a pointer to the D3D12 resource.
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #52, Line 56:
bool GetSingleTextureRecommended(bool* is_recommended)Militant Chromium reviewers would prefer having `GetSingleTextureRecommended` return `std::optional<bool>`. Less potential for developers to ignore the return value or forget to assign `is_recommended`.
Patch Set #52, Line 323:
D3D11DecoderConfigurator*Can `decoder_configurator` be a const reference?
File media/gpu/windows/d3d12_helpers.h:
Patch Set #53, Line 45:
base::win::ScopedHandle handle_;Since you're not going to use the handle for anything other than creating the resource, you can simply keep the ID3D12Resource reference alive. Saves an entry in the process handle table.
absl::InlinedVector<ID3D12Resource*, kMaxReferenceFrameListSize> resources_;
absl::InlinedVector<UINT, kMaxReferenceFrameListSize> subresources_;
absl::InlinedVector<ID3D12VideoDecoderHeap*, kMaxReferenceFrameListSize>
heaps_;If these cannot exceed `kMaxReferenceFrameListSize`, then `std::array` would be a simpler container to use.
File media/gpu/windows/d3d12_helpers.cc:
subresource + i * desc.MipLevels *
desc.DepthOrArraySize,
We have the latest Windows SDK (22H2) but apparently that doesn't come with d3dx12.h. […]
I didn't realize using this helper would involve taking these dependencies. Calculating subresource numbers is not complicated so probably fine to have our own helper for Chromium.
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #53, Line 122:
GetD3D12DevicePlease rename to `CreateD3D12Device`.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #53, Line 85:
WaitForFrameBeginsUnless I am missing something, I not see any "waiting" in this method. Why is it called `WaitForFrameBegins`?
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue;
D3D12_COMMAND_QUEUE_DESC command_queue_desc{
D3D12_COMMAND_LIST_TYPE_VIDEO_DECODE};
hr = device->CreateCommandQueue(&command_queue_desc,
IID_PPV_ARGS(&command_queue));
RETURN_IF_FAILED2(hr, "D3D12Device CreateCommandQueue failed");Creating a command queue for every single video can be expensive. You'll typically want one per adapter. Can we share these between videos as an optimization down the road? Command queues are thread safe in D3D12 so you can submit work to them from multiple threads with no crashes.
Patch Set #53, Line 434:
CHECK_EQ(command_list->Close(), S_OK);Why are closing a command list you just created?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Jianlin Qiu, Ted (Chromium) Meyer, Zhibo Wang.
Zhibo Wang uploaded patch set #54 to this change.
28 files changed, 1,431 insertions(+), 163 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Bruce Dawson, Jianlin Qiu.
Zhibo Wang has uploaded this change for review.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Patch set 54:Auto-Submit +1
12 comments:
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #52, Line 350:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);
All of the other code I see which calls `GetBitStreamBuffer` uses the return value for something. […]
`GetBitstreamBuffer()` will create the buffer if it does not exist with the given size and return the buffer. Calling it here is to make sure it creates one with a large enough size `current_frame_size_`, since
- D3D12 video decoder don't accept chopped bitstream buffer, so we need to reserve the buffer with the size large enough to contain the whole frame, and
- the following calls are in the base class who don't know this size.
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #52, Line 104:
Microsoft::WRL::ComPtr<ID3D12Device> device
Since `ToSharedD3D12Resource` does not take ownership of the SmartPointer, would be better to pass a […]
Done
File media/gpu/windows/d3d11_picture_buffer.cc:
shared_d3d12_resource_ =
SharedD3D12Resource::Create(device.Get(), std::move(handle_holder));If SharedD3D12Resource::Create fails, it will return null here. We'll want to propagate the "status" out of the factory method.
Yes I missed something here.
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #52, Line 56:
bool GetSingleTextureRecommended(bool* is_recommended)
Militant Chromium reviewers would prefer having `GetSingleTextureRecommended` return `std::optional< […]
Done. But still we may get confused that bool(return_value) is whether there is a value instead of the value is true, especially when we use auto.
Patch Set #52, Line 323:
D3D11DecoderConfigurator*Can `decoder_configurator` be a const reference?
Adding `const`, but I don't see reference(instead of pointer) is commonly used in the code around.
File media/gpu/windows/d3d12_helpers.h:
Patch Set #53, Line 45:
base::win::ScopedHandle handle_;
Since you're not going to use the handle for anything other than creating the resource, you can simp […]
My question is when the ID3D12Resource is still alive, can the handle that creates the resource be closed? SharedD3D12Resource here is to make sure the handle get closed after the resource is released.
absl::InlinedVector<ID3D12Resource*, kMaxReferenceFrameListSize> resources_;
absl::InlinedVector<UINT, kMaxReferenceFrameListSize> subresources_;
absl::InlinedVector<ID3D12VideoDecoderHeap*, kMaxReferenceFrameListSize>
heaps_;
If these cannot exceed `kMaxReferenceFrameListSize`, then `std::array` would be a simpler container […]
Done
File media/gpu/windows/d3d12_helpers.cc:
subresource + i * desc.MipLevels *
desc.DepthOrArraySize,
I didn't realize using this helper would involve taking these dependencies. […]
Done
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #53, Line 122:
GetD3D12DevicePlease rename to `CreateD3D12Device`.
Done
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #53, Line 85:
WaitForFrameBegins
Unless I am missing something, I not see any "waiting" in this method. […]
This is abstracted interface. In D3D11, we call `DecoderBeginFrame()` in loop until it is ready.
Microsoft::WRL::ComPtr<ID3D12CommandQueue> command_queue;
D3D12_COMMAND_QUEUE_DESC command_queue_desc{
D3D12_COMMAND_LIST_TYPE_VIDEO_DECODE};
hr = device->CreateCommandQueue(&command_queue_desc,
IID_PPV_ARGS(&command_queue));
RETURN_IF_FAILED2(hr, "D3D12Device CreateCommandQueue failed");
down the road
Yes of course. Adding a TODO here.
Patch Set #53, Line 434:
CHECK_EQ(command_list->Close(), S_OK);Why are closing a command list you just created?
I am referring to
https://github.com/microsoft/DirectX-Graphics-Samples/blob/master/Samples/Desktop/D3D12HelloWorld/src/HelloWindow/D3D12HelloWindow.cpp#L138-L143,
see the comments in the context.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
1 comment:
Patchset:
I am not quite finished with my review and will not be in the office next week. […]
Friendly ping in case you missed it. ptal.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
2 comments:
Patchset:
@Zhibo, thank you very much for your patience. Will review today/tomorrow
File media/gpu/windows/d3d12_helpers.h:
Patch Set #53, Line 45:
base::win::ScopedHandle handle_;
My question is when the ID3D12Resource is still alive, can the handle that creates the resource be c […]
The handle and ID3D12Resource pointers both keep a strong reference to the texture. Close/Releasing one does not affect the reference count of the other one.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron, Zhibo Wang.
Zhibo Wang uploaded patch set #55 to this change.
28 files changed, 1,410 insertions(+), 164 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Patch set 55:Auto-Submit +1
2 comments:
Patchset:
@Zhibo, thank you very much for your patience. […]
Good to hear from you. Thanks for taking time reviewing this XL patch.
File media/gpu/windows/d3d12_helpers.h:
Patch Set #53, Line 45:
base::win::ScopedHandle handle_;
The handle and ID3D12Resource pointers both keep a strong reference to the texture. […]
I see. In that case I don't even need the SharedD3D12Resource class. Removing it in PS55.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
11 comments:
Patchset:
@Zhibo, thank you for your patience. Here is another round of feedback.
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #55, Line 107:
HRESULT hr = texture_.As(&dxgi_resource);Nit: We can `CHECK_EQ(..., S_OK)` QIs from `ID3D11Texture2D` to `IDXGIResource1`.
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #55, Line 22:
ToD3D12VideoDecodeReferenceFramesNit: Can `ToD3D12VideoDecodeReferenceFrames` be const?
Patch Set #55, Line 54:
SUCCEEDED(temp->GetDesc(&desc)Nit: Should be save to `CHECK_EQ(..., S_OK)` for calls to `GetDesc`.
Patch Set #55, Line 57:
adapter = temp;Nit: `adapter = std::move(temp)`
Patch Set #55, Line 84:
resourceNit: Since `CreateD3D12TransitionBarriersForAllPlanes` does not take ownership of the resource, you can save an unnecessary AddRef/Release by just passing as a raw pointer.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #53, Line 434:
CHECK_EQ(command_list->Close(), S_OK);I am referring to […]
The comment states that the main loop of the sample app expects the command list to be closed.
Is that also true for Chromium?
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #55, Line 207:
CreateEventEx`CreateEventEx` seems overkill for what you want to do. You can simply do `::CreateEvent(nullptr, /*bManualReset*/ TRUE, /*bInitialState*/ FALSE, nullptr)`.
Patch Set #55, Line 273:
raw_ptrFor pointers you're assigning in the constructor, favor `raw_ref`.
Patch Set #55, Line 319:
picture_parameters_buffer_If these three data buffers (`picture_parameters_buffer_`, `inverse_quantization_matrix_buffer_` and `slice_control_buffer_`) are mutually exclusive with one another, consider just having one `std::vector` in the class and call it `data_buffer_` or something similar.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)I see that the d3d11 device is nice and cached. Can we do the same for the D3D12 one?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Zhibo Wang.
Zhibo Wang uploaded patch set #56 to this change.
28 files changed, 1,411 insertions(+), 164 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Patch set 56:Auto-Submit +1
10 comments:
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #55, Line 107:
HRESULT hr = texture_.As(&dxgi_resource);Nit: We can `CHECK_EQ(..., S_OK)` QIs from `ID3D11Texture2D` to `IDXGIResource1`.
Done
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #55, Line 22:
ToD3D12VideoDecodeReferenceFramesNit: Can `ToD3D12VideoDecodeReferenceFrames` be const?
Got compile error:
```
../../media/gpu/windows/d3d12_helpers.cc(25,23): error: cannot initialize a member subobject of type 'ID3D12Resource **' with an rvalue of type 'const value_type *' (aka 'ID3D12Resource *const *')
25 | .ppTexture2Ds = resources_.data(),
| ^~~~~~~~~~~~~~~~~
../../media/gpu/windows/d3d12_helpers.cc(26,24): error: cannot initialize a member subobject of type 'UINT *' (aka 'unsigned int *') with an rvalue of type 'const value_type *' (aka 'const unsigned int *')
26 | .pSubresources = subresources_.data(),
| ^~~~~~~~~~~~~~~~~~~~
../../media/gpu/windows/d3d12_helpers.cc(27,18): error: cannot initialize a member subobject of type 'ID3D12VideoDecoderHeap **' with an rvalue of type 'const value_type *' (aka 'ID3D12VideoDecoderHeap *const *')
27 | .ppHeaps = heaps_.data(),
| ^~~~~~~~~~~~~
```
Patch Set #55, Line 54:
SUCCEEDED(temp->GetDesc(&desc)Nit: Should be save to `CHECK_EQ(..., S_OK)` for calls to `GetDesc`.
Done
Patch Set #55, Line 57:
adapter = temp;Nit: `adapter = std::move(temp)`
Done
Patch Set #55, Line 84:
resource
Nit: Since `CreateD3D12TransitionBarriersForAllPlanes` does not take ownership of the resource, you […]
Done
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #53, Line 434:
CHECK_EQ(command_list->Close(), S_OK);
The comment states that the main loop of the sample app expects the command list to be closed. […]
Yes, in the implementation I also do reset-record-close in the loop, which expects the command list to be initially closed.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #55, Line 207:
CreateEventEx
`CreateEventEx` seems overkill for what you want to do. […]
Done
Patch Set #55, Line 273:
raw_ptrFor pointers you're assigning in the constructor, favor `raw_ref`.
Done
Patch Set #55, Line 319:
picture_parameters_buffer_
If these three data buffers (`picture_parameters_buffer_`, `inverse_quantization_matrix_buffer_` and […]
No, at least two (possible all three of them) are used at the same time.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
I see that the d3d11 device is nice and cached. […]
Now cached inside CreateD3D12Device().
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Dale Curtis, Jianlin Qiu, Zhibo Wang.
13 comments:
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #52, Line 350:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);
`GetBitstreamBuffer()` will create the buffer if it does not exist with the given size and return th […]
Thank you for the explanation. Please include your reply in the comments for the code.
File media/gpu/windows/d3d11_video_decoder.h:
#include <d3d12.h>
#include <d3d12video.h>Since you're not using any D3D12 types, you should be able to remove these D3D12-specific includes.
File media/gpu/windows/d3d11_video_decoder.cc:
// Note that we assume that this is the ANGLE device, since we don't implement
// texture sharing properly. That also implies that this is the GPU main
// thread, since we use non-threadsafe properties of the device (e.g., we get
// the immediate context).
//
// Also note that we don't technically have a guarantee that the ANGLE device
// will use the most recent version of D3D11; it might be configured to use
// D3D9. In practice, though, it seems to use 11.1 if it's available, unless
// it's been specifically configured via switch to avoid d3d11.These comments could use updating now that we're sometimes using D3D12 for video decoding.
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #52, Line 323:
D3D11DecoderConfigurator*
Adding `const`, but I don't see reference(instead of pointer) is commonly used in the code around.
https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs says to prefer const references to const pointers for non-optional input parameters. But if the media code prefers pointers, I'm fine with that. You made the parameter const, which is the most important thing.
File media/gpu/windows/d3d12_helpers.h:
Patch Set #56, Line 16:
#include "base/win/scoped_handle.h"scoped_handle.h doesn't appear to be used in the header file, please move it to the cc file.
Patch Set #56, Line 23:
inlined_vector.h"inlined_vector.h doesn't appear to be used in the header file, please move it to the cc file.
File media/gpu/windows/d3d12_helpers.h:
std::vector<ID3D12Resource*> resources_;
std::vector<UINT> subresources_;
std::vector<ID3D12VideoDecoderHeap*> heaps_;
Their life cycle are now(PS50) managed by D3D11PictureBuffer. […]
Acknowledged
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #55, Line 22:
ToD3D12VideoDecodeReferenceFrames
Got compile error: […]
I think using `const_cast` here is warranted.
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #56, Line 49:
luid != CHROME_LUID{}We've had at least a couple of scenarios where two calls to `EnumAdapters` return different default adapters on multi-adapter systems due to race conditions. These result in hard to repro bugs for end users.
Unless there is a good reason not to, please force the caller to pass a LUID here. For test code, use `CHECK_IS_TEST` (with a comment) to only allow test code to pass a null LUID.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #43, Line 218:
return WaitForSingleObject(fence_event_.get(), INFINITE) == WAIT_OBJECT_0;
I see. Let me do it in a followup CL.
Acknowledged
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
HANDLE handle;
hr = dxgi_resource->CreateSharedHandle(nullptr, GENERIC_ALL, nullptr,
&handle);
> Creating a share handle can be expensive since it makes a kernel transition. […]
Acknowledged
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #53, Line 85:
WaitForFrameBegins
This is abstracted interface. In D3D11, we call `DecoderBeginFrame()` in loop until it is ready.
Acknowledged
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
Now cached inside CreateD3D12Device().
Caching them as static variables like you've done in the latest update is not thread safe.
Is there a reason we can't store/cache the D3D12 device in the same place we do the D3D11 one?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Jianlin Qiu, Zhibo Wang.
Zhibo Wang uploaded patch set #57 to this change.
28 files changed, 1,423 insertions(+), 164 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Patch set 57:Auto-Submit +1
9 comments:
File media/gpu/windows/d3d11_h264_accelerator.cc:
Patch Set #52, Line 350:
video_decoder_wrapper_->GetBitstreamBuffer(current_frame_size_);
Thank you for the explanation. Please include your reply in the comments for the code.
Done
File media/gpu/windows/d3d11_video_decoder.h:
#include <d3d12.h>
#include <d3d12video.h>Since you're not using any D3D12 types, you should be able to remove these D3D12-specific includes.
Done
File media/gpu/windows/d3d11_video_decoder.cc:
// Note that we assume that this is the ANGLE device, since we don't implement
// texture sharing properly. That also implies that this is the GPU main
// thread, since we use non-threadsafe properties of the device (e.g., we get
// the immediate context).
//
// Also note that we don't technically have a guarantee that the ANGLE device
// will use the most recent version of D3D11; it might be configured to use
// D3D9. In practice, though, it seems to use 11.1 if it's available, unless
// it's been specifically configured via switch to avoid d3d11.These comments could use updating now that we're sometimes using D3D12 for video decoding.
Current D3D12 video decoder implementation still lives with D3D11, as in `d3d11_video_decoder.cc` the code here manages the picture buffers with D3D11.
How do you expect me update the comments? I think the code here is not changed (I am doing some renaming and refactoring works in this file, but not D3D12-specific) and is functioning as-is. The following code do creates a D3D11 device even when D3D12 backend is used.
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #52, Line 323:
D3D11DecoderConfigurator*
https://google.github.io/styleguide/cppguide. […]
Acknowledged
File media/gpu/windows/d3d12_helpers.h:
Patch Set #56, Line 16:
#include "base/win/scoped_handle.h"scoped_handle.h doesn't appear to be used in the header file, please move it to the cc file.
The `.cc` is not using it either. Removing.
Patch Set #56, Line 23:
inlined_vector.h"inlined_vector.h doesn't appear to be used in the header file, please move it to the cc file.
Around line 64
```
MEDIA_GPU_EXPORT absl::InlinedVector<D3D12_RESOURCE_BARRIER, 2>
CreateD3D12TransitionBarriersForAllPlanes(ID3D12Resource* resource,
...
```
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #55, Line 22:
ToD3D12VideoDecodeReferenceFrames
Avoid const_cast to remove const
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/styleguide/c++/const.md
And I don't find leaving this method non-const will cause any problems.
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #56, Line 49:
luid != CHROME_LUID{}
We've had at least a couple of scenarios where two calls to `EnumAdapters` return different default […]
Done
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
Is there a reason we can't store/cache the D3D12 device in the same place we do the D3D11 one?
The D3D11 video device comes from angle/dawn.
https://source.chromium.org/chromium/chromium/src/+/main:gpu/command_buffer/service/shared_context_state.cc;l=1026-1040;drc=777b9f2d520538f1ce8c63e61e7e3efa81a6f88d
Angle don't have D3D12, and dawn is not always enabled, so we have to create one by ourselves.In `media_gpu_channel_manager` I cannot get the luid. So I didn't find a better place to cache the d3d12 device.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
13 comments:
Patchset:
@Zhibo, thank you for your patience. I think we're getting close.
File media/gpu/windows/d3d11_video_decoder.cc:
// Note that we assume that this is the ANGLE device, since we don't implement
// texture sharing properly. That also implies that this is the GPU main
// thread, since we use non-threadsafe properties of the device (e.g., we get
// the immediate context).
//
// Also note that we don't technically have a guarantee that the ANGLE device
// will use the most recent version of D3D11; it might be configured to use
// D3D9. In practice, though, it seems to use 11.1 if it's available, unless
// it's been specifically configured via switch to avoid d3d11.
Current D3D12 video decoder implementation still lives with D3D11, as in `d3d11_video_decoder. […]
Apologies. I was confusing this call to `get_d3d_device_cb_.Run` with the one where you grab the D3D12 device.
File media/gpu/windows/d3d12_helpers.h:
Patch Set #56, Line 23:
inlined_vector.h"Around line 64 […]
Acknowledged
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #55, Line 22:
ToD3D12VideoDecodeReferenceFrames
> Avoid const_cast to remove const […]
I think the compiler is unhappy with the fact that you're assigning a pointer you're not meant to change `resources_.data()` to `ppTexture2Ds` which is a non-const pointer. The callers does not change these pointers, which is why I suggested to use const_cast.
I do not feel strongly about this one so feel free to resolve if you wish.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #57, Line 106:
if (!(desc.MiscFlags & D3D11_RESOURCE_MISC_SHARED_NTHANDLE))Should lack of `D3D11_RESOURCE_MISC_SHARED_NTHANDLE` be `CHECK`-ed? Seems like developer error during setup of the texture ... and kind of late to be doing such error handling here.
Patch Set #57, Line 154:
NumFrameArguments++What code wraps `NumFrameArguments` back to zero? Does code need to call `Reset`?
Patch Set #57, Line 234:
num_planes_Nit: `num_planes_` can be const since you initialize it in the constructor.
Patch Set #57, Line 250:
ScopedD3D12MemoryBuffer::Nit: Since you're already in `ScopedD3D12MemoryBuffer`, this can be simplified to just call `Commit`
Patch Set #57, Line 262:
NumFrameArguments++I see there's a `CHECK_LE(input_stream_arguments_.NumFrameArguments, 3u)` in `SubmitSlice`. We should probably have one here as well.
Patch Set #57, Line 274:
type_Nit: `type_` can be const since it is initialized in the constructor.
Patch Set #57, Line 419:
TODO:Please have a crbug # for this and other TODOs in the CL. CommandQueues are expensive, memorywise. Typically best to have one per kind.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
> Is there a reason we can't store/cache the D3D12 device in the same place we do the D3D11 one? […]
If you're always starting with a D3D11 device and a LUID is not always available, then you can QI the D3D11 device for an `IDXGIDevice`, then QI the `IDXGIDevice` for an `IDXGIAdapter` and pass the adapter to `D3D12CreateDevice`.
We can cache that in a static and initialize it in a threadsafe manner. See `DXGISwapChainTearingSupported` in src\ui\gl\direct_composition_support.cc for an example.
Hopefully they'll add a `SharedContextState::GetD3D12Device` soon so we don't have to keep local caches in each Chromium component.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #57, Line 7:
#include <d3d12.h>We shouldn't need this `d3d12.h` include.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Zhibo Wang.
Zhibo Wang uploaded patch set #58 to this change.
28 files changed, 1,419 insertions(+), 196 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Patch set 58:Auto-Submit +1
10 comments:
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #55, Line 22:
ToD3D12VideoDecodeReferenceFrames
I think the compiler is unhappy with the fact that you're assigning a pointer you're not meant to ch […]
I think this is the case that ideally it should be const, but in implementation it could not be (due to D3D12_VIDEO_DECODE_REFERENCE_FRAMES having non-const pointers). I would like to keep this non-const to avoid potential bugs if the caller accidentally changed the value.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #57, Line 106:
if (!(desc.MiscFlags & D3D11_RESOURCE_MISC_SHARED_NTHANDLE))
Should lack of `D3D11_RESOURCE_MISC_SHARED_NTHANDLE` be `CHECK`-ed? Seems like developer error durin […]
Done
Patch Set #57, Line 154:
NumFrameArguments++What code wraps NumFrameArguments back to zero?
In `Reset()`.
Does code need to call Reset?
We shouldn't do it here. The arguments is stored but not submitted yet.
`WaitForFrameBegins()` will call `Reset()`.
Patch Set #57, Line 234:
num_planes_Nit: `num_planes_` can be const since you initialize it in the constructor.
Done
Patch Set #57, Line 250:
ScopedD3D12MemoryBuffer::Nit: Since you're already in `ScopedD3D12MemoryBuffer`, this can be simplified to just call `Commit`
I got an warning if I call virtual method in the destructor. The `ScopedD3D12MemoryBuffer::` is to eliminate the ambiguity.
Patch Set #57, Line 262:
NumFrameArguments++
I see there's a `CHECK_LE(input_stream_arguments_.NumFrameArguments, 3u)` in `SubmitSlice`. […]
`SubmitSlice()` do final submission of the arguments, and it knows there should be at most three arguments. The buffer itself is innocent.
The `CHECK_LE` in `SubmitSlice()` is the semantics of finishing a slice, not for `NumFrameArguments++`.
Patch Set #57, Line 274:
type_Nit: `type_` can be const since it is initialized in the constructor.
Done
Patch Set #57, Line 419:
TODO:
Typically best to have one per kind.
```
typedef
enum D3D12_COMMAND_QUEUE_FLAGS
{
D3D12_COMMAND_QUEUE_FLAG_NONE = 0,
D3D12_COMMAND_QUEUE_FLAG_DISABLE_GPU_TIMEOUT = 0x1
} D3D12_COMMAND_QUEUE_FLAGS;
typedef struct D3D12_COMMAND_QUEUE_DESC
{
D3D12_COMMAND_LIST_TYPE Type;
INT Priority;
D3D12_COMMAND_QUEUE_FLAGS Flags;
UINT NodeMask;
} D3D12_COMMAND_QUEUE_DESC;
```What if we have different priority/flags/node_mask setting?
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
PS58 cached inside `GetD3DDeviceCallback()` using `gpu_info.active_gpu().luid`, and reverted @jianl...@intel.com 's CL5009578 since passing luid into video decoder seems redundant now.
Hopefully they'll add a SharedContextState::GetD3D12Device soon so we don't have to keep local caches in each Chromium component.
https://bugs.chromium.org/p/dawn/issues/detail?id=2210 may be related.
But we still cannot get one from angle when dawn is not enabled.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #57, Line 7:
#include <d3d12.h>We shouldn't need this `d3d12.h` include.
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Zhibo Wang uploaded patch set #59 to this change.
28 files changed, 1,423 insertions(+), 196 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
Patch set 58:Auto-Submit +1
1 comment:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #57, Line 262:
NumFrameArguments++`SubmitSlice()` do final submission of the arguments, and it knows there should be at most three arg […]
Added checks to ensure the index won't exceed the size of array, which is 10.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
4 comments:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #57, Line 154:
NumFrameArguments++> What code wraps NumFrameArguments back to zero? […]
Acknowledged
Patch Set #57, Line 250:
ScopedD3D12MemoryBuffer::
I got an warning if I call virtual method in the destructor. […]
Acknowledged
Patch Set #57, Line 419:
TODO:
> Typically best to have one per kind. […]
Chromium should never create a queue which disables GPU timeouts since that will negatively affect other applications on the system.
If we ever use multi-node adapters, then having one queue for each of these is fine.
We should tread lightly before creating queues of different priorities to avoid higher priority work completely starving low priority work. But that's a challenge for another day.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
PS58 cached inside `GetD3DDeviceCallback()` using `gpu_info.active_gpu(). […]
Since you're always starting with a D3D11 device, it's more efficient to obtain the DXGI adapter from it directly and use that to create the D3D12 device. No need to re-enumerate all of the adapters and search for matching LUIDs.
In practice, when will the weakpointer to the `MediaGpuChannelManager` not resolve?
Seems like we shouldn't create the D3D12 device if it doesn't exist?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
Zhibo Wang has uploaded this change for review.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
1 comment:
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #59, Line 37:
Microsoft::WRL::ComPtr<ID3D12Device> deviceYou can't really store objects with destructors in statics. Since the destructor is not going to run in practice, should be good to store a raw pointer here or use base::NoDestructor.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Zhibo Wang.
Zhibo Wang uploaded patch set #60 to this change.
28 files changed, 1,413 insertions(+), 196 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Zhibo Wang.
Zhibo Wang uploaded patch set #61 to this change.
28 files changed, 1,415 insertions(+), 196 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Attention is currently required from: Zhibo Wang.
1 comment:
Patchset:
with PS#61, I got below stack:
```
Received fatal exception EXCEPTION_ACCESS_VIOLATION
(No symbol) [0x00007FFD7EF28D8E]
(No symbol) [0x00007FFD7EF28C91]
D3D11TranslateCreateDevice [0x00007FFC99A9164D+230781]
(No symbol) [0x0000015919670B68]
2
(No symbol) [0x0000015919671000]
malloc_base [0x00007FFD83011B06+54]
media::D3D11VideoDecoderWrapper::Create [0x00007FFCA220D5FF+103] (C:\Users\zhusi\Desktop\Projects\chromium\src\media\gpu\windows\d3d11_video_decoder_wrapper.cc:326)
media::D3D11VideoDecoder::CreateD3DVideoDecoderWrapper [0x00007FFCA2203B65+773] (C:\Users\zhusi\Desktop\Projects\chromium\src\media\gpu\windows\d3d11_video_decoder.cc:299)
media::D3D11VideoDecoder::ResetD3DVideoDecoder [0x00007FFCA220356D+653] (C:\Users\zhusi\Desktop\Projects\chromium\src\media\gpu\windows\d3d11_video_decoder.cc:252)
media::D3D11VideoDecoder::Initialize [0x00007FFCA22045DC+1116] (C:\Users\zhusi\Desktop\Projects\chromium\src\media\gpu\windows\d3d11_video_decoder.cc:414)
```
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Zhibo Wang.
Zhibo Wang uploaded patch set #62 to this change.
28 files changed, 1,419 insertions(+), 196 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron, 朱思达.
Patch set 62:Auto-Submit +1
6 comments:
Patchset:
with PS#61, I got below stack: […]
Fixed in ps62.
Commit Message:
Patch Set #52, Line 9:
Since the rendering side don't accept a D3D12Resource now, the d3d12https://bugs.chromium.org/p/chromium/issues/detail?id=1434145 may be informative.
which means rendering may support it in the future, so `don't` should be fine.
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #42, Line 197:
bool D3D11VideoDecoder::ResetD3DVideoDecoder() {The creation of `ID3D11VideoDecoder` is moved to `CreateD3DVideoDecoderWrapper()`. […]
Now reset to `Reset`.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #57, Line 419:
TODO:Chromium should never create a queue which disables GPU timeouts since that will negatively affect o […]
Acknowledged and done
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #55, Line 37:
CreateD3D12Device(luid)
Since you're always starting with a D3D11 device, it's more efficient to obtain the DXGI adapter fro […]
Done
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #59, Line 37:
Microsoft::WRL::ComPtr<ID3D12Device> device
You can't really store objects with destructors in statics. […]
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Attention is currently required from: Rafael Cintron, Zhibo Wang.
1 comment:
Patchset:
When D3D12 Decoder is enabled and When try to playback a 10bit 420 P010 HEVC video, I got the below message:
```
00:00:00.031 info "D3D11VideoDecoder is using hevc main 10 / 4:2:0"
00:00:00.031 info "D3D11VideoDecoder producing P010"
00:00:00.031 info "D3D11VideoDecoder: Selected P016LE"
00:00:00.031 info "D3D11VideoDecoder output color space: (same as input)"
00:00:00.031 info "D3D11VideoDecoder is binding textures"
00:00:00.031 error "D3D12VideoDecoder does not support codec hevc with chroma subsampling format 4:2:0 and bit depth 8"
00:00:00.031 info "Cannot select D3D11VideoDecoder for video decoding"
00:00:00.031 info "Cannot select VpxVideoDecoder for video decoding"
00:00:00.031 info "Cannot select Dav1dVideoDecoder for video decoding"
00:00:00.031 info "Cannot select FFmpegVideoDecoder for video decoding"
00:00:00.031 error "video decoder initialization failed"
00:00:00.031 error {"code":15,"data":{},"group":"PipelineStatus","message":"","stack":[{"file":"media\\renderers\\video_renderer_impl.cc","line":304}]}
```Seems the bit depth guessing doesn't working and you are using the default 8 bit depth.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #62, Line 294:
media_log_.get(), video_device, config_, bit_depth_, chroma_sampling_);I guess, this can always be the default bit depth which cause the problem.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Patchset:
When D3D12 Decoder is enabled and When try to playback a 10bit 420 P010 HEVC video, I got the below […]
When D3D12 Decoder is enabled, I also run into problem when I try to playback multi video at the same time. When D3D12 Decoder is disabled, no issue repo though, my GPU is Intel A380. Here is the page link for you to repro: https://lf-tk-sg.ibytedtos.com/obj/tcs-client-sg/resources/video_demo_hevc.html, and this is a screen record: https://lf-tk-sg.ibytedtos.com/obj/tcs-client-sg/resources/d3d12_green_artifact.mp4
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron, Zhibo Wang.
Since the rendering side don't accept a D3D12Resource now, the d3d12
28 files changed, 1,421 insertions(+), 196 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron, 朱思达.
Patch set 63:Auto-Submit +1
2 comments:
Patchset:
Seems the bit depth guessing doesn't working and you are using the default 8 bit depth.
Fixed.
Here is the page link for you to repro: https://lf-tk-sg.ibytedtos.com/obj/tcs-client-sg/resources/video_demo_hevc.html
I cannot open 8k30p, 5.7k60p and several specific video, got HTTP 403 for those files.
this is a screen record: https://lf-tk-sg.ibytedtos.com/obj/tcs-client-sg/resources/d3d12_green_artifact.mp4
Is this happening only when one video is playing and one is started but loading? If so, I have observed same issue with D3D11 backend, in that case this should be an existing bug, not being related to this CL.
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #62, Line 294:
media_log_.get(), video_device, config_, bit_depth_, chroma_sampling_);I guess, this can always be the default bit depth which cause the problem.
Done
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Patchset:
403 is normal, as CDN may limit the max download file count, I would suggest download the html and all of the mp4 files into your local disk and open the local html.
Is this happening only when one video is playing and one is started but loading? If so, I have observed same issue with D3D11 backend, in that case this should be an existing bug, not being related to this CL.
I guess you need to download those file to local and try again, it's not something you can reproduce with D3D11 backend, only happening with D3D12 backend.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Patchset:
403 is normal, as CDN may limit the max download file count, I would suggest download the html and a […]
You can try this link (https://lf3-cdn-tos.bytegoofy.com/obj/tcs-client/resources/video_demo_hevc.html) which will have better CDN speed if your ip location is CN.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron, 朱思达.
1 comment:
Patchset:
New link works.
I guess you need to download those file to local and try again, it's not something you can reproduce with D3D11 backend, only happening with D3D12 backend.
I have checked for another time that D3D12 decoding multiple downloaded local files works fine in my place, A380. Maybe we are talking about different issues, and I cannot reproduce yours. Is that still happening on ps63?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Attention is currently required from: Rafael Cintron, Zhibo Wang.
1 comment:
Patchset:
New link works. […]
My steps to reproduce are to let about 15 videos play at the same time together, and then randomly perform a seek operation on the videos among them. I will retry PS#63 tonight to see if I can reproduce.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Patchset:
My steps to reproduce are to let about 15 videos play at the same time together, and then randomly p […]
Still repro with PS#63, see my screen record here: https://lf3-cdn-tos.bytegoofy.com/obj/tcs-client/resources/d3d12_reproduce_ps63.mp4
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
朱思达 (Gerrit)
Patchset:
Still repro with PS#63, see my screen record here: https://lf3-cdn-tos.bytegoofy. […]
Interesting, I also tested D3D12 decoder on NVIDIA RTX3050 and AMD RX6600:
1. Intel A380 + 31.0.101.4953 = repro.
2. NVIDIA RTX3050 + 31.0.15.3758 = no repro, working well.
3. AMD RX660 + 31.0.22017.3004 = completely not working with below error log.```
00:00:06.089 info "D3D11VideoDecoder config change: profile: hevc main, chroma_sampling_format: 4:2:0, coded_size: 7680x4320, bit_depth: 8, color_space: {primary=1, transfer=1, matrix=1, range=1}"
00:00:06.089 info "D3D11VideoDecoder is using hevc main / 4:2:0"
00:00:06.089 info "D3D11VideoDecoder producing NV12"
00:00:06.089 info "D3D11VideoDecoder: Selected NV12"
00:00:06.089 info "D3D11VideoDecoder output color space: (same as input)"
00:00:06.089 info "D3D11VideoDecoder is binding textures"
00:00:06.090 info "D3D11VideoDecoder is using D3D12 backend"
00:00:06.090 info "D3D11VideoDecoder is using single textures"
00:00:06.102 error "Failed to close command list: 参数错误。 (0x80070057)"
00:00:06.102 warning "video decoder fallback after initial decode error."
00:00:06.102 info "Cannot select VpxVideoDecoder for video decoding"
00:00:06.102 info "Cannot select Dav1dVideoDecoder for video decoding"
00:00:06.103 info "Cannot select FFmpegVideoDecoder for video decoding"
00:00:06.123 error {"cause":{"cause":{"code":6,"data":{"VDA Error":0},"group":"D3D11Status","message":"","stack":[{"file":"media\\gpu\\windows\\d3d11_video_decoder.cc","line":675}]},"code":1,"data":{},"group":"DecoderStatus","message":"","stack":[{"file":"media\\gpu\\windows\\d3d11_video_decoder.cc","line":973}]},"code":3,"data":{},"group":"PipelineStatus","message":"","stack":[{"file":"media\\renderers\\video_renderer_impl.cc","line":592}]}
```Since RTX3050 can working without issue, is this an intel driver bug?
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang, 朱思达.
4 comments:
Patchset:
Interesting, I also tested D3D12 decoder on NVIDIA RTX3050 and AMD RX6600: […]
Making this a real issue so we don't lose track of it.
The string for error code 0x80070057 is "The parameter is incorrect" which is hopefully something we can get to the bottom of by using the D3D debug layer.
As a followon change, we should consider emitting this error code to the media log using `logging::SystemErrorCodeToString(hr)` so we don't have to look it up.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #63, Line 357:
compressed_bitstream_Nit: `std::move(compressed_bitstream_)`
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #62, Line 29:
gpu_info`gpu_info` is no longer used in the function and can be removed.
Microsoft::WRL::ComPtr<IDXGIDevice> dxgi_device;
CHECK_EQ(d3d11_device.As(&dxgi_device), S_OK);
Microsoft::WRL::ComPtr<IDXGIAdapter> adapter;
CHECK_EQ(dxgi_device->GetAdapter(&adapter), S_OK);
static base::NoDestructor<Microsoft::WRL::ComPtr<ID3D12Device>>
d3d12_device(CreateD3D12Device(adapter.Get()));
return *d3d12_device;Please make this thread safe by assigning a `d3d12_device` to a function which runs all of the code I've highlighted, or an inline function.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Zhibo Wang, 朱思达.
Zhibo Wang uploaded patch set #64 to this change.
28 files changed, 1,438 insertions(+), 205 deletions(-)
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron, 朱思达.
Patch set 64:Auto-Submit +1
4 comments:
Patchset:
Since RTX3050 can working without issue, is this an intel driver bug?
I have reported it internally.
As a followon change, we should consider emitting this error code to the media log using logging::SystemErrorCodeToString(hr) so we don't have to look it up.
I guess that is already media_log?
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #63, Line 357:
compressed_bitstream_Nit: `std::move(compressed_bitstream_)`
We shouldn't `std::move` it. It will be used again. Passing a raw pointer now.
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #62, Line 29:
gpu_info`gpu_info` is no longer used in the function and can be removed.
Done
Microsoft::WRL::ComPtr<IDXGIDevice> dxgi_device;
CHECK_EQ(d3d11_device.As(&dxgi_device), S_OK);
Microsoft::WRL::ComPtr<IDXGIAdapter> adapter;
CHECK_EQ(dxgi_device->GetAdapter(&adapter), S_OK);
static base::NoDestructor<Microsoft::WRL::ComPtr<ID3D12Device>>
d3d12_device(CreateD3D12Device(adapter.Get()));
return *d3d12_device;
Please make this thread safe by assigning a `d3d12_device` to a function which runs all of the code […]
Done. I think static variable initialization is already thread safe. Adding an immediately-invoked lambda to avoid re-construction of dxgi_device and adapter.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang, 朱思达.
2 comments:
Patchset:
> Since RTX3050 can working without issue, is this an intel driver bug? […]
If we use `logging::SystemErrorCodeToString(hr)`, we'll get a more helpful error message than "0x80070057", which is what I see in the log output above.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
if (!d3d12_resource_or_error.has_value()) {
return false;
}(Apologies for not realizing this earlier)
The D3D12 `WaitForFrameBegins` implementation takes more raw pointer references than the D3D11 one. We take the texture from the `output_picture` and store it in non-raw_ptr D3D12 structures like `output_stream_arguments_`. We never clear these structures on error or in Reset.
Are we sure the `output_picture` textures are never going to deleted out from under us? Seems like we should at least `std::move` `output_stream_arguments_` into a local variable and clear out the member variable in `Reset` in case this call to `ToD3D12Resource` fails.
Please test with `dxcaps.exe -forcetdr` to ensure device removed is handled without crashing.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Zhibo Wang (Gerrit)
Attention is currently required from: Rafael Cintron.
2 comments:
Patchset:
00:00:06.102 error "Failed to close command list: 参数错误。 (0x80070057)"
"参数错误。" is the localized helpful message, which is "The parameter is incorrect" in Chinese. `logging::SystemErrorCodeToString()` is using system language.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
if (!d3d12_resource_or_error.has_value()) {
return false;
}
and store it in non-raw_ptr D3D12 structures like output_stream_arguments_.
`output_stream_arguments_.pOutputTexture2D` is raw pointer.
```
typedef struct D3D12_VIDEO_DECODE_OUTPUT_STREAM_ARGUMENTS
{
ID3D12Resource *pOutputTexture2D;
UINT OutputSubresource;
D3D12_VIDEO_DECODE_CONVERSION_ARGUMENTS ConversionArguments;
} D3D12_VIDEO_DECODE_OUTPUT_STREAM_ARGUMENTS;
```
We never clear these structures on error or in Reset.
Are we sure the output_picture textures are never going to deleted out from under us?
The `output_picture` is get from `D3D11VideoDecoder::picture_buffers_`, which is created once and repeatedly used by the frames in the decoding procedure.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Zhibo Wang.
3 comments:
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
if (!d3d12_resource_or_error.has_value()) {
return false;
}
output_stream_arguments_.pOutputTexture2D is raw pointer.
Apologies for being unclear. What I meant by my statement is that we're storing pointers to D3D textures into raw pointers instead of using `raw_ptr`. We're, of course, forced to use these structures so we can pass them to D3D but it means we have to be extra careful about not letting these ever point to freed memory.
The output_picture is get from D3D11VideoDecoder::picture_buffers_, which is created once and repeatedly used by the frames in the decoding procedure.
What about things like config changes? Are the pointers cleared out then? If so, then many we'll be OK but I'll defer to Chromium media experts on the code review.
Patch Set #64, Line 257:
written_sizePlease add a CHECK that `written_size` is less than `data_.size()`.
Patch Set #64, Line 301:
written_sizePlease add a CHECK that `written_size` is less than `data_.size()`.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Zhibo Wang.
14 comments:
File media/base/media_switches.h:
Patch Set #64, Line 492:
MEDIA_EXPORT BASE_DECLARE_FEATURE(kUseD3D12VideoDecoder);Just `kD3D12VideoDecoder`
File media/base/media_switches.cc:
Patch Set #64, Line 1652:
"UseD3D12VideoDecoder",`D3D12VideoDecoder` ensure this is in the IS_WIN section (ditto for header)
File media/gpu/windows/d3d11_picture_buffer.cc:
Patch Set #64, Line 125:
d3d12_resource_->GetDevice(IID_PPV_ARGS(&used_device));Does this need a FAILED test?
File media/gpu/windows/d3d11_video_decoder.cc:
Patch Set #64, Line 216:
// TODO: supported check?Remove stale comment, not useful.
Patch Set #64, Line 217:
std::unique_ptr<D3D11DecoderConfigurator> decoder_configurator =Good use for auto.
Patch Set #64, Line 240:
std::unique_ptr<TextureSelector> texture_selector = TextureSelector::Create(Ditto.
Patch Set #64, Line 1096:
auto d3d_device = get_d3d_device_cb.Run(D3DVersion::kD3D11);We don't expect differences in support between 11 and 12 do we?
File media/gpu/windows/d3d11_video_decoder_wrapper.cc:
Patch Set #64, Line 349:
// DXVA HEVC, VP9, and AV1 specifications say ConfigBitstreamRawComment should go at l.345 now.
Patch Set #64, Line 357:
// ConfigBitstreamRaw == 2 means the decoder uses DXVA_Slice_H264_Short.Put at l.355 and consider flipping order to be the same as above (check ConfigBitstreamRaw first)
File media/gpu/windows/d3d12_helpers.h:
Patch Set #64, Line 51:
std::array<ID3D12Resource*, kMaxSize> resources_;The Chromium side c++ structures should either be using ComPtr or raw_ptr if this is held across call stacks.
Consider replacing `ToD3D12VideoDecodeReferenceFrames()` with `WriteTo(D3D12_VIDEO_DECODE_REFERENCE_FRAMES* dest)` given its usage.
File media/gpu/windows/d3d12_helpers.cc:
Patch Set #64, Line 127:
NOTREACHED_NORETURN();Not needed since you have a default clause.
File media/gpu/windows/d3d12_video_decoder_wrapper.cc:
Patch Set #64, Line 374:
#if BUILDFLAG(ENABLE_HEVC_PARSER_AND_HW_DECODER)No need to check the build flag here, it should already be checked in GetD3D12VideoDecodeGUID?
File media/gpu/windows/d3d_video_decoder_wrapper.h:
Patch Set #64, Line 38:
virtual absl::optional<bool> GetSingleTextureRecommended() const = 0;Just `UseSingleTexture` or `ShouldUseSingleTexture`
File media/mojo/services/gpu_mojo_media_client_win.cc:
Patch Set #64, Line 41:
static base::NoDestructor<Microsoft::WRL::ComPtr<ID3D12Device>>Is this right? It seems like this could change / be invalidated? I guess it might be tied to GPU process life time, but probably safer not store it unless we really have to.
To view, visit change 4410406. To unsubscribe, or for help writing mail filters, visit settings.