vda_unittest: Add a new test case to verify the visible size.

media/gpu/video_decode_accelerator_unittest.cc

Index: media/gpu/video_decode_accelerator_unittest.cc
diff --git a/media/gpu/video_decode_accelerator_unittest.cc b/media/gpu/video_decode_accelerator_unittest.cc
index d76bc615be2e8dfe58d99a60a69fd29fc1006311..83de4177c9298481873fd62815584046d7277b3a 100644
--- a/media/gpu/video_decode_accelerator_unittest.cc
+++ b/media/gpu/video_decode_accelerator_unittest.cc
@@ -182,6 +182,13 @@ struct TestVideoFile {
   std::string data_str;
 };
 
+struct VisibleRectEntry {

[Pawel Osciak, 2016/11/17 07:50:01]

Please add a short sentence describing what this is, and also what count means.

+  gfx::Rect rect;
+  int count;
+  VisibleRectEntry(const gfx::Rect rect, int count)

[Pawel Osciak, 2016/11/17 07:50:01]

const&

+      : rect(rect), count(count) {}
+};
+
 const gfx::Size kThumbnailsPageSize(1600, 1200);
 const gfx::Size kThumbnailSize(160, 120);
 const int kMD5StringLength = 32;
@@ -451,6 +458,9 @@ class GLRenderingVDAClient
   double frames_per_second();
   // Return the median of the decode time of all decoded frames.
   base::TimeDelta decode_time_median();
+  const std::vector<VisibleRectEntry>& visible_rects() {
+    return visible_rects_;
+  }
   bool decoder_deleted() { return !decoder_.get(); }
 
  private:
@@ -512,6 +522,8 @@ class GLRenderingVDAClient
   std::map<int, base::TimeTicks> decode_start_time_;
   // The decode time of all decoded frames.
   std::vector<base::TimeDelta> decode_time_;
+  // The visible rectangles of the picture returned from PictureReady().
+  std::vector<VisibleRectEntry> visible_rects_;
   // The number of VDA::Decode calls per second. This is to simulate webrtc.
   int decode_calls_per_second_;
   bool render_as_thumbnails_;
@@ -730,6 +742,13 @@ void GLRenderingVDAClient::PictureReady(const Picture& picture) {
 
   frame_delivery_times_.push_back(now);
 
+  if (visible_rects_.empty() ||
+      visible_rects_.back().rect != picture.visible_rect()) {
+    visible_rects_.push_back(VisibleRectEntry(picture.visible_rect(), 1));

[Pawel Osciak, 2016/11/17 07:50:01]

Perhaps just have count=1 as default when constructing? Up to you.

+  } else {
+    ++visible_rects_.back().count;
+  }
+
   // Save the decode time of this picture.
   std::map<int, base::TimeTicks>::iterator it =
       decode_start_time_.find(picture.bitstream_buffer_id());
@@ -1708,6 +1727,52 @@ TEST_F(VideoDecodeAcceleratorTest, TestDecodeTimeMedian) {
   WaitUntilIdle();
 };
 
+// This test Logs the visible rectangles of the picture returned from

[Pawel Osciak, 2016/11/17 07:50:01]

s/Logs/verifies/

+// PictureReady().
+TEST_F(VideoDecodeAcceleratorTest, TestVisibleRect) {
+  RenderingHelperParams helper_params;
+
+  // Disable rendering by setting the rendering_fps = 0.
+  helper_params.rendering_fps = 0;
+  helper_params.warm_up_iterations = 0;
+  helper_params.render_as_thumbnails = false;
+
+  ClientStateNotification<ClientState>* note =
+      new ClientStateNotification<ClientState>();
+  GLRenderingVDAClient* client = new GLRenderingVDAClient(
+      0, &rendering_helper_, note, test_video_files_[0]->data_str, 1, 1,
+      test_video_files_[0]->reset_after_frame_num, CS_RESET,
+      test_video_files_[0]->width, test_video_files_[0]->height,
+      test_video_files_[0]->profile, g_fake_decoder, true,
+      std::numeric_limits<int>::max(), 0, false /* render_as_thumbnail */);
+  helper_params.window_sizes.push_back(
+      gfx::Size(test_video_files_[0]->width, test_video_files_[0]->height));
+  InitializeRenderingHelper(helper_params);
+  CreateAndStartDecoder(client, note);

[Pawel Osciak, 2016/11/17 07:50:01]

I wonder if we need an additional test case just for this, we could maybe just
verify this in SimpleDecode, otherwise we are making the automated tests run
longer...

[wuchengli, 2016/11/17 08:23:41]

Adding a test case has a benefits that it's clear what is broken.
TestDecodeTimeMedian is a performance test and runs very long. We can remove
TestDecodeTimeMedian from the default test suite to save some time.

[Pawel Osciak, 2016/11/17 08:40:31]

Yes, but I believe making the test run longer outweights this advantage, and
proper logging via EXPECT() should be enough to see what went wrong.
That's an orthogonal issue. The test stream is 10 seconds long. By adding this
test, we are making each run of this test 10 seconds longer in the test lab for
each run, but we could achieve this by adding this verification to one of the
existing test cases, just like we are already verifying multiple things in them,
like the number of frames decoded.

+  WaitUntilDecodeFinish(note);
+
+  for (const auto& entry : client->visible_rects()) {
+    const auto& r = entry.rect;
+    std::string output_string =
+        base::StringPrintf("(%d, %d, %d, %d) %d", r.x(), r.y(), r.width(),

[kcwu, 2016/11/17 07:54:51]

How about 
LOG(INFO) << entry.rect << " " << entry.count;

+                           r.height(), entry.count);
+    LOG(INFO) << output_string;

[Pawel Osciak, 2016/11/17 07:50:01]

Perhaps just DVLOG?

+  }
+
+  // Resolution change video is not supported yet. Simply verify there is
+  // only one resolution and it matches to the expected visible rect.
+  ASSERT_EQ(1U, client->visible_rects().size());
+  gfx::Rect expected_rect(test_video_files_[0]->width,
+                          test_video_files_[0]->height);
+  const auto& entry = client->visible_rects().front();
+  ASSERT_TRUE(entry.rect.IsEmpty() || (entry.rect == expected_rect));

[Pawel Osciak, 2016/11/17 07:50:01]

I think we could just EXPECT?

[kcwu, 2016/11/17 07:54:51]

EXPECT_TRUE

+  ASSERT_EQ(test_video_files_[0]->num_frames, entry.count);

[kcwu, 2016/11/17 07:54:51]

EXPECT_EQ

+
+  g_env->GetRenderingTaskRunner()->DeleteSoon(FROM_HERE, client);
+  g_env->GetRenderingTaskRunner()->DeleteSoon(FROM_HERE, note);
+  WaitUntilIdle();
+};
+
 // TODO(fischman, vrk): add more tests!  In particular:
 // - Test life-cycle: Seek/Stop/Pause/Play for a single decoder.
 // - Test alternate configurations