[WebRTC] Add ScreenCapturerDifferWrapper to share Differ across ScreenCapturers

webrtc/modules/desktop_capture/differ.h

 /*
  *  Copyright (c) 2013 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.
  */
 
 #ifndef WEBRTC_MODULES_DESKTOP_CAPTURE_DIFFER_H_
 #define WEBRTC_MODULES_DESKTOP_CAPTURE_DIFFER_H_
 
 #include <memory>
 #include <vector>
 
 #include "webrtc/base/constructormagic.h"
-#include "webrtc/modules/desktop_capture/desktop_region.h"
+#include "webrtc/typedefs.h"
 
 namespace webrtc {
 
+class DesktopRect;
+class DesktopRegion;
+
 // TODO(sergeyu): Simplify differ now that we are working with DesktopRegion.
 // diff_info_ should no longer be needed, as we can put our data directly into
 // the region that we are calculating.
 // http://crbug.com/92379
 // TODO(sergeyu): Rename this class to something more sensible, e.g.
 // ScreenCaptureFrameDifferencer.
 class Differ {

[Sergey Ulanov, 2016/08/03 23:19:11]

I would prefer cleaning up this code before extending it. As the TODO above
suggests we no longer need diff_info_ array. It still makes sense to compare
macro blocks, but the result can be stored directly in DesktopRegion. 
I think you can just replace this class with a single static function: 
 void CompareFrames(const DesktopFrame& old_frame,
                    const DesktopFrame& new_frame,
                    DestkopRegion* diff_region_out).

That would make this code simpler and easier to use. WDYT?

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

I am happy with the change. But it seems like a very huge change, which impacts
various ScreenCapturer(s). I think we can reduce the change by,
1. Submit this change.
2. Using ScreenCapturerDifferWrapper for each ScreenCapturer implementation, and
removing the reference to Differ class from them.
3. Update Differ.
Then we will only need to change one place (ScreenCapturerDifferWrapper) instead
of three capturers (ScreenCapturerLinux / ScreenCapturerWin /
ScreenCapturerMac).

[Sergey Ulanov, 2016/08/08 18:42:42]

Note that ScreenCapturerMac doesn't use Differ.

How about a slightly different plan:
1. Implement ScreenCapturerDifferWrapper without using Differ, e.g. put all diff
logic directly in screen_capturer_differ_wrapper.cc
2. Remove Differ and it's usage from everywhere.

That way we can avoid wasting time working on this old code that needs to be
rewritten anyway.

  public:
   // Create a differ that operates on bitmaps with the specified width, height
   // and bytes_per_pixel.
   Differ(int width, int height, int bytes_per_pixel, int stride);
   ~Differ();
 
   int width() { return width_; }
   int height() { return height_; }
   int bytes_per_pixel() { return bytes_per_pixel_; }
   int bytes_per_row() { return bytes_per_row_; }
 
-  // Given the previous and current screen buffer, calculate the dirty region
+  // Given the previous and current screen buffer, calculates the dirty region

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

FWIW, either calculate or calculates would be appropriate here. Since the other
comments here are written as instructions rather than descriptions (eg,
"Identify all the blocks", not "Identifies all the blocks"), I'd argue that the
original was more consistent.

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

Done.

   // that encloses all of the changed pixels in the new screen.
-  void CalcDirtyRegion(const uint8_t* prev_buffer, const uint8_t* curr_buffer,
+  void CalcDirtyRegion(const uint8_t* prev_buffer,
+                       const uint8_t* curr_buffer,
                        DesktopRegion* region);
 
+  // Given the previous and current screen buffer, and a superset of dirty
+  // region as hint, calculates the dirty region that encloses all the changed
+  // pixels in the new screen.
+  void CalcDirtyRegionWithHints(const uint8_t* prev_buffer,
+                                const uint8_t* curr_buffer,
+                                const DesktopRegion& hints,
+                                DesktopRegion* region);
+
  private:
   // Allow tests to access our private parts.
   friend class DifferTest;
 
-  // Identify all of the blocks that contain changed pixels.
-  void MarkDirtyBlocks(const uint8_t* prev_buffer, const uint8_t* curr_buffer);
+  // Identify all of the blocks that contain changed pixels in the |rect|.
+  void MarkDirtyBlocks(const uint8_t* prev_buffer,
+                       const uint8_t* curr_buffer,
+                       const DesktopRect& rect);
 
   // After the dirty blocks have been identified, this routine merges adjacent
   // blocks into a region.
   // The goal is to minimize the region that covers the dirty blocks.
-  void MergeBlocks(DesktopRegion* region);
+  void MergeBlocks(DesktopRegion* region) const;

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

Optional: I'm not sure it's worth adding these consts. I don't have a strong
opinion about it, but I don't think it's something we commonly do, so it leaves
the reader wondering what's special about this class that needs it.

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

IMO, a const indeed tells the reader, this function does not need to impact the
class internal states. i.e. It impacts only the parameters.

And in Google C++ style guide, using const for functions is strongly
recommended. (https://goo.gl/sbqlAI) So does Chromium have a different
suggestion?

 
   // Checks whether the upper-left portions of the buffers are equal. The size
   // of the portion to check is specified by the |width| and |height| values.
   // Note that if we force the capturer to always return images whose width and
   // height are multiples of kBlockSize, then this will never be called.
   bool PartialBlocksEqual(const uint8_t* prev_buffer,
                           const uint8_t* curr_buffer,
                           int stride,
                           int width, int height);
 
   // Dimensions of screen.
-  int width_;
-  int height_;
+  const int width_;
+  const int height_;
 
   // Number of bytes for each pixel in source and dest bitmap.
   // (Yes, they must match.)
-  int bytes_per_pixel_;
+  const int bytes_per_pixel_;
 
   // Number of bytes in each row of the image (AKA: stride).
-  int bytes_per_row_;
+  const int bytes_per_row_;
+
+  // Dimensions and total size of diff info array.
+  const int diff_info_width_;
+  const int diff_info_height_;
+  const int diff_info_size_;
 
   // Diff information for each block in the image.
-  std::unique_ptr<bool[]> diff_info_;
-
-  // Dimensions and total size of diff info array.
-  int diff_info_width_;
-  int diff_info_height_;
-  int diff_info_size_;
+  const std::unique_ptr<bool[]> diff_info_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(Differ);
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_DESKTOP_CAPTURE_DIFFER_H_