Several minor changes to ScreenCapturerWinMagnifier

webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc

 /*
  *  Copyright (c) 2014 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/screen_capturer_win_magnifier.h"
 
-#include <assert.h>
-
 #include <utility>
 
+#include "webrtc/base/checks.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/modules/desktop_capture/desktop_capture_options.h"
 #include "webrtc/modules/desktop_capture/desktop_frame.h"
 #include "webrtc/modules/desktop_capture/desktop_frame_win.h"
 #include "webrtc/modules/desktop_capture/desktop_region.h"
 #include "webrtc/modules/desktop_capture/differ.h"
 #include "webrtc/modules/desktop_capture/mouse_cursor.h"
 #include "webrtc/modules/desktop_capture/win/cursor.h"
 #include "webrtc/modules/desktop_capture/win/desktop.h"
 #include "webrtc/modules/desktop_capture/win/screen_capture_utils.h"
 #include "webrtc/system_wrappers/include/logging.h"
 
 namespace webrtc {
 
 // kMagnifierWindowClass has to be "Magnifier" according to the Magnification
 // API. The other strings can be anything.
 static LPCTSTR kMagnifierHostClass = L"ScreenCapturerWinMagnifierHost";
 static LPCTSTR kHostWindowName = L"MagnifierHost";
 static LPCTSTR kMagnifierWindowClass = L"Magnifier";
 static LPCTSTR kMagnifierWindowName = L"MagnifierWindow";
-
-Atomic32 ScreenCapturerWinMagnifier::tls_index_(TLS_OUT_OF_INDEXES);
+thread_local static ScreenCapturerWinMagnifier* kCurrent;

[Sergey Ulanov, 2016/09/19 20:53:18]

- thread_local is not listed on chromium-cpp.appspot.com, so I don't think it's
approved. 
- This is clearly not a const, so it shouldn't use const name.
- You probably need "= nullptr;" to make sure it gets initialized properly.

[Hzj_jie, 2016/09/19 22:05:23]

Emmm, a Google internal mail thread shows thread_local will eventually be the
norm. But yes, since it's not actively allowed. I will use __declspec(thread)
instead, which is MSVC specific implementation, and widely used in Chromium.

Others are updated.

[Sergey Ulanov, 2016/09/19 22:21:25]

__declspec(thread) seems used in some third-party projects, but not in chromium
itself. It's also not used in WebRTC, so I'd prefer avoiding it as well. What's
the reason we cannot keep tls_index?

 
 ScreenCapturerWinMagnifier::ScreenCapturerWinMagnifier(
     std::unique_ptr<ScreenCapturer> fallback_capturer)
     : fallback_capturer_(std::move(fallback_capturer)) {}
 
 ScreenCapturerWinMagnifier::~ScreenCapturerWinMagnifier() {
   // DestroyWindow must be called before MagUninitialize. magnifier_window_ is
   // destroyed automatically when host_window_ is destroyed.
   if (host_window_)
     DestroyWindow(host_window_);
 
   if (magnifier_initialized_)
     mag_uninitialize_func_();
 
   if (mag_lib_handle_)
     FreeLibrary(mag_lib_handle_);
 
   if (desktop_dc_)
     ReleaseDC(NULL, desktop_dc_);
 }
 
 void ScreenCapturerWinMagnifier::Start(Callback* callback) {
   assert(!callback_);
   assert(callback);
   callback_ = callback;
 
-  InitializeMagnifier();
+  if (!InitializeMagnifier()) {
+    StartFallbackCapturer();
+  }
 }
 
 void ScreenCapturerWinMagnifier::SetSharedMemoryFactory(
     std::unique_ptr<SharedMemoryFactory> shared_memory_factory) {
   shared_memory_factory_ = std::move(shared_memory_factory);
 }
 
 void ScreenCapturerWinMagnifier::Capture(const DesktopRegion& region) {
   int64_t capture_start_time_nanos = rtc::TimeNanos();
 
   queue_.MoveToNextFrame();
 
-  // Request that the system not power-down the system, or the display hardware.
-  if (!SetThreadExecutionState(ES_DISPLAY_REQUIRED | ES_SYSTEM_REQUIRED)) {
-    if (!set_thread_execution_state_failed_) {
-      set_thread_execution_state_failed_ = true;
-      LOG_F(LS_WARNING) << "Failed to make system & display power assertion: "
-                        << GetLastError();
-    }
-  }
   // Switch to the desktop receiving user input if different from the current
   // one.
   std::unique_ptr<Desktop> input_desktop(Desktop::GetInputDesktop());
   if (input_desktop.get() != NULL && !desktop_.IsSame(*input_desktop)) {
     // Release GDI resources otherwise SetThreadDesktop will fail.
     if (desktop_dc_) {
       ReleaseDC(NULL, desktop_dc_);
       desktop_dc_ = NULL;
     }
     // If SetThreadDesktop() fails, the thread is still assigned a desktop.
     // So we can continue capture screen bits, just from the wrong desktop.
     desktop_.SetThreadDesktop(input_desktop.release());
   }
 
   bool succeeded = false;
 
   // Do not try to use the magnifier if it failed before and in multi-screen
   // setup (where the API crashes sometimes).
-  if (magnifier_initialized_ && (GetSystemMetrics(SM_CMONITORS) == 1) &&
-      magnifier_capture_succeeded_) {
+  if (magnifier_initialized_ && magnifier_capture_succeeded_) {
     DesktopRect rect = GetScreenRect(current_screen_id_, current_device_key_);
     CreateCurrentFrameIfNecessary(rect.size());
 
     // CaptureImage may fail in some situations, e.g. windows8 metro mode.
     succeeded = CaptureImage(rect);
   }
 
   // Defer to the fallback capturer if magnifier capturer did not work.
   if (!succeeded) {
     LOG_F(LS_WARNING) << "Switching to the fallback screen capturer.";
     StartFallbackCapturer();
     fallback_capturer_->Capture(region);
     return;
   }
 
   const DesktopFrame* current_frame = queue_.current_frame();
   const DesktopFrame* last_frame = queue_.previous_frame();
+  DesktopRegion updated_region;
   if (last_frame && last_frame->size().equals(current_frame->size())) {
     // Make sure the differencer is set up correctly for these previous and
     // current screens.
     if (!differ_.get() || (differ_->width() != current_frame->size().width()) ||
         (differ_->height() != current_frame->size().height()) ||
         (differ_->bytes_per_row() != current_frame->stride())) {
       differ_.reset(new Differ(current_frame->size().width(),
                                current_frame->size().height(),
                                DesktopFrame::kBytesPerPixel,
                                current_frame->stride()));
     }
 
     // Calculate difference between the two last captured frames.
-    DesktopRegion region;
     differ_->CalcDirtyRegion(

[Sergey Ulanov, 2016/09/19 20:53:18]

Do we need to keep the differ here?

[Hzj_jie, 2016/09/19 22:05:23]

No, but all the differs will be removed in
https://codereview.chromium.org/2348803003/.

-        last_frame->data(), current_frame->data(), &region);
-    helper_.InvalidateRegion(region);
+        last_frame->data(), current_frame->data(), &updated_region);
   } else {
-    // No previous frame is available, or the screen is resized. Invalidate the
-    // whole screen.
-    helper_.InvalidateScreen(current_frame->size());
+    updated_region.SetRect(DesktopRect::MakeSize(current_frame->size()));
   }
 
-  helper_.set_size_most_recent(current_frame->size());
-
   // Emit the current frame.
   std::unique_ptr<DesktopFrame> frame = queue_.current_frame()->Share();
   frame->set_dpi(DesktopVector(GetDeviceCaps(desktop_dc_, LOGPIXELSX),
                                GetDeviceCaps(desktop_dc_, LOGPIXELSY)));
-  frame->mutable_updated_region()->Clear();
-  helper_.TakeInvalidRegion(frame->mutable_updated_region());
+  frame->mutable_updated_region()->Swap(&updated_region);
   frame->set_capture_time_ms((rtc::TimeNanos() - capture_start_time_nanos) /
                              rtc::kNumNanosecsPerMillisec);
   callback_->OnCaptureResult(Result::SUCCESS, std::move(frame));
 }
 
 bool ScreenCapturerWinMagnifier::GetScreenList(ScreenList* screens) {
   return webrtc::GetScreenList(screens);
 }
 
 bool ScreenCapturerWinMagnifier::SelectScreen(ScreenId id) {
@@ skipping 29 matching lines @@
     LOG_F(LS_WARNING) << "Failed to call SetWindowPos: " << GetLastError()
                       << ". Rect = {" << rect.left() << ", " << rect.top()
                       << ", " << rect.right() << ", " << rect.bottom() << "}";
     return false;
   }
 
   magnifier_capture_succeeded_ = false;
 
   RECT native_rect = {rect.left(), rect.top(), rect.right(), rect.bottom()};
 
+  kCurrent = this;
   // OnCaptured will be called via OnMagImageScalingCallback and fill in the
   // frame before set_window_source_func_ returns.
   result = set_window_source_func_(magnifier_window_, native_rect);
 
   if (!result) {
     LOG_F(LS_WARNING) << "Failed to call MagSetWindowSource: " << GetLastError()
                       << ". Rect = {" << rect.left() << ", " << rect.top()
                       << ", " << rect.right() << ", " << rect.bottom() << "}";
     return false;
   }
 
   return magnifier_capture_succeeded_;
 }
 
 BOOL ScreenCapturerWinMagnifier::OnMagImageScalingCallback(
     HWND hwnd,
     void* srcdata,
     MAGIMAGEHEADER srcheader,
     void* destdata,
     MAGIMAGEHEADER destheader,
     RECT unclipped,
     RECT clipped,
     HRGN dirty) {
-  assert(tls_index_.Value() != static_cast<int32_t>(TLS_OUT_OF_INDEXES));
-
-  ScreenCapturerWinMagnifier* owner =
-      reinterpret_cast<ScreenCapturerWinMagnifier*>(
-          TlsGetValue(tls_index_.Value()));
-
-  owner->OnCaptured(srcdata, srcheader);
-
+  RTC_DCHECK(kCurrent != nullptr);

[Sergey Ulanov, 2016/09/19 20:53:18]

Please replace all asserts with RTC_DCHECK

[Hzj_jie, 2016/09/19 22:05:23]

Done.

+  kCurrent->OnCaptured(srcdata, srcheader);
   return TRUE;
 }
 
 bool ScreenCapturerWinMagnifier::InitializeMagnifier() {
   assert(!magnifier_initialized_);
+  if (GetSystemMetrics(SM_CMONITORS) != 1) {

[Sergey Ulanov, 2016/09/19 20:53:18]

I think it's better to keep this check in ScreenCapturerWinMagnifier::Capture().
Number of monitors can change dynamically while the capturer is active.

[Hzj_jie, 2016/09/19 22:05:23]

Good point. Updated.

+    // Do not try to use the magnifier if it failed before and in multi-screen
+    // setup (where the API crashes sometimes).
+    return false;
+  }
 
   desktop_dc_ = GetDC(nullptr);
 
   mag_lib_handle_ = LoadLibrary(L"Magnification.dll");
   if (!mag_lib_handle_)
     return false;
 
   // Initialize Magnification API function pointers.
   mag_initialize_func_ = reinterpret_cast<MagInitializeFunc>(
       GetProcAddress(mag_lib_handle_, "MagInitialize"));
@@ skipping 89 matching lines @@
         magnifier_window_, MW_FILTERMODE_EXCLUDE, 1, &excluded_window_);
     if (!result) {
       mag_uninitialize_func_();
       LOG_F(LS_WARNING) << "Failed to initialize ScreenCapturerWinMagnifier: "
                         << "error from MagSetWindowFilterList "
                         << GetLastError();
       return false;
     }
   }
 
-  if (tls_index_.Value() == static_cast<int32_t>(TLS_OUT_OF_INDEXES)) {
-    // More than one threads may get here at the same time, but only one will
-    // write to tls_index_ using CompareExchange.
-    DWORD new_tls_index = TlsAlloc();
-    if (!tls_index_.CompareExchange(new_tls_index, TLS_OUT_OF_INDEXES))
-      TlsFree(new_tls_index);
-  }
-
-  assert(tls_index_.Value() != static_cast<int32_t>(TLS_OUT_OF_INDEXES));
-  TlsSetValue(tls_index_.Value(), this);
-
   magnifier_initialized_ = true;
   return true;
 }
 
 void ScreenCapturerWinMagnifier::OnCaptured(void* data,
                                             const MAGIMAGEHEADER& header) {
   DesktopFrame* current_frame = queue_.current_frame();
 
   // Verify the format.
   // TODO(jiayl): support capturing sources with pixel formats other than RGBA.
@@ skipping 41 matching lines @@
   assert(fallback_capturer_);
   if (!fallback_capturer_started_) {
     fallback_capturer_started_ = true;
 
     fallback_capturer_->Start(callback_);
     fallback_capturer_->SelectScreen(current_screen_id_);
   }
 }
 
 }  // namespace webrtc