Add switches: "target_fps" and "disable_rendering" to video_decode_accelerator_unittest.

content/common/gpu/media/video_decode_accelerator_unittest.cc

Index: content/common/gpu/media/video_decode_accelerator_unittest.cc
diff --git a/content/common/gpu/media/video_decode_accelerator_unittest.cc b/content/common/gpu/media/video_decode_accelerator_unittest.cc
index 439fb49453af4b6ecbdfce8138a76978159c08cf..e9ac090d2bd98c12b5d2e766b0ec753183cc51de 100644
--- a/content/common/gpu/media/video_decode_accelerator_unittest.cc
+++ b/content/common/gpu/media/video_decode_accelerator_unittest.cc
@@ -88,6 +88,12 @@ const base::FilePath::CharType* test_video_data =
 // the filename by the "--frame_delivery_log" switch.
 const base::FilePath::CharType* frame_delivery_log = NULL;
 
+// The value is set by the switch "--target_fps".
+float target_fps = 0;

[Ami GONE FROM CHROMIUM, 2013/07/15 18:54:14]

"target_fps" is too vague IMO.
IIUC you want it to simulate the actions of a renderer that takes timestamps
into account and buffers up frames that are delivered too early (and drops
frames that arrive too late).
If that's the case, then rendering_fps seems like a better name.

[Owen Lin, 2013/07/16 10:46:54]

Done.

+
+// Disable rendering, the value is set by the switch "--disable_rendering".
+bool disable_rendering = false;
+
 // Magic constants for differentiating the reasons for NotifyResetDone being
 // called.
 enum ResetPoint {
@@ -187,7 +193,7 @@ enum ClientState {
   CS_INITIALIZED = 2,
   CS_FLUSHING = 3,
   CS_FLUSHED = 4,
-  CS_DONE = 5,
+  CS_COMPLETE_RENDERING = 5,

[Pawel Osciak, 2013/07/15 08:07:06]

I'd call this RENDERED or RENDERING_DONE. IMHO this name is a bit ambiguous in
whether rendering is to be completed or has been completed

[Ami GONE FROM CHROMIUM, 2013/07/15 18:54:14]

I think none of these names is good enough to use (because none of them tell me
what this state is for).  AFAICT from the uses of this value, it is never
useful; there are no codepaths that set it without immediately setting
CS_RESETTING, so there's nothing for the test to check for meaningfully (I see
the added expectation, I just don't understand why it's useful; it's not
possible to get to CS_RESETTING /without/ passing through this state, and the
expectation of CS_RESETTING is already there).

[Owen Lin, 2013/07/16 10:46:54]

Are you suggesting removing this state ? My intention is to make it more clear
that we are waiting for rendering.

On 2013/07/15 18:54:14, Ami Fischman wrote:

[Owen Lin, 2013/07/16 10:46:54]

Done.

   CS_RESETTING = 6,
   CS_RESET = 7,
   CS_ERROR = 8,
@@ -263,6 +269,7 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
                        int frame_width,
                        int frame_height,
                        int profile,
+                       float target_fps,

[Pawel Osciak, 2013/07/15 08:07:06]

Add doc above?

[Owen Lin, 2013/07/16 10:46:54]

Done.

                        bool suppress_rendering,
                        int delay_reuse_after_frame_num);
   virtual ~GLRenderingVDAClient();
@@ -296,6 +303,7 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
   typedef std::map<int, media::PictureBuffer*> PictureBufferById;
 
   void SetState(ClientState new_state);
+  void RenderPicture(const media::Picture& picture);
 
   // Delete the associated OMX decoder helper.
   void DeleteDecoder();
@@ -340,6 +348,9 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
   bool suppress_rendering_;
   std::vector<base::TimeTicks> frame_delivery_times_;
   int delay_reuse_after_frame_num_;
+  base::TimeTicks next_frame_delivered_time_;
+  base::TimeDelta frame_interval_;

[Ami GONE FROM CHROMIUM, 2013/07/15 18:54:14]

s/interval/duration/

[Owen Lin, 2013/07/16 10:46:54]

Done.

+  int pending_rendering_;
 };
 
 GLRenderingVDAClient::GLRenderingVDAClient(
@@ -355,6 +366,7 @@ GLRenderingVDAClient::GLRenderingVDAClient(
     int frame_width,
     int frame_height,
     int profile,
+    float target_fps,
     bool suppress_rendering,
     int delay_reuse_after_frame_num)
     : rendering_helper_(rendering_helper),
@@ -376,6 +388,9 @@ GLRenderingVDAClient::GLRenderingVDAClient(
   CHECK_GT(num_fragments_per_decode, 0);
   CHECK_GT(num_in_flight_decodes, 0);
   CHECK_GT(num_play_throughs, 0);
+  CHECK_GE(target_fps, 0);
+  if (target_fps > 0)
+    frame_interval_ = base::TimeDelta::FromSeconds(1) / target_fps;
 }
 
 GLRenderingVDAClient::~GLRenderingVDAClient() {
@@ -459,10 +474,6 @@ void GLRenderingVDAClient::PictureReady(const media::Picture& picture) {
 
   if (decoder_deleted())
     return;
-  frame_delivery_times_.push_back(base::TimeTicks::Now());
-
-  CHECK_LE(picture.bitstream_buffer_id(), next_bitstream_buffer_id_);
-  ++num_decoded_frames_;
 
   // Mid-stream reset applies only to the last play-through per constructor
   // comment.
@@ -475,13 +486,41 @@ void GLRenderingVDAClient::PictureReady(const media::Picture& picture) {
     encoded_data_next_pos_to_decode_ = 0;
   }
 
+  base::TimeTicks now = base::TimeTicks::Now();
+
+  // Play immediately if it is the first frame or there is no target_fps
+  if (num_decoded_frames_ == 0 || frame_interval_ == base::TimeDelta())
+    next_frame_delivered_time_ = now;
+
+  CHECK_LE(picture.bitstream_buffer_id(), next_bitstream_buffer_id_);
+  ++num_decoded_frames_;
+
+  while (next_frame_delivered_time_ < now) {
+    // frame dropped

[Pawel Osciak, 2013/07/15 08:07:06]

Actually, there might be more than one frame dropped here.

I'm thinking that perhaps if we are past this frame's successor's delivery time,
we should not render it at all either.

[Owen Lin, 2013/07/16 10:46:54]

Done.

+    next_frame_delivered_time_ += frame_interval_;
+  }
+
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&GLRenderingVDAClient::RenderPicture,
+                 base::Unretained(this),
+                 picture),
+      next_frame_delivered_time_ - now);
+  next_frame_delivered_time_ += frame_interval_;
+  ++pending_rendering_;

[Pawel Osciak, 2013/07/15 08:07:06]

There is a chance that when you loop in l.498, next_frame_delivered_time_ might
become less than now, so the delayed task will not execute and your
pending_rendering count will be off.

[Owen Lin, 2013/07/16 10:46:54]

Hi, I don't really get it. When we exit the loop in l.498, it is guaranteed that
next_frame_delivered_time >= now. So, the delayed task will execute eventually. 

+}
+
+void GLRenderingVDAClient::RenderPicture(const media::Picture& picture) {
+  if (decoder_deleted())
+    return;
+  frame_delivery_times_.push_back(base::TimeTicks::Now());
+

[Pawel Osciak, 2013/07/15 08:07:06]

This is render time, not frame delivery time, which was in PictureReady.

[Owen Lin, 2013/07/16 10:46:54]

You're right. It just makes it simple to write the autotest code. I will log the
time in PictureReady and try to modified the code in autotest to measure the
frame drop rate.

   media::PictureBuffer* picture_buffer =
       picture_buffers_by_id_[picture.picture_buffer_id()];
   CHECK(picture_buffer);
   if (!suppress_rendering_) {
     rendering_helper_->RenderTexture(picture_buffer->texture_id());
   }
-
   if (num_decoded_frames_ > delay_reuse_after_frame_num_) {
     base::MessageLoop::current()->PostDelayedTask(FROM_HERE, base::Bind(
         &VideoDecodeAccelerator::ReusePictureBuffer,
@@ -490,6 +529,11 @@ void GLRenderingVDAClient::PictureReady(const media::Picture& picture) {
   } else {
     decoder_->ReusePictureBuffer(picture.picture_buffer_id());
   }
+  if (--pending_rendering_ == 0 && state_ == CS_FLUSHED) {
+    SetState(CS_COMPLETE_RENDERING);
+    decoder_->Reset();
+    SetState(CS_RESETTING);
+  }
 }
 
 void GLRenderingVDAClient::NotifyInitializeDone() {
@@ -519,8 +563,11 @@ void GLRenderingVDAClient::NotifyFlushDone() {
   DCHECK_GE(remaining_play_throughs_, 0);
   if (decoder_deleted())
     return;
-  decoder_->Reset();
-  SetState(CS_RESETTING);
+  if (pending_rendering_ == 0) {

[Ami GONE FROM CHROMIUM, 2013/07/15 18:54:14]

Why do none of the other notifications test for this?
I suspect that's a latent bug.

I also suspect that a DelayingVDAClient might make sense, to encapsulate the
concept of throttling delivery times.  If you do that (i.e. wrap the
GLRenderingVDAClient in a DelayingVDAClient) then I suspect you can make the CL
much more locally-contained, while ensuring that it's easy to see whether you
caught all the relevant codepaths (b/c ones where you ignore pending_rendering
will look more obvious like blind pass-throughs).

[Owen Lin, 2013/07/16 10:46:54]

I don't get it. Can you be more specific ?
Do you mean test for "pending_rendering_ == 0" ?

We care about the pending_rendering_ only when we have flushed all the remaining
pictures (i.e., state == CS_FLUSHED).

And there are only two cases when we get the FlushDone notification:

  1. There are some frames waiting for rendering ---> handled by l.532
  2. There are no frames waiting for rendering ---> handled here
If I understand correctly, you suggested me to add one more layer to delay the
PictureReady callback.

 decoder_ --> DelayingVDAClient --delayed--> GLRenderingVDAClient

It sounds really good. I did try to implement it. But I found it doesn't match
our need. Powal suggested we skip rendering for those dropped frames. In that
case, I will skip calling GLRenderingVDAClient::PictureReady. But it will lead
to incorrect value of num_decoded_frames_. I don't have a good solution for
that.

+    SetState(CS_COMPLETE_RENDERING);
+    decoder_->Reset();
+    SetState(CS_RESETTING);

[Ami GONE FROM CHROMIUM, 2013/07/15 18:54:14]

Duplicating this block here and at l.532 is a code-smell.
I think the right thing to do is to fake a delay of delivery of NFD() until
pending_rendering==0 (and drop l.532-536)

[Owen Lin, 2013/07/16 10:46:54]

Do you suggest me the following:

if (pending_rendering > 0) {
  PostDelayedTask(NotifyFlushDone, some_delay).
}

But it is also like a code-smell for the "some_delay". How can we decide the
delay, 10ms? why? Especially when we can know the exact timing to do it.

+  }
 }
 
 void GLRenderingVDAClient::NotifyResetDone() {
@@ -725,9 +772,9 @@ class VideoDecodeAcceleratorTest
 // its byte representation.
 ::std::ostream& operator<<(
     ::std::ostream& os,
-    const Tuple6<int, int, int, int, ResetPoint, ClientState>& t) {
+    const Tuple7<int, int, int, int, ResetPoint, ClientState, bool>& t) {
   return os << t.a << ", " << t.b << ", " << t.c << ", " << t.d << ", " << t.e
-            << ", " << t.f;
+            << ", " << t.f << ", " << t.g;
 }
 
 // Wait for |note| to report a state and if it's not |expected_state| then
@@ -750,9 +797,6 @@ enum { kMinSupportedNumConcurrentDecoders = 3 };
 // Test the most straightforward case possible: data is decoded from a single
 // chunk and rendered to the screen.
 TEST_P(VideoDecodeAcceleratorTest, TestSimpleDecode) {
-  // Can be useful for debugging VLOGs from OVDA.
-  // logging::SetMinLogLevel(-1);
-
   // Required for Thread to work.  Not used otherwise.
   base::ShadowingAtExitManager at_exit_manager;
 
@@ -771,7 +815,7 @@ TEST_P(VideoDecodeAcceleratorTest, TestSimpleDecode) {
   // Suppress GL rendering when we are logging the frame delivery time and a
   // few other tests, to cut down overall test runtime.
   const bool suppress_rendering = num_fragments_per_decode > 1 ||
-      frame_delivery_log != NULL;
+      content::disable_rendering;
 
   std::vector<ClientStateNotification*> notes(num_concurrent_decoders, NULL);
   std::vector<GLRenderingVDAClient*> clients(num_concurrent_decoders, NULL);
@@ -821,7 +865,7 @@ TEST_P(VideoDecodeAcceleratorTest, TestSimpleDecode) {
         num_fragments_per_decode, num_in_flight_decodes, num_play_throughs,
         video_file->reset_after_frame_num, delete_decoder_state,
         video_file->width, video_file->height, video_file->profile,
-        suppress_rendering, delay_after_frame_num);
+        target_fps, suppress_rendering, delay_after_frame_num);
     clients[index] = client;
 
     rendering_thread.message_loop()->PostTask(
@@ -860,12 +904,14 @@ TEST_P(VideoDecodeAcceleratorTest, TestSimpleDecode) {
           AssertWaitForStateOrDeleted(note, clients[i], CS_FLUSHING));
       ASSERT_NO_FATAL_FAILURE(
           AssertWaitForStateOrDeleted(note, clients[i], CS_FLUSHED));
-      // FlushDone requests Reset().
+      ASSERT_NO_FATAL_FAILURE(
+          AssertWaitForStateOrDeleted(note, clients[i], CS_COMPLETE_RENDERING));
       ASSERT_NO_FATAL_FAILURE(
           AssertWaitForStateOrDeleted(note, clients[i], CS_RESETTING));
     }
     ASSERT_NO_FATAL_FAILURE(
         AssertWaitForStateOrDeleted(note, clients[i], CS_RESET));
+
     // ResetDone requests Destroy().
     ASSERT_NO_FATAL_FAILURE(
         AssertWaitForStateOrDeleted(note, clients[i], CS_DESTROYED));
@@ -1039,6 +1085,16 @@ int main(int argc, char **argv) {
       content::frame_delivery_log = it->second.c_str();
       continue;
     }
+    if (it->first == "target_fps") {
+      double input_value = 0;
+      CHECK(base::StringToDouble(it->second, &input_value));
+      content::target_fps = static_cast<float>(input_value);

[Ami GONE FROM CHROMIUM, 2013/07/15 18:54:14]

why not just use double instead of float and avoid the cast?

[Owen Lin, 2013/07/16 10:46:54]

Done.

+      continue;
+    }
+    if (it->first == "disable_rendering") {
+      content::disable_rendering = true;
+      continue;
+    }
     if (it->first == "v" || it->first == "vmodule")
       continue;
     LOG(FATAL) << "Unexpected switch: " << it->first << ":" << it->second;