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..e0690303953abdb93b2538f07df5dd85e61cba48 100644
--- a/remoting/test/test_video_renderer.cc
+++ b/remoting/test/test_video_renderer.cc
@@ -4,7 +4,11 @@
 
 #include "remoting/test/test_video_renderer.h"
 
+#include <algorithm>
+#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 +19,13 @@
 #include "remoting/proto/video.pb.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
+namespace {
+const int kBytesPerPixel = 4;

[Sergey Ulanov, 2015/07/09 19:43:14]

use webrtc::DesktopFrame::kBytesPerPixel.

[liaoyuke, 2015/07/09 23:21:01]

Done.

+
+// Used to account for video frame resizing and lossy encoding.
+const double fuzzy_threshold_ = 0.05;

[Sergey Ulanov, 2015/07/09 19:43:14]

Not a proper name for a const.

What units is it measured in?

It looks like you use this in two places: to compare position of the rect and to
compare color. It might be better to have two separate values for these two
cases. You don't expect lossy encoding to shift anything around.

[liaoyuke, 2015/07/09 23:21:01]

I think so, have two separate values looks cleaner.

+}  // namespace
+
 namespace remoting {
 namespace test {
 
@@ -41,10 +52,32 @@ class TestVideoRenderer::Core {
   // when the pattern is matched.
   void SetImagePatternAndMatchedCallback(
       const webrtc::DesktopRect& expected_rect,
-      const RgbaColor& expected_color,
+      const RGBA32& expected_color,
       const base::Closure& image_pattern_matched_callback);
 
  private:
+  // Returns true if |rect2| falls within |rect1|, allowing for certain amount

[liaoyuke, 2015/07/09 23:21:01]

Update comments in next patch.

+  // of error specified by a fuzzy threshold.
+  bool ContainsRect(const webrtc::DesktopRect& template_rect,
+                    const webrtc::DesktopRect& candidate_rect) 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_;
 
@@ -68,11 +101,11 @@ class TestVideoRenderer::Core {
 
   // Used to store the expected image pattern.
   webrtc::DesktopRect expected_rect_;
-  RgbaColor expected_color_;
+  RGBColor expected_color_;
 
   // Maintains accumulating image pattern.
   webrtc::DesktopRect accumulating_rect_;
-  RgbaColor accumulating_color_;
+  RGBColor accumulating_color_;

[Sergey Ulanov, 2015/07/09 19:43:14]

I'm not sure why it's called "accumulating" given that it's average of
"accumulating_rect_"

[liaoyuke, 2015/07/09 23:21:01]

Done.

 
   // Used to store the callback when expected pattern is matched.
   base::Closure image_pattern_matched_callback_;
@@ -91,6 +124,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,22 +207,117 @@ 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()) {
+    return;
+  }
+
+  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)) {

[Sergey Ulanov, 2015/07/09 19:43:14]

I don't get this part. What if dirty_rect is partially within expected_rect? Do
we want to ignore it in this case? Or what if dirty_rect covers expected_rect_?
It looks to me that what you want to do here is to intersect the dirty_region
with a rect that contains expected_rect with margins added on each side 

[liaoyuke, 2015/07/09 23:21:01]

Yes, it will be ignored in both case, and we only care about those dirty rects
that fall within the expected_rect_. The reason is that generated expected_rects
will always be the merge result of some dirty rects, which means that if a dirty
rect is one that we are interested in, it is ensured to be contained by the
expected_rect.

[Sergey Ulanov, 2015/07/10 01:13:07]

I still don't see how this can work correctly. Let's say you have
expected_rect=(x=100, y=100, w=100, h=100) and you receive a rectangle that
covers the whole screen, e.g. (x=0, y=0, w=1024, h=768). This code will ignore
that updated, but it shouldn't.

+      MergeRectToAccumulatingRect(dirty_rect);
+    }
+  }
+
+  UpdateAccumulatingColor();

[Sergey Ulanov, 2015/07/09 19:43:14]

would it be better to return "accumulated_color" from  UpdateAccumulatingColor()
and pass it to ExpectedImagePatternIsMatched()? accumulated_color_ doesn't need
to be class memeber.

[liaoyuke, 2015/07/09 23:21:01]

Yes, I think that's a good idea!

+  if (ExpectedImagePatternIsMatched()) {

[Sergey Ulanov, 2015/07/09 19:43:14]

I don't get the logic here. UpdateAccumulatedColor() calculates average color
over accumulated_rect_ and then ExpectedImagePatternIsMatched() verifies that
that color matches expected value. But I don't see anything to verify that this
color fills the whole expected_rect_. I.e. the code seems to consider patter
matched even if we received update for some parts of the target rect.

[liaoyuke, 2015/07/09 23:21:01]

Actually, ExpectedImagePatternIsMatched() not only verifies color is matched,
but also expected_rect is matched i.e. the sum of the update on rect has covered
target rect.

[Sergey Ulanov, 2015/07/10 01:13:07]

Missed that sorry. But I still think it isn't correct. Lets say you have
expected_rect=(x=100, y=100, w=100, h=100) and you receive two updates one for
(x=100, y=100, w=50, h=50) and another one for (x=150, y=150, w=50, h=50).
You'll get accumulated_rect_ to contain the whole expected_rect, while you've
received data only for a half of it.

Here is an approach that I think would work better:
 1. Dump whole dirty_region() to accumulated_region_.
 2. Verify that accumulated_region_ contains the whole expected_rect_, maybe
excluding 1 or 2 pixel from the edges. This is easy implement by contracting
expected_rect_by the margin on each side and intersecting with
accumulated_region_. i.e. something like:
     ContractRect(expected_rect, margin);
     DesktopRegion intersection;
     intersection.Intersect(accumulated_region_, DesktopRegion(expected_rect));
     bool rect_covered = DesktopRegion(expected_rect).Equals(intersection);

 3. If the whole rect is covered by accumulated_region_ compare colors in each
pixel.

+    base::ResetAndReturn(&image_pattern_matched_callback_).Run();
+    accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0);

[Sergey Ulanov, 2015/07/09 19:43:14]

Default constructor creates an empty rect, i.e. this can be
webrtc::DesktopRect();

[liaoyuke, 2015/07/09 23:21:01]

Done.

+  }
 }
 
 void TestVideoRenderer::Core::SetImagePatternAndMatchedCallback(
     const webrtc::DesktopRect& expected_rect,
-    const RgbaColor& expected_color,
+    const RGBA32& expected_color,
     const base::Closure& image_pattern_matched_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   expected_rect_ = expected_rect;
-  expected_color_ = expected_color;
+  expected_color_.SetRGB(expected_color);
   image_pattern_matched_callback_ = image_pattern_matched_callback;
 }
 
+bool TestVideoRenderer::Core::ContainsRect(

[Sergey Ulanov, 2015/07/09 19:43:14]

This can be static function, doesn't need to be a method.

Also name doesn't make it clear that this function allows some error margins on
the edges.

[liaoyuke, 2015/07/09 23:21:01]

Done.

+    const webrtc::DesktopRect& template_rect,
+    const webrtc::DesktopRect& candidate_rect) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  double fuzzy_limit_width = fuzzy_threshold_ * template_rect.width();

[Sergey Ulanov, 2015/07/09 19:43:14]

Why does this need to be relative to the rect size? Can it be just 1 or 2 pixel
in each direction no matter what the size of the rect?

[liaoyuke, 2015/07/09 23:21:01]

I think you are right, since the lossy encoding will not shift anything around,
2 pixels in each direction is fair enough.

+  double fuzzy_limit_height = fuzzy_threshold_ * template_rect.height();
+
+  return candidate_rect.left() >= template_rect.left() - fuzzy_limit_width &&

[Sergey Ulanov, 2015/07/09 19:43:14]

I think you can use DesktopRect::ContainsRect(), just need to add the margins to
the candidate_rect

[liaoyuke, 2015/07/09 23:21:01]

Good point!

+         candidate_rect.top() >= template_rect.top() - fuzzy_limit_height &&
+         candidate_rect.right() <= template_rect.right() + fuzzy_limit_width &&
+         candidate_rect.bottom() <= template_rect.bottom() + fuzzy_limit_height;
+}
+
+void TestVideoRenderer::Core::MergeRectToAccumulatingRect(
+    const webrtc::DesktopRect& rect) {
+  // If |rect| is the first dirty rect that falls within |expected_rect_|,
+  // simply assign |rect| to |accumulating_rect_|.
+  if (!accumulating_rect_.width() && !accumulating_rect_.height()) {

[Sergey Ulanov, 2015/07/09 19:43:15]

accumulating_rect_.is_empty()

[liaoyuke, 2015/07/09 23:21:01]

Done.

+    accumulating_rect_ = rect;
+  } else {
+    // Merge |rect| to obtain a larger |accumulating_rect_|.
+    accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(

[Sergey Ulanov, 2015/07/09 19:43:14]

would it be better to store region instead of rect?

[liaoyuke, 2015/07/09 23:21:01]

Yes, I think so, using a single rectangle would lose some information, but for
now, I haven't come up with a clear solution on how to do comparison on region
and how to generate the expected region. rect is easy to deal with, but it might
be an important future work to move to region in the future. 

[Sergey Ulanov, 2015/07/10 01:13:07]

See my comment above for my suggestions

+        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;
+
+  // Loop through pixels that fall within |accumulating_rect_| to obtain the
+  // average color.
+  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);

[Sergey Ulanov, 2015/07/09 19:43:14]

You can make this code slightly faster by increment offset by 4 every time
instead of calculating it for each pixel. Or even better store pointer to
current position and increment it instead of indexing into buffer->data() every
time.

[Sergey Ulanov, 2015/07/09 19:43:14]

use buffer_->stride() instead of screen_size_.width(). Lines may not be alligned
in the buffer (and sometimes they are in reverse order, i.e. stride() can be
negative).

[Sergey Ulanov, 2015/07/09 19:43:15]

nit: extra parentheses don't add much here

[liaoyuke, 2015/07/09 23:21:01]

Done.

[liaoyuke, 2015/07/09 23:21:01]

Done.

[liaoyuke, 2015/07/09 23:21:01]

Done.

+
+      // Pixels of decoded video frame are presented in ARGB format.
+      red_sum += buffer_->data()[offset + 2];
+      green_sum += buffer_->data()[offset + 1];
+      blue_sum += buffer_->data()[offset + 0];
+    }
+  }
+
+  int area = screen_size_.width() * screen_size_.height();
+  accumulating_color_.SetRGB(red_sum / area, green_sum / area, blue_sum / area);
+}
+
+bool TestVideoRenderer::Core::ExpectedColorIsMatched() const {
+  double error_sum_squares = 0;
+  double red_error = static_cast<double>(expected_color_.GetRed() -

[Sergey Ulanov, 2015/07/09 19:43:14]

I don't think you need explicit cast here

[Sergey Ulanov, 2015/07/09 19:43:14]

Also I don't think that comparing averages is the right thing to do. E.g. if you
expect gray (128, 128, 128) and you get a frame that has half of the pixels
black (0, 0, 0) and half of them white (255, 255, 255) then you will get average
of (128, 128, 128) while the frame clearly doesn't match. You want to calculate
total error over the whole rect for each pixel.

[liaoyuke, 2015/07/09 23:21:01]

Yes, you are right, there are some potential problems with the average color.
But if we are going to calculate total error, then to create a test case, the
input of this test case would be a rectangle with detailed pixel information,
which would be painful for the user. I believe there are better solutions, but
for now, I believe this simple metric should work for most cases, and in the
future, more features can be added to improve the precision and robustness.

[liaoyuke, 2015/07/09 23:21:01]

Done.

[Sergey Ulanov, 2015/07/10 01:13:07]

I don't think you would need to change the interface for that. This code would
still get a rectangle and color, but instead of calculating the average and
comparing it to the expected color it should compare color of each pixel to the
expected values.

+                                         accumulating_color_.GetRed());
+  double green_error = static_cast<double>(expected_color_.GetGreen() -
+                                           accumulating_color_.GetGreen());
+  double blue_error = static_cast<double>(expected_color_.GetBlue() -
+                                          accumulating_color_.GetBlue());
+  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) < fuzzy_threshold_;
+}
+
+bool TestVideoRenderer::Core::ExpectedImagePatternIsMatched() const {
+  // accumulating_rect_ must be valid in order to be matched.

[Sergey Ulanov, 2015/07/09 19:43:14]

nit: |accumulating_rect_|

[liaoyuke, 2015/07/09 23:21:01]

Done.

+  return accumulating_rect_.width() && accumulating_rect_.height() &&
+         ContainsRect(accumulating_rect_, expected_rect_) &&
+         ContainsRect(expected_rect_, accumulating_rect_) &&
+         ExpectedColorIsMatched();
+}
+
 TestVideoRenderer::TestVideoRenderer()
     : video_decode_thread_(
         new base::Thread("TestVideoRendererVideoDecodingThread")),
@@ -275,7 +405,7 @@ scoped_ptr<webrtc::DesktopFrame> TestVideoRenderer::GetBufferForTest() const {
 
 void TestVideoRenderer::SetImagePatternAndMatchedCallback(
     const webrtc::DesktopRect& expected_rect,
-    const RgbaColor& expected_color,
+    const RGBA32& expected_color,
     const base::Closure& image_pattern_matched_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -286,5 +416,29 @@ void TestVideoRenderer::SetImagePatternAndMatchedCallback(
                             expected_color, image_pattern_matched_callback));
 }
 
+RGBColor::RGBColor(int red, int green, int blue) {
+  SetRGB(red, green, blue);
+}
+
+int RGBColor::GetRed() const {
+  return (color_ >> 16) & 0xFF;
+}
+
+int RGBColor::GetGreen() const {
+  return (color_ >> 8) & 0xFF;
+}
+
+int RGBColor::GetBlue() const {
+  return color_ & 0xFF;
+}
+
+void RGBColor::SetRGB(int red, int green, int blue) {
+  // if any of the channels is out of range [0, 255], simply take it as 0 or 255
+  // accordingly.
+  color_ = 0xFF000000 | (std::max(0, std::min(red, 255)) << 16) |
+           (std::max(0, std::min(green, 255)) << 8) |
+           (std::max(0, std::min(blue, 255)));
+}
+
 }  // namespace test
 }  // namespace remoting