[Chromoting] Improve DirectX capturer to support multiple outputs

webrtc/modules/desktop_capture/win/dxgi_duplicator_container.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_container.h"
+
+#include <windows.h>
+
+#include "webrtc/base/checks.h"
+
+namespace webrtc {
+
+bool DxgiDuplicatorContainer::SingleEntry::Enter() {

[Sergey Ulanov, 2016/07/08 22:36:54]

I don't think you really need SingleEntry.

[Hzj_jie, 2016/07/11 00:55:00]

Same as below.

+  rtc::CritScope lock(&lock_);
+  if (entered_) {
+    return false;
+  }
+  entered_ = true;
+  return true;
+}
+
+void DxgiDuplicatorContainer::SingleEntry::Exit() {
+  rtc::CritScope lock(&lock_);
+  RTC_DCHECK(entered_);
+  entered_ = false;
+}
+
+bool DxgiDuplicatorContainer::SingleEntry::Hold() const {
+  rtc::CritScope lock(&lock_);
+  return entered_;
+}
+
+class DxgiDuplicatorContainer::AutoExit {
+ public:
+  explicit AutoExit(DxgiDuplicatorContainer::SingleEntry* gate) : gate_(gate) {
+    RTC_DCHECK(gate_);
+  }
+
+  ~AutoExit() { gate_->Exit(); }
+
+ private:
+  DxgiDuplicatorContainer::SingleEntry* gate_;
+};
+
+/* static */ DxgiDuplicatorContainer* DxgiDuplicatorContainer::Instance() {

[Sergey Ulanov, 2016/07/08 22:36:54]

Move the comment to a separate line:

// static
DxgiDuplicatorContainer* DxgiDuplicatorContainer::Instance()...

[Hzj_jie, 2016/07/11 00:55:00]

Done.

+  // The static instance won't be deleted to ensure it can be used by other
+  // threads even during program exiting.
+  static DxgiDuplicatorContainer* instance = new DxgiDuplicatorContainer();
+  return instance;
+}
+
+DxgiDuplicatorContainer::~DxgiDuplicatorContainer() {
+  rtc::CritScope lock(&lock_);
+  Deinitialize();
+}
+
+bool DxgiDuplicatorContainer::Prepare() {
+  // If the instance has been initialized already, does nothing and returns
+  // true.
+  {
+    rtc::CritScope lock(&lock_);
+    if (!devices_.empty()) {
+      return true;
+    }
+  }
+
+  if (initializing_.Enter()) {

[Sergey Ulanov, 2016/07/08 22:36:54]

I think you can implement the same thing without SingleEntry, using
CritScope::TryLock and rtc::Event:


rtc::CritScope initialization_lock_;
rtc::Event initialized_event_;


if (initialization_lock_.TryLock()) {
  // Initialize Here.
  initialized_event_.Set();
} else {
  initialized_event_.Wait();
}

return !devices_.empty();

[Hzj_jie, 2016/07/11 00:55:00]

Hold on, this logic won't correct.
If the Event is auto reset. And three threads are working on Prepare function
concurrently, one of them will not be able to be released.

If the Event is manual reset. We do not have a correct place to reset the Event.
(If reset is added above TryLock may cause a recent Set to be ignored, other
threads in Wait statement won't be able to pass, even last Set statement has
been executed. If reset is added below TryLock, other threads may pass the Wait
statement before the Set statement.)

+    AutoExit exit(&initializing_);
+    rtc::CritScope lock(&lock_);
+    if (!devices_.empty()) {
+      // Another thread has initialized this instance before this thread reaches
+      // line initializing_.Enter(), so does nothing and returns true.

[Sergey Ulanov, 2016/07/08 22:36:55]

How is that possible?

[Hzj_jie, 2016/07/11 00:55:00]

Another thread has called initializing_.Exit before this thread has reached
initializing_.Enter. i.e.
Thread 1 and thread 2 are both in line 72, right before initializing_.Enter.
Thread 1 has been suspended by system. Thread 2 continually works and finish
entire Prepare function. Thread 1 resumes, and reach initializing_.Enter(). So
both threads will be able to enter initializing_.Enter, but thread 2 has just
finished initialization.

Well, I know this logic is a little bit complex, but I just want to make sure
two or more concurrent Prepare function calls won't deadlock, or do initializing
without purpose (without making one Duplicate call).

+      return true;
+    }
+
+    if (DoInitialize()) {
+      return true;
+    }
+
+    Deinitialize();
+    return false;
+  } else {
+    // Some other thread is initializing, wait for its finish and check the
+    // result.
+    while (initializing_.Hold()) {
+      rtc::CritScope lock(&lock_);
+    }
+    return !devices_.empty();

[Sergey Ulanov, 2016/07/08 22:36:54]

Here the case when initialization has failed and when |devices_| is empty are
treated the same way. I think it would be better to have a separate flag to
indicate that initialization has failed.

[Sergey Ulanov, 2016/07/08 22:36:54]

don't you need to hold the lock_ when verifying that devices_ is not empty here?

[Hzj_jie, 2016/07/11 00:55:00]

If there is no device for any reason, DxgiDuplicatorContainer and
ScreenCapturerWinDirectx won't be able to work. So I assume devices_.empty()
equals DirectX duplication APIs won't be able to work on the system. It's also
consistent with D3dDevice::EnumDevices.

[Hzj_jie, 2016/07/11 00:55:00]

Yes, I missed.

+  }
+}
+
+void DxgiDuplicatorContainer::Deinitialize() {
+  desktop_rect_ = DesktopRect();
+  duplicators_.clear();
+  devices_.clear();
+}
+
+bool DxgiDuplicatorContainer::DoInitialize() {
+  RTC_DCHECK(desktop_rect_.is_empty());
+  RTC_DCHECK(duplicators_.empty());
+  RTC_DCHECK(devices_.empty());
+
+  devices_ = D3dDevice::EnumDevices();

[Sergey Ulanov, 2016/07/08 22:36:54]

We enumerate devices only once here, but they can changed dynamically.
See
https://msdn.microsoft.com/en-us/library/windows/desktop/ff471336(v=vs.85).aspx
: "The number of adapters in a system changes when you add or remove a display
card, or dock or undock a laptop."

[Hzj_jie, 2016/07/11 00:55:00]

Yes, that's the reason we call Prepare each time before Duplicate function. i.e.
We do not have (or I have not found) a signal to indicate an adapter or monitor
has been added to the system. But if display mode changed,
IDXGIOutputDuplication::AcquireNextFrame will fail, and then we know that we
should reinitialize DxgiDuplicatorContainer.

+  if (devices_.empty()) {
+    return false;
+  }
+
+  for (size_t i = 0; i < devices_.size(); i++) {
+    duplicators_.emplace_back();
+    if (!duplicators_.back().Initialize(devices_[i])) {
+      return false;
+    }
+    if (desktop_rect_.is_empty()) {
+      desktop_rect_ = duplicators_.back().desktop_rect();
+    } else {
+      desktop_rect_.JoinWith(duplicators_.back().desktop_rect());
+    }
+  }
+
+  HDC hdc = GetDC(nullptr);
+  // Use old DPI value if failed.
+  if (hdc != nullptr) {
+    dpi_.set(GetDeviceCaps(hdc, LOGPIXELSX), GetDeviceCaps(hdc, LOGPIXELSY));

[Sergey Ulanov, 2016/07/08 22:36:55]

dpi can change dynamically, but this class won't detect it.

[Hzj_jie, 2016/07/11 00:55:00]

Same as above, we will reset the dpi_ once display mode changed. But I have no
idea how to handle dpis for different monitors. So I am still using the old GDI
way to get a dpi.

+    ReleaseDC(nullptr, hdc);
+  }
+
+  return true;
+}
+
+DesktopRect DxgiDuplicatorContainer::ScreenRect(int id) const {
+  if (id < 0) {

[Sergey Ulanov, 2016/07/08 22:36:54]

DCHECK(id >= 0)

[Hzj_jie, 2016/07/11 00:55:00]

Done.

+    return DesktopRect();
+  }
+  rtc::CritScope lock(&lock_);
+  for (size_t i = 0; i < duplicators_.size(); i++) {
+    if (id >= duplicators_[i].screen_count()) {
+      id -= duplicators_[i].screen_count();
+    } else {
+      return duplicators_[i].ScreenRect(id);
+    }
+  }
+  return DesktopRect();
+}
+
+int DxgiDuplicatorContainer::ScreenCount() const {
+  rtc::CritScope lock(&lock_);
+  int result = 0;
+  for (size_t i = 0; i < duplicators_.size(); i++) {
+    result += duplicators_[i].screen_count();
+  }
+  return result;
+}
+
+bool DxgiDuplicatorContainer::Duplicate(DesktopFrame* target,
+                                        const DesktopFrame* last_frame) {
+  RTC_DCHECK(target);
+  if (last_frame != nullptr && !target->size().equals(last_frame->size())) {
+    return false;
+  }
+  target->mutable_updated_region()->Clear();
+  rtc::CritScope lock(&lock_);
+  for (size_t i = 0; i < duplicators_.size(); i++) {
+    if (!duplicators_[i].Duplicate(target, last_frame)) {
+      Deinitialize();
+      return false;
+    }
+  }
+  target->set_dpi(dpi());
+  return true;
+}
+
+bool DxgiDuplicatorContainer::Duplicate(int id,
+                                        DesktopFrame* target,
+                                        const DesktopFrame* last_frame) {
+  RTC_DCHECK(target);
+  if (id < 0) {
+    return false;
+  }
+  if (last_frame != nullptr && !target->size().equals(last_frame->size())) {
+    return false;
+  }
+  target->mutable_updated_region()->Clear();
+  rtc::CritScope lock(&lock_);
+  for (size_t i = 0; i < duplicators_.size(); i++) {
+    if (id >= duplicators_[i].screen_count()) {
+      id -= duplicators_[i].screen_count();
+    } else {
+      if (duplicators_[i].Duplicate(id, target, last_frame)) {
+        target->set_dpi(dpi());
+        return true;
+      }
+      Deinitialize();
+      return false;
+    }
+  }
+  // id >= ScreenCount()
+  return false;
+}
+
+}  // namespace webrtc