DirectX capturer may crash after switching shared screen

webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc

Index: webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
diff --git a/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc b/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
index 04f466455fe11aa525c487e0d97ae38e6283d91b..60a18c813a7a5c8e636fd600a1368a7e1b0f1902 100644
--- a/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
+++ b/webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc
@@ -57,12 +57,6 @@ Rotation DxgiRotationToRotation(DXGI_MODE_ROTATION rotation) {
   return Rotation::CLOCK_WISE_0;
 }
 
-// Translates |rect| with the reverse of |offset|
-DesktopRect ReverseTranslate(DesktopRect rect, DesktopVector offset) {
-  rect.Translate(-offset.x(), -offset.y());
-  return rect;
-}
-
 }  // namespace
 
 DxgiOutputDuplicator::DxgiOutputDuplicator(const D3dDevice& device,
@@ -132,8 +126,7 @@ bool DxgiOutputDuplicator::DuplicateOutput() {
   }
 
   rotation_ = DxgiRotationToRotation(desc_.Rotation);
-  unrotated_size_ =
-      RotateSize(desktop_rect_.size(), ReverseRotation(rotation_));
+  unrotated_size_ = RotateSize(desktop_size(), ReverseRotation(rotation_));
 
   return true;
 }
@@ -175,37 +168,45 @@ bool DxgiOutputDuplicator::Duplicate(Context* context,
 
   // We need to merge updated region with the one from context, but only spread
   // updated region from current frame. So keeps a copy of updated region from
-  // context here.
+  // context here. The |updated_region| always starts from (0, 0).
   DesktopRegion updated_region;
   updated_region.Swap(&context->updated_region);
   if (error.Error() == S_OK &&
       frame_info.AccumulatedFrames > 0 &&
       resource) {
-    DetectUpdatedRegion(frame_info, offset, &context->updated_region);
+    DetectUpdatedRegion(frame_info, &context->updated_region);
+    SpreadContextChange(context);
     if (!texture_->CopyFrom(frame_info, resource.Get())) {
       return false;
     }
-    SpreadContextChange(context);
     updated_region.AddRegion(context->updated_region);
+    // TODO(zijiehe): Why clearing context->updated_region() here triggers

[Sergey Ulanov, 2017/04/05 19:47:57]

nit: s/Why/Why does/ and s/triggers/trigger/. Or alternatively make this a
statement instead of a question, e.g. "Figure out why ..."

[Hzj_jie, 2017/04/05 21:16:59]

Done.

+    // screen flickering?
 
     const DesktopFrame& source = texture_->AsDesktopFrame();
     if (rotation_ != Rotation::CLOCK_WISE_0) {
       for (DesktopRegion::Iterator it(updated_region); !it.IsAtEnd();
            it.Advance()) {
-        const DesktopRect source_rect =
-            RotateRect(ReverseTranslate(it.rect(), offset),
-                       desktop_rect().size(), ReverseRotation(rotation_));
+        // The |updated_region| returned by Windows is rotated, but the |source|
+        // frame is not. So we need to rotate it reversely.
+        const DesktopRect source_rect = RotateRect(
+            it.rect(), desktop_size(), ReverseRotation(rotation_));
         RotateDesktopFrame(source, source_rect, rotation_, offset, target);
       }
     } else {
       for (DesktopRegion::Iterator it(updated_region); !it.IsAtEnd();
            it.Advance()) {
-        target->CopyPixelsFrom(
-            source, ReverseTranslate(it.rect(), offset).top_left(), it.rect());
+        // The DesktopRect in |source|, starts from (0, 0).
+        const DesktopRect source_rect = it.rect();

[Sergey Ulanov, 2017/04/05 19:47:57]

Maybe remove this var and just use it.rect() below?

[Hzj_jie, 2017/04/05 21:16:58]

Definitely.

+        // The DesktopRect in |target|, starts from offset.
+        DesktopRect dest_rect = source_rect;
+        dest_rect.Translate(offset);
+        target->CopyPixelsFrom(source, source_rect.top_left(), dest_rect);
       }
     }
     last_frame_ = target->Share();
     last_frame_offset_ = offset;
+    updated_region.Translate(offset.x(), offset.y());
     target->mutable_updated_region()->AddRegion(updated_region);
     num_frames_captured_++;
     return texture_->Release() && ReleaseFrame();
@@ -216,8 +217,15 @@ bool DxgiOutputDuplicator::Duplicate(Context* context,
     // export last frame to the target.
     for (DesktopRegion::Iterator it(updated_region); !it.IsAtEnd();
          it.Advance()) {
-      target->CopyPixelsFrom(*last_frame_, it.rect().top_left(), it.rect());
+      // The DesktopRect in |source|, starts from last_frame_offset_.
+      DesktopRect source_rect = it.rect();
+      // The DesktopRect in |target|, starts from offset.
+      DesktopRect target_rect = source_rect;
+      source_rect.Translate(last_frame_offset_);
+      target_rect.Translate(offset);
+      target->CopyPixelsFrom(*last_frame_, source_rect.top_left(), target_rect);
     }
+    updated_region.Translate(offset.x(), offset.y());
     target->mutable_updated_region()->AddRegion(updated_region);
   } else {
     // If we were at the very first frame, and capturing failed, the
@@ -229,23 +237,26 @@ bool DxgiOutputDuplicator::Duplicate(Context* context,
   return error.Error() == DXGI_ERROR_WAIT_TIMEOUT || ReleaseFrame();
 }
 
-DesktopRect DxgiOutputDuplicator::TranslatedDesktopRect(DesktopVector offset) {
-  DesktopRect result(DesktopRect::MakeSize(desktop_rect_.size()));
+DesktopRect DxgiOutputDuplicator::TranslatedDesktopRect(

[Sergey Ulanov, 2017/04/05 19:47:57]

GetTranslatedDesktopRect()?

[Hzj_jie, 2017/04/05 21:16:59]

Done.
Also changed UntranslatedDesktopRect() into GetUntranslatedDesktopRect().

+    DesktopVector offset) const {
+  DesktopRect result(DesktopRect::MakeSize(desktop_size()));
   result.Translate(offset);
   return result;
 }
 
+DesktopRect DxgiOutputDuplicator::UntranslatedDesktopRect() const {

[Sergey Ulanov, 2017/04/05 19:47:57]

desktop_rect()?

[Hzj_jie, 2017/04/05 21:16:59]

It may be too confusing, IMO.

+  return TranslatedDesktopRect(DesktopVector());

[Sergey Ulanov, 2017/04/05 19:47:57]

I think just "DesktopRect::MakeSize(desktop_size())" would be more readable.

[Hzj_jie, 2017/04/05 21:16:58]

Done.

+}
+
 void DxgiOutputDuplicator::DetectUpdatedRegion(
     const DXGI_OUTDUPL_FRAME_INFO& frame_info,
-    DesktopVector offset,
     DesktopRegion* updated_region) {
   if (DoDetectUpdatedRegion(frame_info, updated_region)) {
-    updated_region->Translate(offset.x(), offset.y());
     // Make sure even a region returned by Windows API is out of the scope of
     // desktop_rect_, we still won't export it to the target DesktopFrame.
-    updated_region->IntersectWith(TranslatedDesktopRect(offset));
+    updated_region->IntersectWith(UntranslatedDesktopRect());
   } else {
-    updated_region->SetRect(TranslatedDesktopRect(offset));
+    updated_region->SetRect(UntranslatedDesktopRect());
   }
 }
 
@@ -340,7 +351,7 @@ bool DxgiOutputDuplicator::DoDetectUpdatedRegion(
 void DxgiOutputDuplicator::Setup(Context* context) {
   RTC_DCHECK(context->updated_region.is_empty());
   // Always copy entire monitor during the first Duplicate() function call.
-  context->updated_region.AddRect(desktop_rect_);
+  context->updated_region.AddRect(UntranslatedDesktopRect());
   RTC_DCHECK(std::find(contexts_.begin(), contexts_.end(), context) ==
              contexts_.end());
   contexts_.push_back(context);
@@ -361,6 +372,10 @@ void DxgiOutputDuplicator::SpreadContextChange(const Context* const source) {
   }
 }
 
+DesktopSize DxgiOutputDuplicator::desktop_size() const {
+  return desktop_rect_.size();
+}
+
 int64_t DxgiOutputDuplicator::num_frames_captured() const {
 #if !defined(NDEBUG)
   RTC_DCHECK_EQ(!!last_frame_, num_frames_captured_ > 0);