Added image pattern comparison logic for test interface and fixture.

remoting/test/test_video_renderer.cc

Index: remoting/test/test_video_renderer.cc
diff --git a/remoting/test/test_video_renderer.cc b/remoting/test/test_video_renderer.cc
index 75dd2c9511ba614425c8230dcebcd5e82bbcb23f..4e20f625eafcba07ee0e58ce2a542c2ac3311005 100644
--- a/remoting/test/test_video_renderer.cc
+++ b/remoting/test/test_video_renderer.cc
@@ -4,6 +4,9 @@
 
 #include "remoting/test/test_video_renderer.h"
 
+#include <algorithm>
+#include <cmath>

[liaoyuke, 2015/07/08 18:58:15]

Usage:

algorithm: std::min, std::max, cmath: sqrt.

+
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/synchronization/lock.h"
@@ -15,6 +18,10 @@
 #include "remoting/proto/video.pb.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
+namespace {
+const int kBytesPerPixel = 4;
+}  // namespace
+
 namespace remoting {
 namespace test {
 
@@ -45,6 +52,35 @@ class TestVideoRenderer::Core {
       const base::Closure& image_pattern_matched_callback);
 
  private:
+  // Converts a set of RGB color to the RgbaColor format by adding the alpha
+  // values of 0x00.
+  RgbaColor ColorEncode(const int red, const int green, const int blue) const;

[joedow, 2015/07/09 03:03:43]

Why are the int values const here?  ints are passed by value so I don't think
this is very beneficial.

[liaoyuke, 2015/07/09 18:18:32]

I think you are right, they shouldn't be const

+
+  // Converts a RgbaColor instance to an array of RGBA colors.
+  scoped_ptr<int> ColorDecode(const RgbaColor& rgba_color) const;
+
+  // Returns true if |rect2| falls within |rect1|, allowing for certain amount
+  // of error specified by a fuzzy threshold.
+  bool ContainsRect(const webrtc::DesktopRect& rect1,
+                    const webrtc::DesktopRect& rect2) const;
+
+  // Expand the |accumulating_rect_| to a minimum rectangle that contains both
+  // |rect| and itself.
+  void MergeRectToAccumulatingRect(const webrtc::DesktopRect& rect);
+
+  // Update |accumulating_color_| to reflect the average color value of pixels
+  // fall within |accumulating_rect_|.
+  void UpdateAccumulatingColor();
+
+  // Compares |accumulating_color_| to |expected_color_|.
+  // Returns true if the root mean square of the errors in the R, G and B
+  // components does not exceed a fuzzy threshold.
+  bool ExpectedColorIsMatched() const;
+
+  // Returns true if |expected_rect_| and |accumulating_rect_| contains each
+  // other and expected color is matched.
+  bool ExpectedImagePatternIsMatched() const;
+
   // Used to ensure Core methods are called on the same thread.
   base::ThreadChecker thread_checker_;
 
@@ -77,6 +113,9 @@ class TestVideoRenderer::Core {
   // Used to store the callback when expected pattern is matched.
   base::Closure image_pattern_matched_callback_;
 
+  // Used to account for video frame resizing and lossy encoding.
+  const double fuzzy_threshold_ = 0.05;

[joedow, 2015/07/09 03:03:43]

Does this need to be a class member or can it just be a const like
kBytesPerPixel

[liaoyuke, 2015/07/09 18:18:32]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
@@ -91,6 +130,8 @@ TestVideoRenderer::Core::~Core() {
 
 void TestVideoRenderer::Core::Initialize() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0);
 }
 
 void TestVideoRenderer::Core::SetCodecForDecoding(
@@ -172,9 +213,26 @@ void TestVideoRenderer::Core::ProcessVideoPacket(
 
   main_task_runner_->PostTask(FROM_HERE, done);
 
-  // TODO(liaoyuke): Update |accumulating_rect_| and |accumulating_color_|, then
-  // compare to the expected image pattern to check whether the pattern is
-  // matched or not and update |image_pattern_matched| accordingly.
+  // Check to see if a image pattern matched reply is passed in.
+  if (!image_pattern_matched_callback_.is_null()) {

[joedow, 2015/07/09 03:03:43]

This be simpler if you return here if the callback is null instead of enclosing
this work in the if block if the callback is not null.

[liaoyuke, 2015/07/09 18:18:32]

Done.

+    for (webrtc::DesktopRegion::Iterator dirty_region(updated_region_);
+         !dirty_region.IsAtEnd(); dirty_region.Advance()) {
+      webrtc::DesktopRect dirty_rect = dirty_region.rect();
+
+      // Dirty rects not falling within |expected_rect_| doesn't affect the
+      // result.
+      if (ContainsRect(expected_rect_, dirty_rect)) {
+        MergeRectToAccumulatingRect(dirty_rect);
+      }
+    }
+
+    UpdateAccumulatingColor();
+    if (ExpectedImagePatternIsMatched()) {
+      image_pattern_matched_callback_.Run();
+      image_pattern_matched_callback_.Reset();

[joedow, 2015/07/09 03:03:43]

base::ResetAndReturn() can replace lines 231 and 232 (and is a good practice)

[liaoyuke, 2015/07/09 18:18:32]

Done.

+      accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0);
+    }
+  }
 }
 
 void TestVideoRenderer::Core::SetImagePatternAndMatchedCallback(
@@ -188,6 +246,96 @@ void TestVideoRenderer::Core::SetImagePatternAndMatchedCallback(
   image_pattern_matched_callback_ = image_pattern_matched_callback;
 }
 
+RgbaColor TestVideoRenderer::Core::ColorEncode(const int red,
+                                               const int green,
+                                               const int blue) const {
+  RgbaColor rgba_color = red;
+  rgba_color = (rgba_color << 8) + green;
+  rgba_color = (rgba_color << 8) + blue;
+  rgba_color = (rgba_color << 8) + 0;
+
+  return rgba_color;
+}
+
+scoped_ptr<int> TestVideoRenderer::Core::ColorDecode(
+    const RgbaColor& rgba_color) const {

[joedow, 2015/07/09 03:03:43]

If you are going to return an array, you want scoped_array<int>, otherwise the
wrong delete is going to be called when scoped_ptr goes out of scope.

+  int* rgba_colors = new int[3];
+  uint32 mask = (1 << 9) - 1;
+  rgba_colors[2] = ((rgba_color >> 8) & mask);
+  rgba_colors[1] = ((rgba_color >> 16) & mask);
+  rgba_colors[0] = ((rgba_color >> 24) & mask);
+
+  return make_scoped_ptr(rgba_colors);
+}
+
+bool TestVideoRenderer::Core::ContainsRect(

[joedow, 2015/07/09 03:03:43]

rect1 and rect2 aren't quite descriptive enough, would something like rect1 =>
candidate_rect, rect2 => template_rect work?

[liaoyuke, 2015/07/09 18:18:32]

Done.

+    const webrtc::DesktopRect& rect1,
+    const webrtc::DesktopRect& rect2) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!image_pattern_matched_callback_.is_null());

[joedow, 2015/07/09 03:03:43]

Does it matter if the callback is not null here?

[liaoyuke, 2015/07/09 18:18:32]

At first, I thought this function will only be called inside the
ProcessVideoPacket, but it seems it's not the case, it may also be used in the
debug mode.

+
+  return rect2.left() >= (1 - fuzzy_threshold_) * rect1.left() &&
+         rect2.top() >= (1 - fuzzy_threshold_) * rect1.top() &&
+         rect2.right() <= (1 + fuzzy_threshold_) * rect1.right() &&
+         rect2.bottom() <= (1 + fuzzy_threshold_) * rect1.bottom();
+}
+
+void TestVideoRenderer::Core::MergeRectToAccumulatingRect(
+    const webrtc::DesktopRect& rect) {
+  if (accumulating_rect_.width() == 0 && accumulating_rect_.height() == 0) {

[joedow, 2015/07/09 03:03:43]

if (!accumulating_rect_.width() && !accumulating_rect_.height())

no need to compare against 0.

Also, is it possible for one of those dimensions to be 0?  That seems like it
would be odd and possibly be avoided.

[liaoyuke, 2015/07/09 18:18:32]

I think it's not possible, that's why I use it as an initial state for
accumulating_rect, because DesktopRect doesn't have something like is_null().

+    accumulating_rect_ = rect;
+  } else {
+    accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(
+        std::min(accumulating_rect_.left(), rect.left()),
+        std::min(accumulating_rect_.top(), rect.top()),
+        std::max(accumulating_rect_.right(), rect.right()),
+        std::max(accumulating_rect_.bottom(), rect.bottom()));
+  }
+}
+
+void TestVideoRenderer::Core::UpdateAccumulatingColor() {
+  int red_sum = 0;
+  int green_sum = 0;
+  int blue_sum = 0;
+  for (int y = accumulating_rect_.top(); y < accumulating_rect_.bottom(); ++y) {
+    for (int x = accumulating_rect_.left(); x < accumulating_rect_.right();
+         ++x) {
+      int offset =
+          y * kBytesPerPixel * screen_size_.width() + x * kBytesPerPixel;

[joedow, 2015/07/09 03:03:43]

some parens would help with the readability here

[liaoyuke, 2015/07/09 18:18:32]

Done.

+      red_sum += buffer_->data()[offset + 2];
+      green_sum += buffer_->data()[offset + 1];

[liaoyuke, 2015/07/09 18:18:32]

Here, I should mention that the decoded frame is in BGRA format.

+      blue_sum += buffer_->data()[offset + 0];
+    }
+  }
+
+  int area = screen_size_.width() * screen_size_.height();
+  accumulating_color_ =
+      ColorEncode(red_sum / area, green_sum / area, blue_sum / area);
+}
+
+bool TestVideoRenderer::Core::ExpectedColorIsMatched() const {
+  scoped_ptr<int> decoded_expected_color = ColorDecode(expected_color_);
+  scoped_ptr<int> decoded_accumulating_color = ColorDecode(accumulating_color_);
+  double error_sum_squares = 0;
+  for (int i = 0; i < 3; i++) {
+    double expected_value =
+        static_cast<double>(decoded_expected_color.get()[i]);
+    double accumulating_value =
+        static_cast<double>(decoded_accumulating_color.get()[i]);
+    double error = expected_value - accumulating_value;
+    error /= 255.0;
+    error_sum_squares += error * error;
+  }
+
+  return sqrt(error_sum_squares / 3) < fuzzy_threshold_;
+}
+
+bool TestVideoRenderer::Core::ExpectedImagePatternIsMatched() const {
+  return ContainsRect(accumulating_rect_, expected_rect_) &&
+         ContainsRect(expected_rect_, accumulating_rect_) &&
+         ExpectedColorIsMatched();
+}
+
 TestVideoRenderer::TestVideoRenderer()
     : video_decode_thread_(
         new base::Thread("TestVideoRendererVideoDecodingThread")),