From b0cc68e61205fd11a7256a6e85307ec17ad95790 Mon Sep 17 00:00:00 2001 From: henrika Date: Thu, 2 Nov 2023 10:16:00 +0100 Subject: [PATCH] Reduces rate at which TryGetNextFrame returns NULL for WGC This CL is a follow-up of work done in https://webrtc-review.googlesource.com/c/src/+/323882 where the goal was to reduce the amount of FrameDropped error logs in WebRTC.DesktopCapture.Win.WgcCaptureSessionGetFrameResult. The previous work avoids FrameDropped logs for a minimized window being captured with WGC but we still se a large amount of these error (or rather warning) logs. See [1] which comes from Canary. This CL does two different things to improve the situation: 1) It adds kFramePoolEmpty to the existing GetFrameResult::kFrameDropped enum to point out that the warning comes from the frame pool not being able to return a valid new frame. It also makes it more clear that it does not cause an outer/final error as WgcCapturerResult::kFrameDropped. We still keep the inner GetFrameResult::kFrameDropped but it is only produced when the frame pool returns NULL and our external queue is empty. Hence, a real frame-drop error. Note that, it is still easy to provoke kFramePoolEmpty simply by asking for a high resolution at a high rate. The example in [2] comes from a 4K screen @30fps. Hence, we have not fixed anything yet. 2) It also increases the size of the internal frame pool from 1 to 2. This does lead to an almost zero rate of kFramePoolEmpt warnings at the expense of a slightly reduced max capture rate. BUT, with 1 as size, we can "see" a higher max capture rate but it is not a true rate since it comes with a high rate of kFramePoolEmpty errors. Hence, we "emulate" a high capture rate by simply feeding copies of the last frame that we had stored in the external queue. Using 2 leads to a more "true" rate of what we actually can capture in terms of *new* frames and also a substantially lower rate of kFramePoolEmpty. In addition, with 1 as size, if we ask at a too high rate and provide a copy of the last frame, our CPU adaptation will not reduce its rate since we think that things are OK when it is actually not. Also, the samples in [3] and [4] both use 2 as numberOfBuffers as well. Let me also mention that with this small change, I a have not been able to provoke any kFramePoolEmpty error messages. Finally, geDisplayMedia can be called called with constraints where min and max framerate is defined. The mechanism which maintains the min rate is implemented via the RequestRefreshFrame API and it can be sent to the source (DesktopCaptureDevice) back to back with a previous timer interrupt for a capture request. Without this change, these RRFs were also a source of a large amount of kFramePoolEmpty error logs. With 2 buffers instead; this is no longer the case. [1] https://screenshot.googleplex.com/7sfv6HdGXLwyxdj [2] https://paste.googleplex.com/4795680001359872 [3] https://github.com/robmikh/Win32CaptureSample/blob/master/Win32CaptureSample/SimpleCapture.cpp [4] https://learn.microsoft.com/en-us/windows/uwp/audio-video-camera/screen-capture#add-the-screen-capture-capability (cherry picked from commit 4be5927dc788b814dff2a6ca46175254dce59232) Bug: chromium:1314868 Change-Id: I73b823b31a993fd2cd6e007b212826dfe1a80012 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/325521 Commit-Queue: Alexander Cooper Reviewed-by: Alexander Cooper Cr-Original-Commit-Position: refs/heads/main@{#41079} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/326960 Commit-Queue: Henrik Andreassson Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/branch-heads/6099@{#1} Cr-Branched-From: 507f1cc3270d0577f79882acbd78e63e66008f3d-refs/heads/main@{#41042} --- modules/desktop_capture/win/wgc_capture_session.cc | 14 ++++++++++---- modules/desktop_capture/win/wgc_capture_session.h | 8 +++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/modules/desktop_capture/win/wgc_capture_session.cc b/modules/desktop_capture/win/wgc_capture_session.cc index 2f7fac9f02..8c74c2bf24 100644 --- a/modules/desktop_capture/win/wgc_capture_session.cc +++ b/modules/desktop_capture/win/wgc_capture_session.cc @@ -75,7 +75,8 @@ enum class GetFrameResult { kGetContentSizeFailed = 9, kResizeMappedTextureFailed = 10, kRecreateFramePoolFailed = 11, - kMaxValue = kRecreateFramePoolFailed + kFramePoolEmpty = 12, + kMaxValue = kFramePoolEmpty }; void RecordStartCaptureResult(StartCaptureResult error) { @@ -332,10 +333,15 @@ HRESULT WgcCaptureSession::ProcessFrame() { } if (!capture_frame) { - // Avoid logging errors until at least one valid frame has been captured. - if (queue_.current_frame()) { - RTC_DLOG(LS_WARNING) << "Frame pool was empty => kFrameDropped."; + if (!queue_.current_frame()) { + // The frame pool was empty and so is the external queue. + RTC_DLOG(LS_ERROR) << "Frame pool was empty => kFrameDropped."; RecordGetFrameResult(GetFrameResult::kFrameDropped); + } else { + // The frame pool was empty but there is still one old frame available in + // external the queue. + RTC_DLOG(LS_WARNING) << "Frame pool was empty => kFramePoolEmpty."; + RecordGetFrameResult(GetFrameResult::kFramePoolEmpty); } return E_FAIL; } diff --git a/modules/desktop_capture/win/wgc_capture_session.h b/modules/desktop_capture/win/wgc_capture_session.h index 8084bf1e37..d2901d9199 100644 --- a/modules/desktop_capture/win/wgc_capture_session.h +++ b/modules/desktop_capture/win/wgc_capture_session.h @@ -52,10 +52,12 @@ class WgcCaptureSession final { return is_capture_started_; } - // We only keep 1 buffer in the internal frame pool to reduce the latency as - // much as possible. + // We keep 2 buffers in the frame pool since it results in a good compromise + // between latency/capture-rate and the rate at which + // Direct3D11CaptureFramePool.TryGetNextFrame returns NULL and we have to fall + // back to providing a copy from our external queue instead. // We make this public for tests. - static constexpr int kNumBuffers = 1; + static constexpr int kNumBuffers = 2; private: // Initializes `mapped_texture_` with the properties of the `src_texture`,