[WebRTC] Add ScreenCapturerDifferWrapper to share Differ across ScreenCapturers

webrtc/modules/desktop_capture/differ.cc

Index: webrtc/modules/desktop_capture/differ.cc
diff --git a/webrtc/modules/desktop_capture/differ.cc b/webrtc/modules/desktop_capture/differ.cc
index 8140e612a1008c0359237904958779f9c4e795fe..9ddfe1b63c4891d133e4a70dbf067c30bdd9492a 100644
--- a/webrtc/modules/desktop_capture/differ.cc
+++ b/webrtc/modules/desktop_capture/differ.cc
@@ -12,50 +12,89 @@
 
 #include "string.h"
 
+#include "webrtc/base/checks.h"
+#include "webrtc/modules/desktop_capture/desktop_region.h"
+#include "webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "webrtc/modules/desktop_capture/differ_block.h"
 #include "webrtc/system_wrappers/include/logging.h"
 
 namespace webrtc {
 
-Differ::Differ(int width, int height, int bpp, int stride) {
-  // Dimensions of screen.
-  width_ = width;
-  height_ = height;
-  bytes_per_pixel_ = bpp;
-  bytes_per_row_ = stride;
-
-  // Calc number of blocks (full and partial) required to cover entire image.
-  // One additional row/column is added as a boundary on the right & bottom.
-  diff_info_width_ = ((width_ + kBlockSize - 1) / kBlockSize) + 1;
-  diff_info_height_ = ((height_ + kBlockSize - 1) / kBlockSize) + 1;
-  diff_info_size_ = diff_info_width_ * diff_info_height_ * sizeof(bool);
-  diff_info_.reset(new bool[diff_info_size_]);
-}
+Differ::Differ(int width, int height, int bpp, int stride) :
+    // Dimensions of screen.
+    width_(width),
+    height_(height),
+    bytes_per_pixel_(bpp),
+    bytes_per_row_(stride),
+    // Calc number of blocks (full and partial) required to cover entire image.
+    // One additional row/column is added as a boundary on the right & bottom.
+    diff_info_width_(((width_ + kBlockSize - 1) / kBlockSize) + 1),
+    diff_info_height_(((height_ + kBlockSize - 1) / kBlockSize) + 1),
+    diff_info_size_(diff_info_width_ * diff_info_height_ * sizeof(bool)),
+    diff_info_(new bool[diff_info_size_]) {}
 
 Differ::~Differ() {}
 
 void Differ::CalcDirtyRegion(const uint8_t* prev_buffer,
                              const uint8_t* curr_buffer,
                              DesktopRegion* region) {

[Jamie, 2016/08/03 23:52:25]

Maybe just call through to CalcDirtyRegionWithHints() with a full-screen region
to save duplicating the few additional lines here?

[Hzj_jie, 2016/08/04 02:28:07]

If you do not have concern with the unnecessary for-loop in WithHints function,
I am happy to do so.

+  // Reset internal dirty state to false.
+  memset(diff_info_.get(), 0, diff_info_size_);
+
   // Identify all the blocks that contain changed pixels.
-  MarkDirtyBlocks(prev_buffer, curr_buffer);
+  MarkDirtyBlocks(prev_buffer,
+                  curr_buffer,
+                  DesktopRect::MakeWH(width_, height_));
 
   // Now that we've identified the blocks that have changed, merge adjacent
   // blocks to minimize the number of rects that we return.
   MergeBlocks(region);
 }
 
-void Differ::MarkDirtyBlocks(const uint8_t* prev_buffer,
-                             const uint8_t* curr_buffer) {
+void Differ::CalcDirtyRegionWithHints(const uint8_t* prev_buffer,
+                                      const uint8_t* curr_buffer,
+                                      const DesktopRegion& hints,
+                                      DesktopRegion* region) {
+  // Reset internal dirty state to false.
   memset(diff_info_.get(), 0, diff_info_size_);
 
-  // Calc number of full blocks.
-  int x_full_blocks = width_ / kBlockSize;
-  int y_full_blocks = height_ / kBlockSize;
+  // Identify all the blocks that contain changed pixels.
+  for (auto it = DesktopRegion::Iterator(hints);
+       !it.IsAtEnd();
+       it.Advance()) {
+    MarkDirtyBlocks(prev_buffer, curr_buffer, it.rect());
+  }
+
+  // Now that we've identified the blocks that have changed, merge adjacent
+  // blocks to minimize the number of rects that we return.
+  MergeBlocks(region);
+}
+
+void Differ::MarkDirtyBlocks(const uint8_t* prev_buffer,
+                             const uint8_t* curr_buffer,
+                             const DesktopRect& rect) {
+  RTC_DCHECK(rect.width() <= width_);
+  RTC_DCHECK(rect.height() <= height_);
+
+  if (rect.is_empty()) {
+    return;
+  }
+
+  // Calculate the range of blocks.

[Jamie, 2016/08/03 23:52:25]

Since right and bottom are both past-the-end pointers, can't you simplify this
to just:

  x_end = (rect.right() - 1) / kBlockSize;

Then you don't need the conditionals. Also, please make sure you include tests
for changes on or close to a block boundary since this sort of thing is prone to
off-by-one errors.

[Hzj_jie, 2016/08/04 02:28:07]

This seems not correct to me,
Case 1: considering rect.right() is 1000, kBlockSize is 32, width_ is 1024.
Current logic will return x_end = 32. Changing to (rect.right() - 1) /
kBlockSize, will return x_end = 31.
Case 2: considering rect.right() is 1025, kBlockSize is 32, width_ is 1025.
Current logic will return x_end = 32. Changing to (rect.right() - 1) /
kBlockSize, will also return x_end = 32.

But in both cases, x_end is expected to be 32. As it indicates a set of blocks
to cover the |rect|, but leave part of them as partial column width / row
height. i.e. partial_column_width and partial_row_height should be always
smaller than kBlockSize.

I have added two RTC_DCHECK after partial_column_width / partial_row_height
assignments to ensure this conclusion is always correct.

+  int x_start = rect.left() / kBlockSize;
+  int y_start = rect.top() / kBlockSize;
+  int x_end = (rect.right() + kBlockSize - 1) / kBlockSize;
+  int y_end = (rect.bottom() + kBlockSize - 1) / kBlockSize;
+  if (x_end * kBlockSize > width_) {
+    x_end--;
+  }
+  if (y_end * kBlockSize > height_) {
+    y_end--;
+  }
 
   // Calc size of partial blocks which may be present on right and bottom edge.
-  int partial_column_width = width_ - (x_full_blocks * kBlockSize);
-  int partial_row_height = height_ - (y_full_blocks * kBlockSize);
+  int partial_column_width = rect.right() - (x_end * kBlockSize);
+  int partial_row_height = rect.bottom() - (y_end * kBlockSize);
 
   // Offset from the start of one block-column to the next.
   int block_x_offset = bytes_per_pixel_ * kBlockSize;
@@ -64,16 +103,19 @@ void Differ::MarkDirtyBlocks(const uint8_t* prev_buffer,
   // Offset from the start of one diff_info row to the next.
   int diff_info_stride = diff_info_width_ * sizeof(bool);
 
-  const uint8_t* prev_block_row_start = prev_buffer;
-  const uint8_t* curr_block_row_start = curr_buffer;
-  bool* diff_info_row_start = diff_info_.get();
+  const uint8_t* prev_block_row_start =
+      prev_buffer + y_start * block_y_stride + x_start * block_x_offset;
+  const uint8_t* curr_block_row_start =
+      curr_buffer + (prev_block_row_start - prev_buffer);
+  bool* diff_info_row_start =
+      diff_info_.get() + y_start * diff_info_width_ + x_start;
 
-  for (int y = 0; y < y_full_blocks; y++) {
+  for (int y = y_start; y < y_end; y++) {
     const uint8_t* prev_block = prev_block_row_start;
     const uint8_t* curr_block = curr_block_row_start;
     bool* diff_info = diff_info_row_start;
 
-    for (int x = 0; x < x_full_blocks; x++) {
+    for (int x = x_start; x < x_end; x++) {
       // Mark this block as being modified so that it gets incorporated into
       // a dirty rect.
       *diff_info = BlockDifference(prev_block, curr_block, bytes_per_row_);
@@ -84,7 +126,7 @@ void Differ::MarkDirtyBlocks(const uint8_t* prev_buffer,
 
     // If there is a partial column at the end, handle it.
     // This condition should rarely, if ever, occur.

[Jamie, 2016/08/03 23:52:25]

I suspect this is no longer true; it's rare when talking about the full screen
because of typical screen widths, but damage rectangles can be arbitrary.

[Hzj_jie, 2016/08/04 02:28:06]

We always try our best to cover the damage rectangles by full blocks, as full
block comparison is faster. So partial_column_width > 0 should still be rarely,
only at the right and bottom edge of the screen.

But I think the comment to explain whether it's rarely or not is not so useful.
We always need to handle it.

-    if (partial_column_width != 0) {
+    if (partial_column_width > 0) {
       *diff_info = !PartialBlocksEqual(prev_block, curr_block, bytes_per_row_,
                                        partial_column_width, kBlockSize);
       diff_info += sizeof(bool);
@@ -99,19 +141,18 @@ void Differ::MarkDirtyBlocks(const uint8_t* prev_buffer,
   // If the screen height is not a multiple of the block size, then this
   // handles the last partial row. This situation is far more common than the
   // 'partial column' case.

[Jamie, 2016/08/03 23:52:25]

As above, this second sentence can be removed now.

[Hzj_jie, 2016/08/04 02:28:06]

Done.

-  if (partial_row_height != 0) {
+  if (partial_row_height > 0) {
     const uint8_t* prev_block = prev_block_row_start;
     const uint8_t* curr_block = curr_block_row_start;
     bool* diff_info = diff_info_row_start;
-    for (int x = 0; x < x_full_blocks; x++) {
-      *diff_info = !PartialBlocksEqual(prev_block, curr_block,
-                                       bytes_per_row_,
+    for (int x = x_start; x < x_end; x++) {
+      *diff_info = !PartialBlocksEqual(prev_block, curr_block, bytes_per_row_,
                                        kBlockSize, partial_row_height);
       prev_block += block_x_offset;
       curr_block += block_x_offset;
       diff_info += sizeof(bool);
     }
-    if (partial_column_width != 0) {
+    if (partial_column_width > 0) {
       *diff_info = !PartialBlocksEqual(prev_block, curr_block, bytes_per_row_,
                                        partial_column_width,
                                        partial_row_height);
@@ -123,6 +164,7 @@ void Differ::MarkDirtyBlocks(const uint8_t* prev_buffer,
 bool Differ::PartialBlocksEqual(const uint8_t* prev_buffer,
                                 const uint8_t* curr_buffer,
                                 int stride, int width, int height) {
+  RTC_DCHECK(width > 0 && height > 0);
   int width_bytes = width * bytes_per_pixel_;
   for (int y = 0; y < height; y++) {
     if (memcmp(prev_buffer, curr_buffer, width_bytes) != 0)
@@ -133,7 +175,7 @@ bool Differ::PartialBlocksEqual(const uint8_t* prev_buffer,
   return true;
 }
 
-void Differ::MergeBlocks(DesktopRegion* region) {
+void Differ::MergeBlocks(DesktopRegion* region) const {
   region->Clear();
 
   bool* diff_info_row_start = diff_info_.get();