Deliver texture from WGC desktop capturer [chromium/src : main]
Wang, Zhibo1 (Gerrit)
Wang, Zhibo1 voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Cooper
- Elad Alon
- Ilya Nikolaevskiy
- Qiu, Jianlin
- Sam Zackrisson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 6 comments![]( "Open in Gerrit")
if (!frame->size().equals(output_size)) {One thing I don't see yet (please point me to it If I missed it) is how the capture resolution is enforced. Was it done in the previous CL? Does WGC produce frames already downscaled to the correct size? I presume it just outputs the native resolution of the window/desktop.
There are 2 ways to do it:
1) Scale using DirectX in the WGC capturer code before outputing the texture, but you'll have to wire up the resolution down there.
2) Pass natural_size on top of already provided visible_rect. They pass it in VideoFrame::WrapSharedImage in VideoCaptureImpl [1]. So you'll have to extend VideoFrameInfo [2] and populate it in DeliverTextureToClient.
if (!sii_) {You need to recreate sii_ if there's a ContextLoss: https://source.chromium.org/chromium/chromium/src/+/main:gpu/command_buffer/client/shared_image_interface.h;l=371;drc=0d56d0143aa2bb6c7bfad81a4e27063ad2a9de8a
If the GPU process crashes the sii_ will become unusable.
auto shared_image = sii_->CreateSharedImage(Wang, Zhibo1This will probably conflict with https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/video_capture/video_capture_impl.cc;l=571;drc=d899a5c7be25cb43e2c926e643ac147ece0b1d37
Ilya NikolaevskiyBindVideoFrameOnMediaTaskRunner and GpuMemoryBufferTrackerWin assert the format to be NV12. WGC texture outputs ARGB.
No that's actually because the ExternalBuffer you pass has SharedImage, so it will be sent over as a `VideoFrameBufferHandleType::kSharedImageHandle` which doesn't require creation of the SharedImages in the renderer process. So this is fine.
owned_shared_images_.pop_front();Wang, Zhibo1So what happens if we have too many images in flight? Some are destroyed prematurely?
A better approach would be to drop tha frame if there are too many in flight. This will limit the framerate instead of introducing some periods of glitches.
Ilya NikolaevskiyThe latest patchset uses owned ClientSharedImage, the lifetime is managed by scoped_refptr, and the deque is removed.
Acknowledged
media::CapturedExternalVideoBuffer(Please use OnIncomingCapturedImageZeroCopy instead.
dxgi_device_manager_ = DXGIDeviceManager::Create(CHROME_LUID{0, 0});Why is this even needed? You are sending ClientSharedImages, therefore the code on L76 would return from the fucntion and this code here will not be executed.
Then, if you want to set it up, you need to pass the correct LUID here. On the machine with multiple GPU adapters it has to use the correct one.
The screenshare capture system doesn't even set the dxgi device manager anymore, but it was in the past. You can see how it was done here: https://crrev.com/c/4623506/18/content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc
Related details
- Alexander Cooper
- Elad Alon
- Qiu, Jianlin
- Sam Zackrisson
- Wang, Zhibo1
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 1 comment![]( "Open in Gerrit")
Another concern is a multi-gpu systems.
I didn't notice it while reviewing the previous CL.
If the original texture is localted on a wrong adapter, not the one used by the GPU process, It will not be usable. The CreateSharedImage call will fail.
There are several solutions possible:
1) in the DesktopCaptureDevice here create a valid texture and copy the data there before creating the SharedImage.
2) Pass the LUID all the way to the WgcCaptureSession and don't output the texture if it's on a wrong adapter. But I'm not sure how to check if the texture is on the correct adapter yet. Or maybe you should create the D3D11Device on the correct LUID in WgcCapturerWin [1] then the processing of the texture would just fail.
3) Pass the LUID to WgcCaptureSession and somehow register it to output textures on that adapter. I don't know if it's possible. Maybe you can s
You probably can get the LUID from the gpu info where you establish the sii_.
Sam Zackrisson (Gerrit)
Sam Zackrisson added 3 comments![]( "Open in Gerrit")
Thank you for this change.
I don't know the video parts, so will hold off on stamping LGTM for media/webrtc/.
media/webrtc/ LGTM modulo my comment, if the video reviewers agree with and approve the main changes.
COMPONENT_EXPORT(MEDIA_WEBRTC)
BASE_DECLARE_FEATURE(kWebRtcAllowWgcUsingTexture);...same `#if` comment as in the .cc file
// When enabled, DesktopCapturer will provide a texture handle in DesktopFrame
// instead of mapping texture data, if the WGC capturer is available and
// enabled. In this mode, texture are not mapped by default to reduce memory
// copies. Clients should process texture in the same sequence as desktop
// capturer.
BASE_FEATURE(kWebRtcAllowWgcUsingTexture, base::FEATURE_DISABLED_BY_DEFAULT);
Windows-specific feature flag: This should be within the same `#if BUILDFLAG(IS_WIN)` as the WGC flag above.
Related details
- Alexander Cooper
- Elad Alon
- Qiu, Jianlin
- Wang, Zhibo1
Alexander Cooper (Gerrit)
Alexander Cooper added 1 comment![]( "Open in Gerrit")
Looks like you have OWNERS covered without me. Removing myself as I am OOO for the next 4 weeks.
Related details
- Elad Alon
- Qiu, Jianlin
- Wang, Zhibo1
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wang, Zhibo1 (Gerrit)
Wang, Zhibo1 added 7 comments![]( "Open in Gerrit")
Another concern is a multi-gpu systems.
I didn't notice it while reviewing the previous CL.
If the original texture is localted on a wrong adapter, not the one used by the GPU process, It will not be usable. The CreateSharedImage call will fail.
There are several solutions possible:
1) in the DesktopCaptureDevice here create a valid texture and copy the data there before creating the SharedImage.2) Pass the LUID all the way to the WgcCaptureSession and don't output the texture if it's on a wrong adapter. But I'm not sure how to check if the texture is on the correct adapter yet. Or maybe you should create the D3D11Device on the correct LUID in WgcCapturerWin [1] then the processing of the texture would just fail.
3) Pass the LUID to WgcCaptureSession and somehow register it to output textures on that adapter. I don't know if it's possible. Maybe you can s
You probably can get the LUID from the gpu info where you establish the sii_.
Implementing `3.` at https://webrtc-review.googlesource.com/c/src/+/461040
While `3.` is truncated, I think the desktop capture texture will be on the d3d11 device that is used to create the pool. So passing the active gpu luid is ok.
if (!frame->size().equals(output_size)) {One thing I don't see yet (please point me to it If I missed it) is how the capture resolution is enforced. Was it done in the previous CL? Does WGC produce frames already downscaled to the correct size? I presume it just outputs the native resolution of the window/desktop.
There are 2 ways to do it:
1) Scale using DirectX in the WGC capturer code before outputing the texture, but you'll have to wire up the resolution down there.
2) Pass natural_size on top of already provided visible_rect. They pass it in VideoFrame::WrapSharedImage in VideoCaptureImpl [1]. So you'll have to extend VideoFrameInfo [2] and populate it in DeliverTextureToClient.
You need to recreate sii_ if there's a ContextLoss: https://source.chromium.org/chromium/chromium/src/+/main:gpu/command_buffer/client/shared_image_interface.h;l=371;drc=0d56d0143aa2bb6c7bfad81a4e27063ad2a9de8a
If the GPU process crashes the sii_ will become unusable.
Done
media::CapturedExternalVideoBuffer(Please use OnIncomingCapturedImageZeroCopy instead.
We only have the abstract class pointer here and there is no `OnIncomingCapturedImageZeroCopy` member method.
dxgi_device_manager_ = DXGIDeviceManager::Create(CHROME_LUID{0, 0});Why is this even needed? You are sending ClientSharedImages, therefore the code on L76 would return from the fucntion and this code here will not be executed.
Then, if you want to set it up, you need to pass the correct LUID here. On the machine with multiple GPU adapters it has to use the correct one.
The screenshare capture system doesn't even set the dxgi device manager anymore, but it was in the past. You can see how it was done here: https://crrev.com/c/4623506/18/content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc
Removed. It was a legacy of early development.
COMPONENT_EXPORT(MEDIA_WEBRTC)
BASE_DECLARE_FEATURE(kWebRtcAllowWgcUsingTexture);...same `#if` comment as in the .cc file
Done
// When enabled, DesktopCapturer will provide a texture handle in DesktopFrame
// instead of mapping texture data, if the WGC capturer is available and
// enabled. In this mode, texture are not mapped by default to reduce memory
// copies. Clients should process texture in the same sequence as desktop
// capturer.
BASE_FEATURE(kWebRtcAllowWgcUsingTexture, base::FEATURE_DISABLED_BY_DEFAULT);
Windows-specific feature flag: This should be within the same `#if BUILDFLAG(IS_WIN)` as the WGC flag above.
- Elad Alon
- Ilya Nikolaevskiy
- Qiu, Jianlin
- Sam Zackrisson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 3 comments![]( "Open in Gerrit")
if (!frame->size().equals(output_size)) {Wang, Zhibo1One thing I don't see yet (please point me to it If I missed it) is how the capture resolution is enforced. Was it done in the previous CL? Does WGC produce frames already downscaled to the correct size? I presume it just outputs the native resolution of the window/desktop.
There are 2 ways to do it:
1) Scale using DirectX in the WGC capturer code before outputing the texture, but you'll have to wire up the resolution down there.
2) Pass natural_size on top of already provided visible_rect. They pass it in VideoFrame::WrapSharedImage in VideoCaptureImpl [1]. So you'll have to extend VideoFrameInfo [2] and populate it in DeliverTextureToClient.
See https://chromium-review.googlesource.com/c/chromium/src/+/7705341
Thanks!
media::CapturedExternalVideoBuffer(Wang, Zhibo1Please use OnIncomingCapturedImageZeroCopy instead.
We only have the abstract class pointer here and there is no `OnIncomingCapturedImageZeroCopy` member method.
Sorry, I didn't notice that this isn't a public method. Can you use OnIncomingCapturedImage and pass it to OnIncomingCapturedImageZeroCopy() like it's done for android [1]?
options.set_d3d_device_luid({luid.LowPart, luid.HighPart});This will help in most cases, but still will fail if the GPU process restarts.
If you have a E2E PoC running, please find a laptop with 2 adapters: integrated and discreete GPUs. Start the chrome when no external display is attached to the laptop. Then attach the display. Everything should work still. Then open `chrome://crashgpu` in a separate tab. After the process reinitializes the screenshare frames will not be rendereable and you will see a lot of errors at the bottom of chrome://gpu.
The issue is that the active LUID changes mid-capture.
It can be detected relatively easy: just check the `active_gpu().luid` on each frame.
Ideally, in this case you should readback the texture using the starting LUID for adapter creation, then send it down as a YUV software frame.
To that end I think you should make DeliverTextureToClient a bool function and if it fails go down the fallback path.
Related details
- Elad Alon
- Qiu, Jianlin
- Sam Zackrisson
- Wang, Zhibo1
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qiu, Jianlin (Gerrit)
Qiu, Jianlin added 1 comment![]( "Open in Gerrit")
options.set_d3d_device_luid({luid.LowPart, luid.HighPart});This will help in most cases, but still will fail if the GPU process restarts.
If you have a E2E PoC running, please find a laptop with 2 adapters: integrated and discreete GPUs. Start the chrome when no external display is attached to the laptop. Then attach the display. Everything should work still. Then open `chrome://crashgpu` in a separate tab. After the process reinitializes the screenshare frames will not be rendereable and you will see a lot of errors at the bottom of chrome://gpu.
The issue is that the active LUID changes mid-capture.
It can be detected relatively easy: just check the `active_gpu().luid` on each frame.
Ideally, in this case you should readback the texture using the starting LUID for adapter creation, then send it down as a YUV software frame.
To that end I think you should make DeliverTextureToClient a bool function and if it fails go down the fallback path.
The situation I see with current CL:
I'm on a KabyLake-G(i7-8809G) platform which has both Intel integrated graphics and AMD integrated graphics. I have the monitor initially attached to my intel graphics, where I started screen capture/encode. Everything looks fine when I am capturing the entire screen.
Then I attach another monitor to the AMD graphics. No I have an extended desktop, with my chrome browser still shown in the first monitor.
Now the screen capture stops. Some related logs:
"[29828:18696:0417/162911.636:VERBOSE1:components\viz\service\gl\gpu_service_impl.cc:1066] GPU: Display Metrics changed
[29828:18696:0417/162911.643:VERBOSE1:gpu\command_buffer\service\shared_image\shared_image_factory.cc:682] CreateSharedImageWithBuffer[CompoundImageBacking] size=1920x1080 usage=RasterRead|DisplayRead|Scanout|VideoEncodeAccelerator format=BGRA_8888 gmb_type=platform
[13292:23348:0417/162911.677:INFO:third_party\webrtc\modules\desktop_capture\win\wgc_capture_session.cc:844] Capture target has been closed.
[13292:23348:0417/162911.677:ERROR:third_party\webrtc\modules\desktop_capture\win\wgc_capture_session.cc:442] The target source has been closed.
[13292:23348:0417/162911.678:ERROR:third_party\webrtc\modules\desktop_capture\win\wgc_capturer_win.cc:431] GetFrame failed."
chrome://gpu is not reporting GPU process crash, though.
Related details
- Elad Alon
- Sam Zackrisson
- Wang, Zhibo1
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 1 comment![]( "Open in Gerrit")
options.set_d3d_device_luid({luid.LowPart, luid.HighPart});Qiu, JianlinThis will help in most cases, but still will fail if the GPU process restarts.
If you have a E2E PoC running, please find a laptop with 2 adapters: integrated and discreete GPUs. Start the chrome when no external display is attached to the laptop. Then attach the display. Everything should work still. Then open `chrome://crashgpu` in a separate tab. After the process reinitializes the screenshare frames will not be rendereable and you will see a lot of errors at the bottom of chrome://gpu.
The issue is that the active LUID changes mid-capture.
It can be detected relatively easy: just check the `active_gpu().luid` on each frame.
Ideally, in this case you should readback the texture using the starting LUID for adapter creation, then send it down as a YUV software frame.
To that end I think you should make DeliverTextureToClient a bool function and if it fails go down the fallback path.
The situation I see with current CL:
I'm on a KabyLake-G(i7-8809G) platform which has both Intel integrated graphics and AMD integrated graphics. I have the monitor initially attached to my intel graphics, where I started screen capture/encode. Everything looks fine when I am capturing the entire screen.
Then I attach another monitor to the AMD graphics. No I have an extended desktop, with my chrome browser still shown in the first monitor.
Now the screen capture stops. Some related logs:
"[29828:18696:0417/162911.636:VERBOSE1:components\viz\service\gl\gpu_service_impl.cc:1066] GPU: Display Metrics changed
[29828:18696:0417/162911.643:VERBOSE1:gpu\command_buffer\service\shared_image\shared_image_factory.cc:682] CreateSharedImageWithBuffer[CompoundImageBacking] size=1920x1080 usage=RasterRead|DisplayRead|Scanout|VideoEncodeAccelerator format=BGRA_8888 gmb_type=platform
[13292:23348:0417/162911.677:INFO:third_party\webrtc\modules\desktop_capture\win\wgc_capture_session.cc:844] Capture target has been closed.
[13292:23348:0417/162911.677:ERROR:third_party\webrtc\modules\desktop_capture\win\wgc_capture_session.cc:442] The target source has been closed.
[13292:23348:0417/162911.678:ERROR:third_party\webrtc\modules\desktop_capture\win\wgc_capturer_win.cc:431] GetFrame failed."chrome://gpu is not reporting GPU process crash, though.
That failure is something completely different. Seems like attaching a new display breaks something inside WGC and it stops the screenshare.
Also, just attaching a new display to another port might not change the default GPU in the system. It might still use the other one, if you have 2 displays. Please try different configurations and observe which GPU adapter is marked as `*active*` in the chrome://gpu. Remember to restart chrome each time you change the display configuration. You need to see different adapter being active.
Usually, then no external displays are attached, you get integrated intel GPU as the active one, once you attach something to the laptop, the discrete AMD adapter will be active (after the chrome restart, ofcourse! It only changes adapter on GPU process startup).
Please do it as I outlined: start the chrome (not presentation, just the browser). It will pick up intel adapter as a default one. Then attach the display. Now AMD will be default one, but the chrome will still use what it did before. Then start the screenshare. Crash the gpu process and it will restart with a new default adapter.
This shouldn't stop WGC screenshare and would expose the issue I've outlined: capturer would still produce texture on the Intel adapter, which won't be renderable in the GPU process.
Ilya Nikolaevskiy (Gerrit)
Also, in case there was a misunderstanding, you have to open a new tab with "chrome://gpucrash" to manually trigger the gpu process crash to restart it, so it will pick up a new default adapter. There will be no natural crash, it's you who needs to trigger it.
Wang, Zhibo1 (Gerrit)
Wang, Zhibo1 added 1 comment![]( "Open in Gerrit")
options.set_d3d_device_luid({luid.LowPart, luid.HighPart});Jianlin's device is kind of special. I use my notebook with Intel iGPU + NVIDIA dGPU. The results are:
- Regardless of whether the external display is attached, when we start the browser or run `chrome://gpucrash`, the Intel iGPU is always the `*active*` GPU in `chrome://gpu`.
- The `GPU preference` in Windows settings `System > Display > Graphics` is the one that determines the active GPU. For Chrome, `Let Windows decide` or `Power saving` selects the Intel iGPU, while `High performance` selects the NVIDIA dGPU.
Since the above case is very rare, currently we report a client error in case of active gpu luid changing.
Related details
- Elad Alon
- Ilya Nikolaevskiy
- Sam Zackrisson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ilya Nikolaevskiy (Gerrit)
Ilya Nikolaevskiy added 2 comments![]( "Open in Gerrit")
Please add a TODO in the code to implement the readback fallback where DeliverTextureToClient() fails.
Thanks for adding this!
auto luid = GpuDataManager::GetInstance()->GetGPUInfo().active_gpu().luid;There's a very low chance of this happening, but if GPU reinitializes between this and the call above there options is set, it will have mismatching LUIDs.
Please use `options.d3d_device_luid()` instead.
Related details
- Elad Alon
- Sam Zackrisson
- Wang, Zhibo1