Added image pattern comparison logic for test interface and fixture.

remoting/test/test_video_renderer_unittest.cc

Index: remoting/test/test_video_renderer_unittest.cc
diff --git a/remoting/test/test_video_renderer_unittest.cc b/remoting/test/test_video_renderer_unittest.cc
index 8c1d92f7fd04d30fefa57345ad3d167a0172310b..1965f4553d9c00a1fb731836d28efea3b207e4fd 100644
--- a/remoting/test/test_video_renderer_unittest.cc
+++ b/remoting/test/test_video_renderer_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/timer/timer.h"
 #include "media/base/video_frame.h"
 #include "remoting/codec/video_encoder.h"
@@ -20,11 +21,25 @@
 #include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
 
 namespace {
-const int kBytesPerPixel = 4;
+
+// Used to verify that image pattern is not matched.
+void VideoPacketDoneHandler(const base::Closure& done_closure,

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

'VideoPacketDoneHandler' isn't very descriptive, would something like
'EmptyVideoPacketDoneHandler' or 'NeverMatchVideoPacketDoneHandler' be clearer?

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

I have changed the mechanism: in both Match and NotMatch test case, we first
send the test packet and then a copy of the test packet with a done handler is
sent to the TVR to see if the run_loop_->QuitClosure() has been fired, which
indicates if the expected pattern has been matched.

So, I changed the name to 'PatternMatchedVerifyPacketDoneHandler'.

+                            bool* image_pattern_is_matched) {
+  *image_pattern_is_matched = false;
+  done_closure.Run();
+}
+
+// Default screen size, measured in pixels.
 const int kDefaultScreenWidth = 1024;
 const int kDefaultScreenHeight = 768;

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

nit: You can remove the comment if you change the name of the variables to
kDefaultScreenWidthPx and kDefaultScreenHeightPx

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

Done.

+
+// Default max error for encoding and decoding, measured in percent.
 const double kDefaultErrorLimit = 0.02;
-}
+
+// Default expected rect for image pattern, measured in pixels.
+const webrtc::DesktopRect kDefaultExpectedRect =
+    webrtc::DesktopRect::MakeLTRB(100, 100, 200, 200);
+}  // namespace
 
 namespace remoting {
 namespace test {
@@ -37,14 +52,40 @@ class TestVideoRendererTest : public testing::Test {
   ~TestVideoRendererTest() override;
 
   // Generate a frame containing a gradient and test decoding of
-  // TestVideoRenderer. The original frame is compared to the one obtained from
-  // decoding the video packet, and the error at each pixel is the root mean
-  // square of the errors in the R, G and B components, each normalized to
-  // [0, 1]. This routine checks that the mean error over all pixels do not
-  // exceed a given limit.
+  // TestVideoRenderer.
+  // The original frame is compared to the one obtained from decoding the video
+  // packet, and the error at each pixel is the root mean square of the errors
+  // in the R, G and B components, each normalized to [0, 1]. This routine
+  // checks that the mean error over all pixels do not exceed a given limit.
   void TestVideoPacketProcessing(int screen_width, int screen_height,
                                  double error_limit);
 
+  // True positive case: checks correct image pattern will be matched.
+  // Generate a frame containing a gradient and an expected color is first
+  // obtained as the average value of pixels that fall within |expected_rect|,
+  // then set expected image pattern and encode the frame to send to
+  // TestVieoRenderer for processing. This routine expects that the expected
+  // image can be matched and corresponding callback can be fired.

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

These comments are better suited for the definition of the method (i.e. where
you want context of how since you are reading through the code).  The
documenting comment should be simpler and be something like "Handles setting a
pattern and sending a frame which are expected to be matched by the TVR."

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

Done.

+  void TestImagePatternMatchAndCallback(
+      int screen_width,
+      int screen_height,
+      const webrtc::DesktopRect& expected_rect);
+
+  // True negative case: checks incorrect image pattern will not be matched.
+  // Generate a frame containing a gradient and an expected color is first
+  // obtained as the average value of pixels that fall within |expected_rect|,
+  // then each channel of the expected color is shifted by 128 to make sure the
+  // difference between expected color and true value is large enough that
+  // expected image pattern should not match. Next, set expected image pattern
+  // and encode the frame to send to TestVideoRenderer for processing, lastly,
+  // an empty video packet with VideoPacketDoneHandler is sent to
+  // TestVideoRenderer to check whether callback has been fired or not. This
+  // routine checks that the expected image will not matched and callback will
+  // not be fired.
+  void TestImagePatternNotMatch(int screen_width,
+                                int screen_height,
+                                const webrtc::DesktopRect& expected_rect);
+
   // Generate a basic desktop frame containing a gradient.
   scoped_ptr<webrtc::DesktopFrame> CreateDesktopFrameWithGradient(
       int screen_width, int screen_height) const;
@@ -66,6 +107,12 @@ class TestVideoRendererTest : public testing::Test {
   // testing::Test interface.
   void SetUp() override;
 
+  // Returns the average color value of pixels fall within |rect|.
+  // NOTE: Callers should not release the objects.
+  RGBA32 CalculateAverageColorTripletsForFrame(

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

CalculateAverageRgbColorForFrame

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

Done.

+      const webrtc::DesktopFrame* frame,
+      const webrtc::DesktopRect& rect) const;
+
   // return the mean error of two frames.
   double CalculateError(const webrtc::DesktopFrame* original_frame,
                         const webrtc::DesktopFrame* decoded_frame) const;
@@ -109,15 +156,142 @@ void TestVideoRendererTest::TestVideoPacketProcessing(int screen_width,
   // Wait for the video packet to be processed and rendered to buffer.
   test_video_renderer_->ProcessVideoPacket(packet.Pass(),
                                            run_loop_->QuitClosure());
+
   run_loop_->Run();
 
   scoped_ptr<webrtc::DesktopFrame> buffer_copy =
-      test_video_renderer_->GetBufferForTest();
+      test_video_renderer_->GetFrameForTest();
   EXPECT_NE(buffer_copy, nullptr);
   double error = CalculateError(original_frame.get(), buffer_copy.get());
   EXPECT_LT(error, error_limit);
 }
 
+void TestVideoRendererTest::TestImagePatternMatchAndCallback(
+    int screen_width,
+    int screen_height,
+    const webrtc::DesktopRect& expected_rect) {
+  DCHECK(encoder_);
+  DCHECK(test_video_renderer_);
+
+  scoped_ptr<webrtc::DesktopFrame> frame =
+      CreateDesktopFrameWithGradient(screen_width, screen_height);
+  EXPECT_TRUE(frame);

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

I don't think you need EXPECT_TRUE since it is a helper method the test owns. 
If this failed (frame were null) it would be a test bug that you'd find very
quickly.

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

Done.

+  RGBA32 expected_color =
+      CalculateAverageColorTripletsForFrame(frame.get(), expected_rect);
+  DCHECK(!run_loop_ || !run_loop_->running());
+  run_loop_.reset(new base::RunLoop());
+
+  // Set expected image pattern.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestVideoRenderer::SetImagePatternAndMatchedCallback,
+                 base::Unretained(test_video_renderer_.get()), expected_rect,
+                 expected_color, run_loop_->QuitClosure()));

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

Why are you posting a task here instead of just calling the function on the
test_video_renderer_ pointer that have?  Internally the TVR is just going to
post the task to its decode thread so I'm not sure what posting here as well is
doing for you.

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

Yes, you are right, they are basically doing the same thing.

+
+  // Post test video packet, and this packet is expected to match the expected
+  // image pattern.
+  scoped_ptr<VideoPacket> packet = encoder_->Encode(*frame.get());
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestVideoRenderer::ProcessVideoPacket,
+                 base::Unretained(test_video_renderer_.get()),
+                 base::Passed(&packet), base::Bind(&base::DoNothing)));
+
+  // Wait for image pattern matched reply to be fired.

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

If there is a product bug, then the MessageLoop could run forever.  I think you
also want a timer here with a long max timeout (since you saw enourmously long
decode times on certain machines) so that you can ensure this test doesn't block
a run if a bug is introduced.

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

the new mechanism send a copy packet to check if expected pattern is matched in
both Match and NotMatch case.

+  run_loop_->Run();
+  run_loop_.reset();
+}
+
+void TestVideoRendererTest::TestImagePatternNotMatch(
+    int screen_width,
+    int screen_height,
+    const webrtc::DesktopRect& expected_rect) {
+  DCHECK(encoder_);
+  DCHECK(test_video_renderer_);
+
+  scoped_ptr<webrtc::DesktopFrame> frame =
+      CreateDesktopFrameWithGradient(screen_width, screen_height);
+  EXPECT_TRUE(frame);
+  RGBA32 expected_color =
+      CalculateAverageColorTripletsForFrame(frame.get(), expected_rect);
+
+  // Shift each channel by 128.
+  // e.g. (10, 127, 200) -> (138, 255, 73).
+  // In this way, the error between expected color and true value is always
+  // around 0.5.
+  int red_shift = (((expected_color >> 16) & 0xFF) + 128) % 255;
+  int green_shift = (((expected_color >> 8) & 0xFF) + 128) % 255;
+  int blue_shift = ((expected_color & 0xFF) + 128) % 255;
+
+  int expected_color_shift =
+      0xFF000000 | (red_shift << 16) | (green_shift << 8) | blue_shift;
+
+  DCHECK(!run_loop_ || !run_loop_->running());
+  run_loop_.reset(new base::RunLoop());
+
+  // Set expected image pattern.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestVideoRenderer::SetImagePatternAndMatchedCallback,
+                 base::Unretained(test_video_renderer_.get()), expected_rect,
+                 expected_color_shift, run_loop_->QuitClosure()));
+
+  // Post test video packet, and it is not expected to match the expected
+  // image pattern.
+  scoped_ptr<VideoPacket> packet = encoder_->Encode(*frame.get());
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestVideoRenderer::ProcessVideoPacket,
+                 base::Unretained(test_video_renderer_.get()),
+                 base::Passed(&packet), base::Bind(&base::DoNothing)));
+
+  // An empty packet with a VideoPacketDoneHandler is sent to check whether
+  // the callback has been fired or not.

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

Posting an empty packet will just cause your done callback to be called on the
current thread right?  It won't get posted to the decode thread's messageloop

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

You are right, there is a bug. The new mechanism is to send a copy of the test
packet instead of an empty one.

+  bool image_pattern_is_matched = true;
+  scoped_ptr<VideoPacket> empty_packet(new VideoPacket());
+  base::Closure video_packet_done_callback =
+      base::Bind(&VideoPacketDoneHandler, run_loop_->QuitClosure(),
+                 &image_pattern_is_matched);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestVideoRenderer::ProcessVideoPacket,
+                 base::Unretained(test_video_renderer_.get()),
+                 base::Passed(&empty_packet), video_packet_done_callback));
+
+  run_loop_->Run();
+  run_loop_.reset();
+
+  EXPECT_FALSE(image_pattern_is_matched);
+}
+
+RGBA32 TestVideoRendererTest::CalculateAverageColorTripletsForFrame(
+    const webrtc::DesktopFrame* frame,
+    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();
+  return 0xFF000000 | ((red_sum / area) << 16) | ((green_sum / area) << 8) |
+         (blue_sum / area);
+}
+
 double TestVideoRendererTest::CalculateError(
     const webrtc::DesktopFrame* original_frame,
     const webrtc::DesktopFrame* decoded_frame) const {
@@ -156,7 +330,7 @@ double TestVideoRendererTest::CalculateError(
     for (int width = 0; width < screen_width; ++width) {
       // Errors are calculated in the R, G, B components.
       for (int j = 0; j < 3; ++j) {
-        int offset = kBytesPerPixel * width + j;
+        int offset = webrtc::DesktopFrame::kBytesPerPixel * width + j;
         double original_value = static_cast<double>(*(original_ptr + offset));
         double decoded_value = static_cast<double>(*(decoded_ptr + offset));
         double error = original_value - decoded_value;
@@ -268,5 +442,86 @@ TEST_F(TestVideoRendererTest, VerifyVideoPacketSizeChange) {
                             kDefaultErrorLimit);
 }
 
+// Verify setting expected image pattern doesn't break video packet processing.
+TEST_F(TestVideoRendererTest, VerifySetExpectedImagePattern) {
+  encoder_ = VideoEncoderVpx::CreateForVP8();
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VP8);
+
+  DCHECK(encoder_);
+  DCHECK(test_video_renderer_);
+
+  scoped_ptr<webrtc::DesktopFrame> frame =
+      CreateDesktopFrameWithGradient(kDefaultScreenWidth, kDefaultScreenHeight);
+  EXPECT_TRUE(frame);
+
+  // Since we don't care whether expected image pattern is matched or not in
+  // this case, an expected color is chosen arbitrarily.
+  RGBA32 expected_color = 0xFF000000;

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

nit: I'd change the var name to reflect the color you've chosen since you know
exactly what it is.

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

Done.

+
+  // Set expected image pattern.
+  test_video_renderer_->SetImagePatternAndMatchedCallback(
+      kDefaultExpectedRect, expected_color, base::Bind(&base::DoNothing));
+
+  // Post test video packet.
+  scoped_ptr<VideoPacket> packet = encoder_->Encode(*frame.get());
+  test_video_renderer_->ProcessVideoPacket(packet.Pass(),
+                                           base::Bind(&base::DoNothing));
+}
+
+// Verify correct image pattern can be matched for VP8.
+TEST_F(TestVideoRendererTest, VerifyImagePatternMatchForVP8) {
+  encoder_ = VideoEncoderVpx::CreateForVP8();
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VP8);
+  TestImagePatternMatchAndCallback(kDefaultScreenWidth, kDefaultScreenHeight,
+                                   kDefaultExpectedRect);
+}
+
+// Verify expected image pattern can be matched for VP9.
+TEST_F(TestVideoRendererTest, VerifyImagePatternMatchForVP9) {
+  encoder_ = VideoEncoderVpx::CreateForVP9();
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VP9);
+  TestImagePatternMatchAndCallback(kDefaultScreenWidth, kDefaultScreenHeight,
+                                   kDefaultExpectedRect);
+}
+
+// Verify expected image pattern can be matched for VERBATIM.
+TEST_F(TestVideoRendererTest, VerifyImagePatternMatchForVERBATIM) {
+  encoder_.reset(new VideoEncoderVerbatim());
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VERBATIM);
+  TestImagePatternMatchAndCallback(kDefaultScreenWidth, kDefaultScreenHeight,
+                                   kDefaultExpectedRect);
+}
+
+// Verify incorrect image pattern shouldn't be matched for VP8.
+TEST_F(TestVideoRendererTest, VerifyImagePatternNotMatchForVP8) {
+  encoder_ = VideoEncoderVpx::CreateForVP8();
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VP8);
+  TestImagePatternNotMatch(kDefaultScreenWidth, kDefaultScreenHeight,
+                           kDefaultExpectedRect);
+}
+
+// Verify incorrect image pattern shouldn't be matched for VP9.
+TEST_F(TestVideoRendererTest, VerifyImagePatternNotMatchForVP9) {
+  encoder_ = VideoEncoderVpx::CreateForVP9();
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VP9);
+  TestImagePatternNotMatch(kDefaultScreenWidth, kDefaultScreenWidth,
+                           kDefaultExpectedRect);
+}
+
+// Verify incorrect image pattern shouldn't be matched for VERBATIM.
+TEST_F(TestVideoRendererTest, VerifyImagePatternNotMatchForVERBATIM) {
+  encoder_.reset(new VideoEncoderVerbatim());
+  test_video_renderer_->SetCodecForDecoding(
+      protocol::ChannelConfig::Codec::CODEC_VERBATIM);
+  TestImagePatternNotMatch(kDefaultScreenWidth, kDefaultScreenHeight,
+                           kDefaultExpectedRect);
+}
+
 }  // namespace test
 }  // namespace remoting