Use std::unique_ptr<> to pass frame ownership in DesktopCapturer impls.

webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc

Index: webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc
diff --git a/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc b/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc
index 9df2e5fc9b2c1c0e8d9879c7f649caef65a57d42..dca825ba8bf22581e2d7121df1c8843527aa9e88 100644
--- a/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc
+++ b/webrtc/modules/desktop_capture/win/screen_capturer_win_gdi.cc
@@ -39,14 +39,8 @@ const wchar_t kDwmapiLibraryName[] = L"dwmapi.dll";
 
 }  // namespace
 
-ScreenCapturerWinGdi::ScreenCapturerWinGdi(const DesktopCaptureOptions& options)
-    : callback_(NULL),
-      current_screen_id_(kFullDesktopScreenId),
-      desktop_dc_(NULL),
-      memory_dc_(NULL),
-      dwmapi_library_(NULL),
-      composition_func_(NULL),
-      set_thread_execution_state_failed_(false) {
+ScreenCapturerWinGdi::ScreenCapturerWinGdi(
+    const DesktopCaptureOptions& options) {
   if (options.disable_effects()) {
     // Load dwmapi.dll dynamically since it is not available on XP.
     if (!dwmapi_library_)
@@ -61,7 +55,7 @@ ScreenCapturerWinGdi::ScreenCapturerWinGdi(const DesktopCaptureOptions& options)
 
 ScreenCapturerWinGdi::~ScreenCapturerWinGdi() {
   if (desktop_dc_)
-    ReleaseDC(NULL, desktop_dc_);
+    ReleaseDC(nullptr, desktop_dc_);

[Wez, 2016/05/18 01:29:50]

As per header comment, technically this should remain NULL.

[Sergey Ulanov, 2016/05/31 12:02:49]

Done.

   if (memory_dc_)
     DeleteDC(memory_dc_);
 
@@ -97,7 +91,7 @@ void ScreenCapturerWinGdi::Capture(const DesktopRegion& region) {
   PrepareCaptureResources();
 
   if (!CaptureImage()) {
-    callback_->OnCaptureCompleted(NULL);
+    callback_->OnCaptureCompleted(std::unique_ptr<DesktopFrame>());
     return;
   }
 
@@ -170,16 +164,16 @@ void ScreenCapturerWinGdi::PrepareCaptureResources() {
   // 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)) {
+  if (input_desktop && !desktop_.IsSame(*input_desktop)) {
     // Release GDI resources otherwise SetThreadDesktop will fail.
     if (desktop_dc_) {
-      ReleaseDC(NULL, desktop_dc_);
-      desktop_dc_ = NULL;
+      ReleaseDC(nullptr, desktop_dc_);
+      desktop_dc_ = nullptr;
     }
 
     if (memory_dc_) {
       DeleteDC(memory_dc_);
-      memory_dc_ = NULL;
+      memory_dc_ = nullptr;
     }
 
     // If SetThreadDesktop() fails, the thread is still assigned a desktop.
@@ -188,7 +182,7 @@ void ScreenCapturerWinGdi::PrepareCaptureResources() {
 
     // Re-assert our vote to disable Aero.
     // See crbug.com/124018 and crbug.com/129906.
-    if (composition_func_ != NULL) {
+    if (composition_func_) {
       (*composition_func_)(DWM_EC_DISABLECOMPOSITION);
     }
   }
@@ -202,21 +196,21 @@ void ScreenCapturerWinGdi::PrepareCaptureResources() {
       GetSystemMetrics(SM_CYVIRTUALSCREEN)));
   if (!screen_rect.equals(desktop_dc_rect_)) {
     if (desktop_dc_) {
-      ReleaseDC(NULL, desktop_dc_);
-      desktop_dc_ = NULL;
+      ReleaseDC(nullptr, desktop_dc_);
+      desktop_dc_ = nullptr;
     }
     if (memory_dc_) {
       DeleteDC(memory_dc_);
-      memory_dc_ = NULL;
+      memory_dc_ = nullptr;
     }
     desktop_dc_rect_ = DesktopRect();
   }
 
-  if (desktop_dc_ == NULL) {
-    assert(memory_dc_ == NULL);
+  if (!desktop_dc_) {
+    assert(!memory_dc_);
 
     // Create GDI device contexts to capture from the desktop into memory.
-    desktop_dc_ = GetDC(NULL);
+    desktop_dc_ = GetDC(nullptr);
     if (!desktop_dc_)
       abort();

[Wez, 2016/05/18 01:29:50]

How is if (!foo) abort() different from assert(foo)?

[Sergey Ulanov, 2016/05/31 12:02:49]

assert() is enabled only in debug builds.

capturers need to be updated to use RTC_[D]CHECK() instead of assert()/abort(),
but I'd prefer doing it in a separate CL.

[Wez, 2016/06/01 21:29:05]

Acknowledged.

     memory_dc_ = CreateCompatibleDC(desktop_dc_);
@@ -244,14 +238,14 @@ bool ScreenCapturerWinGdi::CaptureImage() {
   // may still be reading from them.
   if (!queue_.current_frame() ||
       !queue_.current_frame()->size().equals(screen_rect.size())) {
-    assert(desktop_dc_ != NULL);
-    assert(memory_dc_ != NULL);
+    assert(desktop_dc_);
+    assert(memory_dc_);

[Wez, 2016/05/18 01:29:50]

Why assert here, given that we already check that they're non-null earlier on?

[Sergey Ulanov, 2016/05/31 12:02:49]

It's not checked in this function, so I think this asserts are useful.

[Wez, 2016/06/01 21:29:05]

Acknowledged.

 
-    std::unique_ptr<DesktopFrame> buffer(DesktopFrameWin::Create(
-        size, shared_memory_factory_.get(), desktop_dc_));
+    std::unique_ptr<DesktopFrame> buffer = DesktopFrameWin::Create(
+        size, shared_memory_factory_.get(), desktop_dc_);
     if (!buffer)
       return false;
-    queue_.ReplaceCurrentFrame(SharedDesktopFrame::Wrap(buffer.release()));
+    queue_.ReplaceCurrentFrame(SharedDesktopFrame::Wrap(std::move(buffer)));
   }
 
   // Select the target bitmap into the memory dc and copy the rect from desktop
@@ -259,11 +253,9 @@ bool ScreenCapturerWinGdi::CaptureImage() {
   DesktopFrameWin* current = static_cast<DesktopFrameWin*>(
       queue_.current_frame()->GetUnderlyingFrame());
   HGDIOBJ previous_object = SelectObject(memory_dc_, current->bitmap());
-  if (previous_object != NULL) {
-    BitBlt(memory_dc_,
-           0, 0, screen_rect.width(), screen_rect.height(),
-           desktop_dc_,
-           screen_rect.left(), screen_rect.top(),
+  if (previous_object) {
+    BitBlt(memory_dc_, 0, 0, screen_rect.width(), screen_rect.height(),
+           desktop_dc_, screen_rect.left(), screen_rect.top(),
            SRCCOPY | CAPTUREBLT);
 
     // Select back the previously selected object to that the device contect