Skips the first frame in DxgiDuplicatorController

webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.h"
 
 #include <windows.h>
 
 #include <algorithm>
 #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() {
   DxgiDuplicatorController::Instance()->Unregister(this);
 }
 
 void DxgiDuplicatorController::Context::Reset() {
   identity_ = 0;
 }
 
@@ skipping 63 matching lines @@
       id -= duplicators_[i].screen_count();
     } else {
       return duplicators_[i].ScreenRect(id);
     }
   }
   return DesktopRect();
 }
 
 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) {
   rtc::CritScope lock(&lock_);
   if (ContextExpired(context)) {
     // The Context has not been setup after a recent initialization, so it
     // should not been registered in duplicators.
     return;
   }
   for (size_t i = 0; i < duplicators_.size(); i++) {
@@ skipping 103 matching lines @@
 
 bool DxgiDuplicatorController::DoDuplicate(Context* context,
                                            int monitor_id,
                                            SharedDesktopFrame* target) {
   RTC_DCHECK(target);
   target->mutable_updated_region()->Clear();
   rtc::CritScope lock(&lock_);
   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;
 }
 
 bool DxgiDuplicatorController::DoDuplicateUnlocked(Context* context,
                                                    int monitor_id,
                                                    SharedDesktopFrame* target) {
   if (!Initialize()) {
     // Cannot initialize COM components now, display mode may be changing.
     return false;
   }
 
   if (resolution_change_detector_.IsChanged(
           GetScreenRect(kFullDesktopScreenId, std::wstring()).size())) {
     // Resolution of entire screen has been changed, which usually means a new
     // monitor has been attached or one has been removed. The simplest way is to
     // Deinitialize() and returns false to indicate downstream components.
     return false;
   }
 
   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()) {
       monitor_id -= duplicators_[i].screen_count();
     } 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