Skips the first frame in DxgiDuplicatorController

webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc

Index: webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc
diff --git a/webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc b/webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc
index 065a6148c06ba85ded23d9db3943eca650e1a306..b5fe677e40cf932eeee38375845290f0174b8302 100644
--- a/webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc
+++ b/webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc
@@ -16,11 +16,29 @@
 #include <string>
 
 #include "webrtc/base/checks.h"
+#include "webrtc/base/timeutils.h"
 #include "webrtc/modules/desktop_capture/desktop_capture_types.h"
 #include "webrtc/modules/desktop_capture/win/screen_capture_utils.h"
+#include "webrtc/system_wrappers/include/sleep.h"
 
 namespace webrtc {
 
+namespace {
+
+void SleepMoreThanMs(int milliseconds) {

[Sergey Ulanov, 2017/02/23 19:07:30]

Do you really need this function? webrtc::SleepMs() calls Sleep() and I don't
think Sleep() can return early on windows.

[Hzj_jie, 2017/02/23 20:52:11]

According to MSDN, the Sleep() function may still return before the time we
provided. Especially when the time is less than one time slice of Windows, which
is typically 16 milliseconds on Windows 10. In our scenario, DX capturer uses ~2
milliseconds for one frame, so we usually ask the thread to sleep 15
milliseconds.

But I think this function should belong to webrtc/base. See
https://cs.chromium.org/chromium/src/base/threading/platform_thread.h.

[Sergey Ulanov, 2017/02/23 21:06:43]

Where does it say that? I was reading these page:
https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx
I don't see it saying anywhere it may return early.

[Hzj_jie, 2017/02/23 21:48:58]

It states,
If dwMilliseconds is less than the resolution of the system clock, the thread
may sleep for less than the specified length of time. If dwMilliseconds is
greater than one tick but less than two, the wait can be anywhere between one
and two ticks, and so on.
Here the resolution of the system means the time slice length of the OS. It can
be impacted by the timeBegionPeriod() function.

[Sergey Ulanov, 2017/02/24 00:40:40]

Ah, I see. Sorry I missed it. Still, I don't think we really need this function.
If Sleep() returns too soon then EnsureFrameCaptured() will just call this
function again.

[Hzj_jie, 2017/02/24 01:28:52]

Done.

+  if (milliseconds <= 0) {
+    return;
+  }
+  int64_t end_ms = rtc::TimeMillis() + milliseconds;
+  int64_t current_ms = rtc::TimeMillis();
+  while (current_ms < end_ms) {
+    webrtc::SleepMs(end_ms - current_ms);
+    current_ms = rtc::TimeMillis();
+  }
+}
+
+}  // namespace
+
 DxgiDuplicatorController::Context::Context() = default;
 
 DxgiDuplicatorController::Context::~Context() {
@@ -104,14 +122,7 @@ DesktopRect DxgiDuplicatorController::ScreenRect(int id) {
 
 int DxgiDuplicatorController::ScreenCount() {
   rtc::CritScope lock(&lock_);
-  if (!Initialize()) {
-    return 0;
-  }
-  int result = 0;
-  for (auto& duplicator : duplicators_) {
-    result += duplicator.screen_count();
-  }
-  return result;
+  return ScreenCountUnlocked();
 }
 
 void DxgiDuplicatorController::Unregister(const Context* const context) {
@@ -235,7 +246,11 @@ bool DxgiDuplicatorController::DoDuplicate(Context* context,
   if (DoDuplicateUnlocked(context, monitor_id, target)) {
     return true;
   }
-  Deinitialize();
+  if (monitor_id < ScreenCountUnlocked()) {
+    // It's a user error to provide a |monitor_id| larger than screen count. We
+    // do not need to deinitialize.
+    Deinitialize();
+  }
   return false;
 }
 
@@ -256,18 +271,41 @@ bool DxgiDuplicatorController::DoDuplicateUnlocked(Context* context,
   }
 
   Setup(context);
+
+  if (!EnsureFrameCaptured(context, target)) {
+    return false;
+  }
+
+  bool result = false;
   if (monitor_id < 0) {
     // Capture entire screen.
-    for (size_t i = 0; i < duplicators_.size(); i++) {
-      if (!duplicators_[i].Duplicate(&context->contexts_[i], target)) {
-        return false;
-      }
-    }
+    result = DoDuplicateAll(context, target);
+  } else {
+    result = DoDuplicateOne(context, monitor_id, target);
+  }
+
+  if (result) {
     target->set_dpi(dpi());
     return true;
   }
 
-  // Capture one monitor.
+  return false;
+}
+
+bool DxgiDuplicatorController::DoDuplicateAll(Context* context,
+                                              SharedDesktopFrame* target) {
+  for (size_t i = 0; i < duplicators_.size(); i++) {
+    if (!duplicators_[i].Duplicate(&context->contexts_[i], target)) {
+      return false;
+    }
+  }
+  return true;
+}
+
+bool DxgiDuplicatorController::DoDuplicateOne(Context* context,
+                                              int monitor_id,
+                                              SharedDesktopFrame* target) {
+  RTC_DCHECK(monitor_id >= 0);
   for (size_t i = 0; i < duplicators_.size() && i < context->contexts_.size();
        i++) {
     if (monitor_id >= duplicators_[i].screen_count()) {
@@ -275,15 +313,77 @@ bool DxgiDuplicatorController::DoDuplicateUnlocked(Context* context,
     } else {
       if (duplicators_[i].DuplicateMonitor(&context->contexts_[i], monitor_id,
                                            target)) {
-        target->set_dpi(dpi());
         return true;
       }
       return false;
     }
   }
-  // id >= ScreenCount(). This is a user error, so we do not need to
-  // deinitialize.
   return false;
 }
 
+int64_t DxgiDuplicatorController::num_frames_captured() const {

[Sergey Ulanov, 2017/02/23 19:07:30]

GetNumFramesCaptured()

[Hzj_jie, 2017/02/23 20:52:11]

Done.

+  int64_t min = INT64_MAX;
+  for (size_t i = 0; i < duplicators_.size(); i++) {
+    if (duplicators_[i].num_frames_captured() < min) {
+      min = duplicators_[i].num_frames_captured();
+    }
+  }
+
+  return min;
+}
+
+int DxgiDuplicatorController::ScreenCountUnlocked() {
+  if (!Initialize()) {
+    return 0;
+  }
+  int result = 0;
+  for (auto& duplicator : duplicators_) {
+    result += duplicator.screen_count();
+  }
+  return result;
+}
+
+bool DxgiDuplicatorController::EnsureFrameCaptured(Context* context,
+                                                   SharedDesktopFrame* target) {
+  // On a modern system, the FPS / monitor refresh rate is usually larger than
+  // or equal to 60. So 17 milliseconds is enough to capture at least one frame.
+  const int64_t ms_per_frame = 17;
+  const int64_t skip_frames = 2;

[Sergey Ulanov, 2017/02/23 19:07:30]

Where does 2 come from? why 1 is not enough?

[Sergey Ulanov, 2017/02/23 19:07:30]

maybe call it frames_to_skip?

[Hzj_jie, 2017/02/23 20:52:11]

Done.

[Hzj_jie, 2017/02/23 20:52:11]

By using 2, we can ensure we always wait for 17 milliseconds. (Around line 370).

[Sergey Ulanov, 2017/02/23 21:06:43]

But why do we need to ensure that at all? I think we just need to make sure at
least one frame has been captured, that's all.

[Hzj_jie, 2017/02/23 21:48:58]

Here the real issue is, we need to ensure two succeeded AcquireNextFrame()
function calls in two monitor fresh cycles.
If we return before the frame has been fully updated (here we sleep 17
milliseconds to ensure it), we may still get a black frame.
DxgiDuplicatorController immediately starts to capture the frame after this
function.
Indeed I have tried several values of frames_to_skip, and 2 is the minimum but
workable value.

+  if (num_frames_captured() >= skip_frames) {
+    return true;
+  }
+
+  std::unique_ptr<SharedDesktopFrame> fallback_frame;
+  SharedDesktopFrame* shared_frame = nullptr;
+  if (target->size().width() >= desktop_size().width() &&
+      target->size().height() >= desktop_size().height()) {

[Sergey Ulanov, 2017/02/23 19:07:30]

Why is this necessary? Shouldn't the capturer make sure that the target frame
fits the screen?

[Hzj_jie, 2017/02/23 20:52:11]

A ScreenCapturerWinDirectx may capture only one of the monitors, so the |target|
size may be smaller than entire screen resolution, which causes some of the
DxgiOutputDuplicator to be ignored.

+    // |target| is large enough to cover entire screen, we do not need to use
+    // |fallback_frame|.
+    shared_frame = target;
+  } else {
+    fallback_frame = SharedDesktopFrame::Wrap(std::unique_ptr<DesktopFrame>(
+        new BasicDesktopFrame(desktop_size())));
+    shared_frame = fallback_frame.get();
+  }
+
+  int64_t start_ms = rtc::TimeMillis();
+  int64_t last_frame_start_ms = 0;
+  while (num_frames_captured() < skip_frames) {
+    if (num_frames_captured() > 0) {
+      // Sleep |ms_per_frame| before capturing next frame to ensure the screen
+      // has been updated by the video adapter.
+      SleepMoreThanMs(
+          ms_per_frame - (rtc::TimeMillis() - last_frame_start_ms));
+    }
+    last_frame_start_ms = rtc::TimeMillis();
+    if (!DoDuplicateAll(context, shared_frame)) {
+      return false;
+    }
+    if (rtc::TimeMillis() - start_ms > (ms_per_frame * skip_frames * 4)) {

[Sergey Ulanov, 2017/02/23 19:07:30]

Where does 4 come from? I suggest adding a separate const for total timeout for
the first frame. MSDN examples pass 500 to AcquireNextFrame(), so I suggest
using it here as well.

[Hzj_jie, 2017/02/23 20:52:11]

It's a very random number. :)
Updated.

+      return false;
+    }
+  }
+  return true;
+}
+
 }  // namespace webrtc