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..d97442bbe883b6b9f9e9d9a5cd7918c864c6240e 100644
--- a/remoting/test/test_video_renderer.cc
+++ b/remoting/test/test_video_renderer.cc
@@ -4,7 +4,10 @@
 
 #include "remoting/test/test_video_renderer.h"
 
+#include <cmath>
+
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/synchronization/lock.h"
 #include "base/thread_task_runner_handle.h"
@@ -15,6 +18,20 @@
 #include "remoting/proto/video.pb.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
+// RGB Color triplets, and it can be converted to and from RGBA32.

[joedow, 2015/07/10 23:39:16]

This comment is a little confusing, I don't think you need the struct name in
the comment and also, how does the conversion occur (casting?)

[liaoyuke, 2015/07/13 16:05:38]

Acknowledged.

+struct RGBTriplets {

[joedow, 2015/07/10 23:39:15]

Would RgbValue be a better name here?  If we care about alpha in the future, I'm
assuming we wouldn't have an RGBAQuartet (vs. RgbaValue)

[liaoyuke, 2015/07/13 16:05:38]

Yes, and maybe I should move the convertion functions to here.

+  RGBTriplets(int r, int g, int b) : red(r), green(g), blue(b) {}
+  int red;
+  int green;
+  int blue;
+};

[joedow, 2015/07/10 23:39:16]

This struct should be in the anonymous namespace below

[liaoyuke, 2015/07/13 16:05:38]

Done.

+
+namespace {
+// Used to account for video frame resizing and lossy encoding, and the error is
+// in percent.

[joedow, 2015/07/10 23:39:16]

nit: Can you condense this comment so it fits on one line?

[liaoyuke, 2015/07/13 16:05:38]

Done.

+const double kMaxColorError = 0.02;
+}  // namespace
+

[joedow, 2015/07/10 23:39:15]

no need for an end "// namespace" comment when the namespace block is so short
(pretty sure you shouldn't get a lint error)

[liaoyuke, 2015/07/13 16:05:38]

I actually do get a lint error: "Anonymous namespace should be terminated with
"// namespace"  [readability/namespace] [5]"

 namespace remoting {
 namespace test {
 
@@ -34,17 +51,33 @@ class TestVideoRenderer::Core {
   // Initialize a decoder to decode video packets.
   void SetCodecForDecoding(const protocol::ChannelConfig::Codec codec);
 
-  // Returns a copy of the current buffer.
-  scoped_ptr<webrtc::DesktopFrame> GetBufferForTest() const;
+  // Returns a copy of the current frame.
+  scoped_ptr<webrtc::DesktopFrame> GetFrameForTest() const;
 
   // Set expected image pattern for comparison and the callback will be called
   // when the pattern is matched.
   void SetImagePatternAndMatchedCallback(
       const webrtc::DesktopRect& expected_rect,
-      const RgbaColor& expected_color,
+      const RGBA32& expected_avg_color,

[joedow, 2015/07/10 23:39:16]

Does this need to be a const ref since it is just a 32bit uint?

[liaoyuke, 2015/07/13 16:05:38]

Done.

       const base::Closure& image_pattern_matched_callback);
 
+  // Convert a RGB triplets to 32 bit RGBA.
+  static RGBA32 ConvertTripletsToRGBA32(const RGBTriplets& rgb_triplets);
+
+  // Convert a 32 bit RGBA to a RGB triplets.
+  static RGBTriplets ConvertRGBA32ToTriplets(RGBA32 color);

[joedow, 2015/07/10 23:39:15]

Do these need to be static or can they live in the anonymous namespace above
(with the RGBTriplets struct)

[liaoyuke, 2015/07/13 16:05:38]

Yes, I think so, since they are only used with RGBValue struct.

+
  private:
+  // Returns average color of pixels fall within |rect| on the current frame.
+  RGBTriplets CalculateAverageColorTriplets(
+      const webrtc::DesktopRect& rect) const;
+
+  // Compares |candidate_avg_triplets| to |expected_avg_color_|.
+  // Returns true if the root mean square of the errors in the R, G and B
+  // components does not exceed a given limit.
+  bool ExpectedColorIsMatched(const RGBTriplets& candidate_avg_triplets,
+                              double error_limit) const;
+
   // Used to ensure Core methods are called on the same thread.
   base::ThreadChecker thread_checker_;
 
@@ -61,18 +94,14 @@ class TestVideoRenderer::Core {
   scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 
   // Used to store decoded video frame.
-  scoped_ptr<webrtc::DesktopFrame> buffer_;
+  scoped_ptr<webrtc::DesktopFrame> frame_;
 
-  // Protects access to |buffer_|.
+  // Protects access to |frame_|.
   mutable base::Lock lock_;
 
   // Used to store the expected image pattern.
   webrtc::DesktopRect expected_rect_;
-  RgbaColor expected_color_;
-
-  // Maintains accumulating image pattern.
-  webrtc::DesktopRect accumulating_rect_;
-  RgbaColor accumulating_color_;
+  RGBA32 expected_avg_color_;
 
   // Used to store the callback when expected pattern is matched.
   base::Closure image_pattern_matched_callback_;
@@ -123,11 +152,11 @@ void TestVideoRenderer::Core::SetCodecForDecoding(
   }
 }
 
-scoped_ptr<webrtc::DesktopFrame>
-    TestVideoRenderer::Core::GetBufferForTest() const {
+scoped_ptr<webrtc::DesktopFrame> TestVideoRenderer::Core::GetFrameForTest()
+    const {
   base::AutoLock auto_lock(lock_);
-  DCHECK(buffer_);
-  return make_scoped_ptr(webrtc::BasicDesktopFrame::CopyOf(*buffer_.get()));
+  DCHECK(frame_);
+  return make_scoped_ptr(webrtc::BasicDesktopFrame::CopyOf(*frame_.get()));
 }
 
 void TestVideoRenderer::Core::ProcessVideoPacket(
@@ -147,7 +176,7 @@ void TestVideoRenderer::Core::ProcessVideoPacket(
     if (!screen_size_.equals(source_size)) {
       screen_size_ = source_size;
       decoder_->Initialize(screen_size_);
-      buffer_.reset(new webrtc::BasicDesktopFrame(screen_size_));
+      frame_.reset(new webrtc::BasicDesktopFrame(screen_size_));
     }
   }
 
@@ -166,28 +195,100 @@ void TestVideoRenderer::Core::ProcessVideoPacket(
     // previous video frame.
     decoder_->RenderFrame(screen_size_,
                           webrtc::DesktopRect::MakeWH(screen_size_.width(),
-                          screen_size_.height()), buffer_->data(),
-                          buffer_->stride(), &updated_region_);
+                                                      screen_size_.height()),
+                          frame_->data(), frame_->stride(), &updated_region_);
   }
 
   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, and whether
+  // the |expected_rect_| falls within the current frame.
+  if (image_pattern_matched_callback_.is_null() ||
+      expected_rect_.right() > frame_->size().width() ||
+      expected_rect_.bottom() > frame_->size().height()) {
+    return;
+  }
+
+  // Compare the expected image pattern with the corresponding rectangle region
+  // on the current frame.
+  RGBTriplets accumulating_avg_triplets =
+      CalculateAverageColorTriplets(expected_rect_);
+  LOG(INFO) << accumulating_avg_triplets.red << " "

[joedow, 2015/07/10 23:39:16]

LOG(INFO) is pretty chatty, can this be a VLOG(2)?

[liaoyuke, 2015/07/13 16:05:38]

Done.

+            << accumulating_avg_triplets.green << " "
+            << accumulating_avg_triplets.blue;
+  if (ExpectedColorIsMatched(accumulating_avg_triplets, kMaxColorError)) {

[joedow, 2015/07/10 23:39:15]

Does kMaxColorError need to be passed?  If you always use a constant, can you
just use that constant in the function and remove the second param?

[liaoyuke, 2015/07/13 16:05:38]

Done.

+    main_task_runner_->PostTask(
+        FROM_HERE, base::ResetAndReturn(&image_pattern_matched_callback_));
+  }
 }
 
 void TestVideoRenderer::Core::SetImagePatternAndMatchedCallback(
     const webrtc::DesktopRect& expected_rect,
-    const RgbaColor& expected_color,
+    const RGBA32& expected_avg_color,

[joedow, 2015/07/10 23:39:16]

Remove const?

[liaoyuke, 2015/07/13 16:05:38]

Done.

     const base::Closure& image_pattern_matched_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   expected_rect_ = expected_rect;
-  expected_color_ = expected_color;
+  expected_avg_color_ = expected_avg_color;
   image_pattern_matched_callback_ = image_pattern_matched_callback;
 }
 
+RGBA32 TestVideoRenderer::Core::ConvertTripletsToRGBA32(
+    const RGBTriplets& rgb_triplets) {
+  return 0xFF000000 | (rgb_triplets.red << 16) | (rgb_triplets.green << 8) |
+         rgb_triplets.blue;
+}
+
+RGBTriplets TestVideoRenderer::Core::ConvertRGBA32ToTriplets(RGBA32 color) {
+  RGBTriplets rgb_triplets((color >> 16) & 0xFF, (color >> 8) & 0xFF,
+                           color & 0xFF);
+  return rgb_triplets;
+}
+
+RGBTriplets TestVideoRenderer::Core::CalculateAverageColorTriplets(
+    const webrtc::DesktopRect& rect) const {
+  int red_sum = 0;
+  int green_sum = 0;
+  int blue_sum = 0;
+
+  // Loop through pixels that fall within |accumulating_rect_| to obtain the
+  // average color triplets.
+  for (int y = rect.top(); y < rect.bottom(); ++y) {
+    uint8_t* frame_pos =
+        frame_->data() + (y * frame_->stride() +
+                          rect.left() * webrtc::DesktopFrame::kBytesPerPixel);
+
+    // Pixels of decoded video frame are presented in ARGB format.
+    for (int x = 0; x < rect.width(); ++x) {
+      red_sum += frame_pos[2];
+      green_sum += frame_pos[1];
+      blue_sum += frame_pos[0];
+      frame_pos += 4;
+    }
+  }
+
+  int area = rect.width() * rect.height();
+  RGBTriplets rgb_triplets(red_sum / area, green_sum / area, blue_sum / area);
+  return rgb_triplets;
+}
+
+bool TestVideoRenderer::Core::ExpectedColorIsMatched(
+    const RGBTriplets& candidate_avg_triplets,
+    double error_limit) const {
+  RGBTriplets expected_avg_triplets =
+      ConvertRGBA32ToTriplets(expected_avg_color_);
+  double error_sum_squares = 0;
+  double red_error = expected_avg_triplets.red - candidate_avg_triplets.red;
+  double green_error =
+      expected_avg_triplets.green - candidate_avg_triplets.green;
+  double blue_error = expected_avg_triplets.blue - candidate_avg_triplets.blue;
+  error_sum_squares = red_error * red_error + green_error * green_error +
+                      blue_error * blue_error;
+  error_sum_squares /= (255.0 * 255.0);
+
+  return sqrt(error_sum_squares / 3) < error_limit;
+}
+
 TestVideoRenderer::TestVideoRenderer()
     : video_decode_thread_(
         new base::Thread("TestVideoRendererVideoDecodingThread")),
@@ -267,23 +368,25 @@ void TestVideoRenderer::SetCodecForDecoding(
                             codec));
 }
 
-scoped_ptr<webrtc::DesktopFrame> TestVideoRenderer::GetBufferForTest() const {
+scoped_ptr<webrtc::DesktopFrame> TestVideoRenderer::GetFrameForTest() const {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  return core_->GetBufferForTest();
+  return core_->GetFrameForTest();
 }
 
 void TestVideoRenderer::SetImagePatternAndMatchedCallback(
     const webrtc::DesktopRect& expected_rect,
-    const RgbaColor& expected_color,
+    const RGBA32& expected_avg_color,

[joedow, 2015/07/10 23:39:16]

remove const?

[liaoyuke, 2015/07/13 16:05:38]

Done.

     const base::Closure& image_pattern_matched_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!expected_rect.is_empty()) << "Expected rect cannot be empty";
 
   DVLOG(2) << "TestVideoRenderer::SetImagePatternAndMatchedCallback() Called";
   video_decode_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&Core::SetImagePatternAndMatchedCallback,
-                            base::Unretained(core_.get()), expected_rect,
-                            expected_color, image_pattern_matched_callback));
+      FROM_HERE,
+      base::Bind(&Core::SetImagePatternAndMatchedCallback,
+                 base::Unretained(core_.get()), expected_rect,
+                 expected_avg_color, image_pattern_matched_callback));
 }
 
 }  // namespace test