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 <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"
 #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"
 
+// 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 {
 
 // Implements video decoding functionality.
 class TestVideoRenderer::Core {
  public:
   Core();
   ~Core();
 
   // Initializes the internal structures of the class.
   void Initialize();
 
   // Used to decode video packets.
   void ProcessVideoPacket(scoped_ptr<VideoPacket> packet,
                           const base::Closure& done);
 
   // 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_;
 
   // 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.
   webrtc::DesktopSize screen_size_;
 
   // Used to post tasks back to main thread.
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 
 TestVideoRenderer::Core::Core()
     : main_task_runner_(base::ThreadTaskRunnerHandle::Get()) {
   thread_checker_.DetachFromThread();
@@ skipping 30 matching lines @@
       VLOG(1) << "Test Video Renderer will use VERBATIM decoder";
       decoder_.reset(new VideoDecoderVerbatim());
       break;
     }
     default: {
       NOTREACHED() << "Unsupported codec: " << codec;
     }
   }
 }
 
-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(
     scoped_ptr<VideoPacket> packet, const base::Closure& done) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(decoder_);
   DCHECK(packet);
 
   VLOG(2) << "TestVideoRenderer::Core::ProcessVideoPacket() Called";
 
   // Screen size is attached on the first packet as well as when the
   // host screen is resized.
   if (packet->format().has_screen_width() &&
       packet->format().has_screen_height()) {
     webrtc::DesktopSize source_size(packet->format().screen_width(),
                                     packet->format().screen_height());
     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_));
     }
   }
 
   // To make life easier, assume that the desktop shape is a single rectangle.
   packet->clear_use_desktop_shape();
   if (!decoder_->DecodePacket(*packet.get())) {
     LOG(ERROR) << "Decoder::DecodePacket() failed.";
     return;
   }
 
   {
     base::AutoLock auto_lock(lock_);
 
     // Render the decoded packet and write results to the buffer.
     // 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_);
+                                                      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")),
       weak_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   core_.reset(new Core());
   if (!video_decode_thread_->Start()) {
     LOG(ERROR) << "Cannot start TestVideoRenderer";
   } else {
@@ skipping 59 matching lines @@
     const protocol::ChannelConfig::Codec codec) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   VLOG(2) << "TestVideoRenderer::SetDecoder() Called";
   video_decode_task_runner_->PostTask(
       FROM_HERE, base::Bind(&Core::SetCodecForDecoding,
                             base::Unretained(core_.get()),
                             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
 }  // namespace remoting