Added image pattern comparison logic for test interface and fixture.

remoting/test/test_video_renderer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/thread.h"
 #include "remoting/codec/video_decoder.h"
 #include "remoting/codec/video_decoder_verbatim.h"
 #include "remoting/codec/video_decoder_vpx.h"
 #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 {
 
 // Implements video decoding functionality.
 class TestVideoRenderer::Core {
  public:
   Core();
   ~Core();
 
   // Initializes the internal structures of the class.
@@ skipping 10 matching lines @@
   scoped_ptr<webrtc::DesktopFrame> GetBufferForTest() 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 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_;
 
   // Used to decode video packets.
   scoped_ptr<VideoDecoder> decoder_;
 
   // Updated region of the current desktop frame compared to previous one.
   webrtc::DesktopRegion updated_region_;
 
   // Screen size of the remote host.
@@ skipping 12 matching lines @@
   webrtc::DesktopRect expected_rect_;
   RgbaColor expected_color_;
 
   // Maintains accumulating image pattern.
   webrtc::DesktopRect accumulating_rect_;
   RgbaColor accumulating_color_;
 
   // 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);
 };
 
 TestVideoRenderer::Core::Core()
     : main_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
   thread_checker_.DetachFromThread();
 }
 
 TestVideoRenderer::Core::~Core() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 void TestVideoRenderer::Core::Initialize() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0);
 }
 
 void TestVideoRenderer::Core::SetCodecForDecoding(
     const protocol::ChannelConfig::Codec codec) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (decoder_) {
     LOG(WARNING) << "Decoder is set more than once";
   }
 
@@ skipping 61 matching lines @@
     // Note that the |updated_region_| maintains the changed regions compared to
     // previous video frame.
     decoder_->RenderFrame(screen_size_,
                           webrtc::DesktopRect::MakeWH(screen_size_.width(),
                           screen_size_.height()), buffer_->data(),
                           buffer_->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.
+  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(
     const webrtc::DesktopRect& expected_rect,
     const RgbaColor& expected_color,
     const base::Closure& image_pattern_matched_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   expected_rect_ = expected_rect;
   expected_color_ = expected_color;
   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")),
       weak_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   core_.reset(new Core());
   if (!video_decode_thread_->Start()) {
     LOG(ERROR) << "Cannot start TestVideoRenderer";
   } else {
@@ skipping 80 matching lines @@
 
   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));
 }
 
 }  // namespace test
 }  // namespace remoting