video: Enable VP9 kSVC HW decoding with D3D11 on Windows. [chromium/src : main]
Shuhai Peng (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
Shuhai Peng uploaded patch set #7 to this change.
video: Enable VP9 kSVC HW decoding with D3D11 on Windows.
The Google Meets application, when running with VP9 as the codec,
is encoding at kSVC mode. On the receiving side, HW decoder must
be able to decode the super-frame(key-frame) sent by WebRTC,
otherwise WebRTC will fallback to SW decoding.
Currently this HW decoding is completely blocked in
rtc_video_decoder_adapter if more than 1 spatial layer is detected
on Windows. This CL aims to enable HW kSVC decoding through D3D11VA.
By the way, another CL needs to be done to support kSVC DXVA decoding
through Microsoft VP9 decoder extension.
Change-Id: Id3c1063f45a2cb48cd26c559fa6190c7b7a35809
---
M media/base/media_switches.cc
M media/base/media_switches.h
M media/gpu/vp9_decoder.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.h
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.h
8 files changed, 60 insertions(+), 19 deletions(-)
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato, Ted Meyer.
13 comments:
Commit Message:
Patch Set #6, Line 7:
video: Enable VP9 kSVC HW decoding on Windows.please provide some background on what this is and why it is needed
Done
Patchset:
Hi, Frank and Ted. Could you please take a look?
File media/base/media_switches.h:
Patch Set #6, Line 209:
definedplease merge this with the OS_WIN block @243.
Done
Patch Set #6, Line 210:
MEDIA_EXPORT extern const base::Feature kD3D11Vp9kSVCHWDecoding;will this work fine on windows ARM?
D3D11 depends on windows and gpu, both x86 and arm support it [1]. There are fewer ARM machines we can use for testing, but I think it will work on ARM.looks like cros/Linux is limited to x86 for a reason
In ChromeOS, only the x86 architecture has VAAPI, ARM doesn't have VAAPI [2].[1] https://en.wikipedia.org/wiki/Direct3D
[2] https://en.wikipedia.org/wiki/Video_Acceleration_API
File media/base/media_switches.cc:
Patch Set #6, Line 542:
definedditto, @724
Done
Patch Set #6, Line 543:
// Enable VP9 k-SVC decoding with HW decoder for webrtc use case on Windows.is it k-SVC or kSVC? the patch calls it one, the comment another
Done
File media/gpu/vp9_decoder.cc:
Patch Set #6, Line 49:
return {};keep the same DLOG message as above, since it shouldn't be talking directly about the switch like th […]
Done
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #6, Line 657:
video_decoder_type == media::VideoDecoderType::kD3D11;is DXVAVideoDecodeAccelerator not capable of handling this too?
Yes, DXVA don't support it currently. For this bug referenced by Frank, DXVA would expect superframe from WebRTC with show_frame flag for each sub-frame correctly set and super_frame_idx properly configured. This is currently not implemented and would need another CL to enable the DXVA path.I wonder why the decoder type is really needed.
Faking the decoder_type_ is not a good choice, I have reverted it. See #282 for more detail.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc:
Patch Set #6, Line 278:
D3D11DecoderStream's support for this should also depend on kExposeSwDecodersToWebRTC [1], since that wil […]
Good catch! I will add the check in the patch for enabling DXVA path.
Patch Set #6, Line 282:
VideoDecoderTypei think this is used for media capabilities. […]
In fact, the Media Capability can't get the decoder type , because decoder has not been created yet. Faking the decoder type is not a good idea, it may trigger unnecessary creation process then fallback.
At runtime, we can get the decoder type is D3D11/DXVA, it can work properly. Check the decoder_type_ in RTCVideoDecoderAdapter::Decode, allow the D3D11 decoder to decode and DXVA fallback to software decoding.
For D3D11VA path, as mentioned above, is verified to work. We would expect it not to be blacklisted by rtc_video_decoder_adapter when kSVC is enabled.
Currently, we first land D3D11 path. When the DXVA path can work properly[1], I will refind the code(win7 check is worth, and return the corret result to Media Capability). But now, I think we can still return false to Media Capability, and keep the origin code. What's your suggestion?
[1]https://chromium-review.googlesource.com/c/chromium/src/+/3222682
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #6, Line 394:
// TODO(chromium:1187565): Updatewhy was this formatting changed?
Done
Patch Set #6, Line 399:
base::AutoLock auto_lock(lock_rather than acquire and drop the lock multiple times, can this be merged with the autolock scope @37 […]
Done
Patch Set #6, Line 861:
video_decoder_type_i don't know if this matters for your use-case, but this may get called only after one or more calls […]
Have reverted it.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Ted Meyer (Gerrit)
Attention is currently required from: Shuhai Peng, Frank Liberato.
Patch set 13:Code-Review +1
1 comment:
Patchset:
How will you be testing this? We can probably do an experiment unless you've got something ready to go.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato, Ted Meyer.
2 comments:
Patchset:
How will you be testing this? I have verified that this CL works properly in Google Meet. Process is as follows:
- Need two accounts, and join Google Meet room.
- Open chrome://webrtc-internals/
- For the sender, ensure the 'googCodecName' is VP9. We can see while data 'googFrameRateSent' is n, the data 'framesEncoded' will increase 3*n(maybe 2*n, it depends on the number of layers), it means that currently it is kSVC stream.
- For the receiver, we can see 'codecImplementationName' is ExternalDecoder and 'googCodecName' is VP9. It means we decode VP9 kSVC stream properly. The code log also shows that the decoded stream is kSVC stream.
Please let me know if you have any concerns/suggestions, thanks. 😊
Hi Ted, PTAL.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
1 comment:
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #13, Line 293:
defined(OS_WIN)i want to make sure that i understand your plan.
dxva will support this before too long, so Vp9HwSupportForSpatialLayers() will return true for windows (maybe excluding win7). the code from 292-303 will revert to what it is in ToT today.
do i understand correctly?
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Ted Meyer (Gerrit)
Attention is currently required from: Shuhai Peng.
1 comment:
Patchset:
> How will you be testing this? […]
Ah, ok so google meet is doing this already in the wild. I had been talking about an in-the-wild experiment anyway, since I'd like to avoid launching something with only a "works on my machine!" test backing it up.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato, Ted Meyer.
3 comments:
Patchset:
Ah, ok so google meet is doing this already in the wild. […]
Ah, I see. Agree with you, can you help me run it with the in-the-wild experiment?
Thanks for the comments, PT-Another-L.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #13, Line 293:
defined(OS_WIN)
i want to make sure that i understand your plan. […]
Yes, but there is one thing needs to be explained. When the DXVA supports decoding the VP9 kSVC stream, the code from 287-303 should be removed. Currently I am focusing on implementing DXVA path.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
2 comments:
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #13, Line 293:
defined(OS_WIN)
the code from 287-303 should be removed
287-303 is needed for non-windows platforms. i'm not sure that it should be removed. wouldn't one remove the #if win parts only?
When the DXVA supports decoding the VP9 kSVC stream
if that's not going to take too long to get DXVA working, would it be easier to wait on these changes until then? it seems like most of this CL is working around the fact that DXVA support isn't landed yet, and Vp9HwSupport...() must give the wrong answer sometimes as a result.
Patch Set #13, Line 294:
if (video_decoder_->GetDecoderType() == media::VideoDecoderType::kD3D11 &&does the equivalent of this check need to show up in RTCVideoDecoderStreamAdapter too? else it'll fall back when it doesn't need to. that, in turn, will skew the results of the DecoderStream experiment that's running right now.
alternatively, once DXVA supports svc, neither DecoderAdapter nor DecoderStreamAdapter will need any changes since Vp9HwSupport...() will give the right answer.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
Shuhai Peng uploaded patch set #14 to this change.
video: Enable VP9 kSVC HW decoding with D3D11 on Windows.
The Google Meets application, when running with VP9 as the codec,
is encoding at kSVC mode. On the receiving side, HW decoder must
be able to decode the super-frame(key-frame) sent by WebRTC,
otherwise WebRTC will fallback to SW decoding.
This CL aims to enable HW kSVC decoding through D3D11VA.
Change-Id: Id3c1063f45a2cb48cd26c559fa6190c7b7a35809
---
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc
M media/base/media_switches.cc
M media/gpu/vp9_decoder.cc
M media/base/media_switches.h
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc
5 files changed, 54 insertions(+), 9 deletions(-)
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Patchset:
Thanks for the comments, PTAL.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #13, Line 293:
defined(OS_WIN)
wouldn't one remove the #if win parts only?
Ah, I see. You are right!
if that's not going to take too long to get DXVA working, would it be easier to wait on these changes until then?
The plan temporarily fell through. I tried to implement DXVA path to support VP9 kSVC stream with VP9 Spec, but the decoder can't decode successfully. So we contacted the Microsoft media team, and they told us that currently Media Foundation Transform doesn't support decoding VP9 kSVC stream. It shows that only when the MFT support decoding the VP9 stream that change resolution dynamically without key-frame, we can implement the corresponding dxva's path with the Spec.
Do you think it is a good idea to keep the original code unitl MFT support it?
Patch Set #13, Line 294:
if (video_decoder_->GetDecoderType() == media::VideoDecoderType::kD3D11 &&
does the equivalent of this check need to show up in RTCVideoDecoderStreamAdapter too?
It makes sense. The decoder may be changed between hardware and software.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
5 comments:
Patchset:
overall looks good. just a few questions. sorry for the delay.
thanks
-fl
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #13, Line 293:
defined(OS_WIN)
> wouldn't one remove the #if win parts only? […]
yeah, if there's going to be a delay in adding dxva support, then this makes sense. please be sure to add a comment about it, and what the goal is. if the DXVA changes fall through for some reason permanently, then we might need to come up with something else here.
but as a reasonably temporary measure, this seems fine.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc:
Patch Set #6, Line 282:
VideoDecoderTypeIn fact, the Media Capability can't get the decoder type , because decoder has not been created yet. […]
if i understand correctly, returning false is the only safe option. we don't know if the hw decoder will support or not support kSVC at the point when MediaCap asks. so we have to say no.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #16, Line 273:
video_decoder_type_ = media::VideoDecoderType::kUnknown;nit: please move this into the .h -- most of the other init that doesn't depend on ctor params happens there.
i don't know why i put `decoder_info_` here -- maybe {} didn't work with GUARDED_BY or something. don't remember.
Patch Set #16, Line 402:
if (video_decoder_type_ == media::VideoDecoderType::kD3D11 &&i don't remember if the decoder type is set by the first decode. if you haven't already done so, i'd suggest verifying that.
i'm worried that it defers picking a decoder until the first successful decode.
right now, if that turns out to be the case, then that decode will fail. however, if the switch kExposeSwDecodersToWebRTC is enabled, then it will try a chrome (not webrtc) sw decoder instead of failing. if chrome's sw decoders support kSVC then maybe no action is needed here at all if kExposeSw... is enabled.
i don't know if anything has to be configured specially in libvpx to kSVC support.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato, Ted Meyer.
6 comments:
Patchset:
Thanks for your comments. PTAL!
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #13, Line 293:
defined(OS_WIN)
yeah, if there's going to be a delay in adding dxva support, then this makes sense. […]
Agree. We will continue to pay attention to the changes in DXVA features and keep in touch with Microsoft. Once dxva supports decoding vp9 kSVC stream, i will try to implement the path.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc:
Patch Set #6, Line 282:
VideoDecoderType
if i understand correctly, returning false is the only safe option. […]
Yes, you are right!
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #16, Line 273:
video_decoder_type_ = media::VideoDecoderType::kUnknown;
nit: please move this into the . […]
I moved them all to .h file. Work properly.
Patch Set #16, Line 402:
if (video_decoder_type_ == media::VideoDecoderType::kD3D11 &&
i don't remember if the decoder type is set by the first decode.
Yes, it defers picking a decoder due to decode and decoder initialiazation work in two different thread. I add the boolean |decoder_configured_| as one of the VP9 kSVC check condition in RTCVideoDecoderStreamAdapter::Decode, the check will be executed only when the decoder is valid.
if chrome's sw decoders support kSVC then maybe no action is needed here at all if kExposeSw... is enabled
When kExposeSwDecodersToWebRTC is enabled, it will use VpxVideoDecoder instead of MojoVideoDecoder(in this CL, it's D3D11), beacause here[1] sorts the priority of decoders based on threshold of resolution, the MojoVideoDecoder(D3D11Decoder) is pushed into deprioritized vector(180p and is_platform_decoder)... So D3D11 will not be configured unless all normal decoder can't decode the stream. Although chrome sw decoder can work properly, it seems that we need another patch to consider the impact of dynamic resolution?
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #20, Line 423:
decoder_info_.is_hardware_acceleratedOnly hardware decoder need to check, currently all sw decoder(webrtc sw decoder, chrome sw decoder) can decode VP9 kSVC stream. It will avoid fallback to webrtc sw decoder when it should use chrome sw decoder(enable kExposeSwDecodersToWebRTC). It's different from RTCVideoDecoderAdapter.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
1 comment:
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #16, Line 402:
if (video_decoder_type_ == media::VideoDecoderType::kD3D11 &&> i don't remember if the decoder type is set by the first decode. […]
i don't know enough about SVC -- does it result in a config change when resolution switches? if so, the DecoderStream can re-choose the decoder at that point.
if not, then we probably need to do something smarter in DecoderSelector.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato, Ted Meyer.
2 comments:
Patchset:
PTAL!
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #16, Line 402:
if (video_decoder_type_ == media::VideoDecoderType::kD3D11 &&
i don't know enough about SVC -- does it result in a config change when resolution switches? if so, […]
kSVC means changing resolution dynamically without key-frame required. It doesn't change the config when resolution changed.
if i understand correctly, we should not allow decoder switch for dynamic resolution change. Reason for that is, if you've decoded a 180p picture with SW decoder and recontructed it, there's no way HW decoder can reference the same 180p reconstructed picture, for decoding subsequent 360p or 720p frame. i think we need another patch to consider the impact of dynamic resolution, it needs some work.
Now, based on this CL, VP9 kSVC decoding can work properly in all cases, except when kExposeSwDecodersToWebRTC is enabled, it would use VpxVideoDecoder(yeah... it can decode kSVC as above mentioned), it's not bad.
could we submit this patch first? what's your suggestion?
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
Patch set 21:Code-Review +1
3 comments:
Patchset:
Ah, I see. […]
there is a canary DecoderStreamAdapter experiment that i'm running now. it has three arms: control (uses DecoderAdapter), "DecoderStreamAdapter but enable hardware decoders only", and "DecoderStreamAdapter with both hw and sw enabled".
the experiment is off at this moment, because of a bug that showed up today. as soon as the fix hits canary, i'll re-enable it.
i suggest that we get some results from that experiment. if everything looks good, then the "DecoderStream + hw only" branch will go away -- it's just an intermediate step away from DecoderAdapter. the real goal is to enable DecoderStream+hw/sw decoders. i plan to push a DecoderAdapter (control) and DecoderStreamAdapter+hw/sw to a 50% beta experiment as soon as it's stable in canary (hopefully very soon!), because the data in canary is low volume for rtc.
once we do that, we could do a 50/50 SVC experiment on canary and see what happens. else we're changing too many things at once, given the low volume of canary rtc. we likely don't need DecoderAdapter at all -- could be 50/50 on DecoderStream+hw/sw.
WDYT?
Patchset:
lgtm. thanks for iterating on this.
-fl
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc:
Patch Set #16, Line 402:
if (video_decoder_type_ == media::VideoDecoderType::kD3D11 &&
kSVC means changing resolution dynamically without key-frame required. […]
if there's not an explicit config change, then DecoderStream won't change decoders. config change => key frame, so at least it won't break as-is.
submitting this patch as is sgtm.
as a follow-up, we can teach DecoderSelector to change its behavior if SVC is enabled. maybe pick hw decoders over sw even for low-res configs. alternatively, we could start with SW for low res using the existing DecoderSelector heuristic. if the stream later adapts to a resolution that's high enough, we could ask rtc for a keyframe and swap decoders at that point. that could be done by DecoderStreamAdapter, without any changes in DecoderStream or DecoderSelector. it knows if it has a hw or sw decoder, and it knows the decoded frame size.
i'm not sure what the right behavior is, maybe it's neither of those.
but, all of that is future work.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Jianlin Qiu (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer.
1 comment:
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Ted Meyer.
Shuhai Peng has uploaded this change for review.
video: Enable VP9 kSVC HW decoding with D3D11 on Windows.
The Google Meets application, when running with VP9 as the codec,
is encoding at kSVC mode. On the receiving side, HW decoder must
be able to decode the super-frame(key-frame) sent by WebRTC,
otherwise WebRTC will fallback to SW decoding.
This CL aims to enable HW kSVC decoding through D3D11VA.
Change-Id: Id3c1063f45a2cb48cd26c559fa6190c7b7a35809
---
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc
M media/base/media_switches.cc
M media/gpu/vp9_decoder.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.h
M media/base/media_switches.h
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc
7 files changed, 108 insertions(+), 30 deletions(-)
Shuhai Peng (Gerrit)
Attention is currently required from: Ted Meyer.
2 comments:
Patchset:
there is a canary DecoderStreamAdapter experiment that i'm running now. […]
Agree with you, thanks!
Patchset:
Thanks for your comments!
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Chunbo Hua (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer, Chunbo Hua.
Patch set 21:Commit-Queue +2
Shuhai Peng (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer, Chunbo Hua.
Shuhai Peng removed a vote from this change.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Ted Meyer, Chunbo Hua.
1 comment:
Patchset:
Agree with you, thanks!
sorry, i misunderstood what you meant at first... but i understand what you mean now! OK, hold this patch until you finish the experiment. thanks!
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng, Ted Meyer, Chunbo Hua.
4 comments:
Patchset:
sorry, i misunderstood what you meant at first... […]
sorry for the confusion -- i only meant to delay the experiment. this patch's changes are behind a default-off flag in media_switches, so landing it won't change any behavior.
however, when i took another look at PS21, i noticed one or two unrelated things that i missed before. nothing serious.
Patchset:
still lgtm, but with two new comments that i missed before.
thanks
-fl
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #21, Line 300:
kD3D11Vp9kSVCHWDecodingi just thought of something -- don't you need to enable kVp9kSVCHWDecoding in addition to kD3D11Vp9... in order to make svc decoding work? vp9_decoder.cc [1] checks kVp9kSVCHWDecoding, anad d3d11 uses VP9Decoder [2].
if i understand correctly, then i think the MediaCapabilities check in the factory will return true, which wasn't our goal.
i guess one could make VP9Decoder check both flags, but that seems ugly.
instead, maybe the kD3D11Vp9kSVC switch could just be removed, and make kVp9 the only switch. it's just used for chromeos now, and is off everywhere else. so if you merged them, then you could get rid of the IsEnabled @300 (keep the rest of the #if win block, of course).
that doesn't fix the MediaCap issue, but that issue is the same issue either way (assuming, of course, that i'm right that you need to enable both switches anyway).
it's okay to land this with both switches if you like - we can always revisit that part if it's necessary.
if i'm wrong about both switches, then none of the above applies. :)
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.h:
Patch Set #21, Line 49:
constexpr const char* kExternalDecoderName = "ExternalDecoder";sorry, i missed this. i didn't mean to move the decoder_info_ init here. i just meant to move the init of the new `decoder_configurd_` to the .h .
in particular, @49 needs to be in the .cc file, which is why `decoder_info_` is initialized in the ctor.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato.
4 comments:
Patchset:
sorry for the confusion -- i only meant to delay the experiment. […]
Thanks.
Patchset:
Thanks for your review. Comments addressed/replied, PT-Another-L.
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #21, Line 300:
kD3D11Vp9kSVCHWDecoding
don't you need to enable kVp9kSVCHWDecoding in addition to kD3D11Vp9... in order to make svc decoding work?
No. I have the same idea as you at the beginning. i find that 'RTCVideoDecoderAdapter::Vp9HwSupportForSpatialLayers'[1] is modified. when the os is win, currently based this CL, we still return false to Media Capability[2]. if i remove the switch 'kD3D11Vp9kSVC', when we want to do experiment in canary, we will set kVp9kSVCHWDecoding with 'base::FEATURE_ENABLED_BY_DEFAULT', then it will return true to MediaCapability, this is not what we want.i think we need still keep the switch kD3D11Vp9HWDecoding... it looks ugly in vp9_decoder.cc ... in fact, i really don't like these code, but seems there is no better way if we keep returning false to MediaCapability :(
WDYT?
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.h:
Patch Set #21, Line 49:
constexpr const char* kExternalDecoderName = "ExternalDecoder";
sorry, i missed this. i didn't mean to move the decoder_info_ init here. […]
Done. BTW, is it not recommended to add anonymous space in .h?
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Attention is currently required from: Frank Liberato.
1 comment:
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.h:
Patch Set #21, Line 49:
constexpr const char* kExternalDecoderName = "ExternalDecoder";
Done. BTW, is it not recommended to add anonymous space in . […]
'Use of internal linkage in .cc files is encouraged for all code that does not need to be referenced elsewhere. Do not use internal linkage in .h files.'[1]
This should be the reason why init decoder_info_ in ctor instead of .h file!
[1]https://google.github.io/styleguide/cppguide.html#Internal_Linkage
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Shuhai Peng (Gerrit)
Patchset:
Hi Frank, should we land this patch now?
thanks,
Shuhai
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Frank Liberato (Gerrit)
Attention is currently required from: Shuhai Peng.
Patch set 24:Code-Review +1
2 comments:
Patchset:
still lgtm -- please feel free to land this as is. the remaining discussion on mediacap and the flags doesn't need to block it.
sorry for the delay.
-fl
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #21, Line 300:
kD3D11Vp9kSVCHWDecoding
> don't you need to enable kVp9kSVCHWDecoding in addition to kD3D11Vp9... […]
i don't see how that changes the MediaCap results. for vp9_decoder.cc to support svc on windows, it still requires both the d3d11 and Vp9SVC flags to be set [1] or d3d11 won't decode svc. so, when testing this on d3d11 (even in the current, two-flag situation), MediaCap will return true for svc support. that seems to happen regardless of whether we have one flag or two; if we have two, both must be set or the vp9 decoder will still fail on svc. i think -- please let me know if i've traced through this incorrectly.
maybe the answer is to make rtc_decoder_factory check for OS_WIN and return false unconditionally for svc? as in, there is no case in windows where we want to advertise svc via mediacap, so ignore the flags and say no.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.
Chunbo Hua (Gerrit)
Attention is currently required from: Shuhai Peng.
Patch set 24:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
video: Enable VP9 kSVC HW decoding with D3D11 on Windows.
The Google Meets application, when running with VP9 as the codec,
is encoding at kSVC mode. On the receiving side, HW decoder must
be able to decode the super-frame(key-frame) sent by WebRTC,
otherwise WebRTC will fallback to SW decoding.
This CL aims to enable HW kSVC decoding through D3D11VA.
Change-Id: Id3c1063f45a2cb48cd26c559fa6190c7b7a35809
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3169330
Reviewed-by: Frank Liberato <libe...@chromium.org>
Reviewed-by: Ted Meyer <tmath...@chromium.org>
Commit-Queue: Chunbo Hua <chunb...@intel.com>
Cr-Commit-Position: refs/heads/main@{#940643}
---
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc
M media/base/media_switches.cc
M media/gpu/vp9_decoder.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_stream_adapter.h
M media/base/media_switches.h
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc
7 files changed, 106 insertions(+), 21 deletions(-)
Shuhai Peng (Gerrit)
2 comments:
Patchset:
Frank, Ted. Thanks for your review!
-Shuhai
File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc:
Patch Set #21, Line 300:
kD3D11Vp9kSVCHWDecoding
i don't see how that changes the MediaCap results. for vp9_decoder. […]
Please see here[1], if os is windows, this function[2] always return false to MediaCap.
To view, visit change 3169330. To unsubscribe, or for help writing mail filters, visit settings.