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

content/common/gpu/media/video_decode_accelerator_unittest.cc

 // Copyright (c) 2012 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.
 //
 // The bulk of this file is support code; sorry about that.  Here's an overview
 // to hopefully help readers of this code:
 // - RenderingHelper is charged with interacting with X11/{EGL/GLES2,GLX/GL} or
 //   Win/EGL.
 // - ClientState is an enum for the state of the decode client used by the test.
 // - ClientStateNotification is a barrier abstraction that allows the test code
@@ skipping 70 matching lines @@
 // - |profile| is the media::VideoCodecProfile set during Initialization.
 // An empty value for a numeric field means "ignore".
 const base::FilePath::CharType* test_video_data =
     // FILE_PATH_LITERAL("test-25fps.vp8:320:240:250:250:50:175:11");
     FILE_PATH_LITERAL("test-25fps.h264:320:240:250:258:50:175:1");
 
 // The path of the frame delivery time log. We can enable the log and specify
 // 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 {
   MID_STREAM_RESET = -2,
   END_OF_STREAM_RESET = -1
 };
 
 const int kMaxResetAfterFrameNum = 100;
 const int kMaxFramesToDelayReuse = 64;
 const int kReuseDelayMs = 1000;
@@ skipping 79 matching lines @@
 }
 
 // State of the GLRenderingVDAClient below.  Order matters here as the test
 // makes assumptions about it.
 enum ClientState {
   CS_CREATED = 0,
   CS_DECODER_SET = 1,
   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,
   CS_DESTROYED = 9,
   CS_MAX,  // Must be last entry.
 };
 
 // Helper class allowing one thread to wait on a notification from another.
 // If notifications come in faster than they are Wait()'d for, they are
 // accumulated (so exactly as many Wait() calls will unblock as Notify() calls
@@ skipping 55 matching lines @@
                        ClientStateNotification* note,
                        const std::string& encoded_data,
                        int num_fragments_per_decode,
                        int num_in_flight_decodes,
                        int num_play_throughs,
                        int reset_after_frame_num,
                        int delete_decoder_state,
                        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();
   void CreateDecoder();
 
   // VideoDecodeAccelerator::Client implementation.
   // The heart of the Client.
   virtual void ProvidePictureBuffers(uint32 requested_num_of_buffers,
                                      const gfx::Size& dimensions,
                                      uint32 texture_target) OVERRIDE;
@@ skipping 13 matching lines @@
   int num_skipped_fragments() { return num_skipped_fragments_; }
   int num_queued_fragments() { return num_queued_fragments_; }
   int num_decoded_frames() { return num_decoded_frames_; }
   double frames_per_second();
   bool decoder_deleted() { return !decoder_.get(); }
 
  private:
   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();
 
   // Compute & return the first encoded bytes (including a start frame) to send
   // to the decoder, starting at |start_pos| and returning
   // |num_fragments_per_decode| units. Skips to the first decodable position.
   std::string GetBytesForFirstFragments(size_t start_pos, size_t* end_pos);
   // Compute & return the next encoded bytes to send to the decoder (based on
   // |start_pos| & |num_fragments_per_decode_|).
@@ skipping 24 matching lines @@
   int num_skipped_fragments_;
   int num_queued_fragments_;
   int num_decoded_frames_;
   int num_done_bitstream_buffers_;
   PictureBufferById picture_buffers_by_id_;
   base::TimeTicks initialize_done_ticks_;
   int profile_;
   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(
     RenderingHelper* rendering_helper,
     int rendering_window_id,
     ClientStateNotification* note,
     const std::string& encoded_data,
     int num_fragments_per_decode,
     int num_in_flight_decodes,
     int num_play_throughs,
     int reset_after_frame_num,
     int delete_decoder_state,
     int frame_width,
     int frame_height,
     int profile,
+    float target_fps,
     bool suppress_rendering,
     int delay_reuse_after_frame_num)
     : rendering_helper_(rendering_helper),
       rendering_window_id_(rendering_window_id),
       encoded_data_(encoded_data),
       num_fragments_per_decode_(num_fragments_per_decode),
       num_in_flight_decodes_(num_in_flight_decodes), outstanding_decodes_(0),
       encoded_data_next_pos_to_decode_(0), next_bitstream_buffer_id_(0),
       note_(note),
       remaining_play_throughs_(num_play_throughs),
       reset_after_frame_num_(reset_after_frame_num),
       delete_decoder_state_(delete_decoder_state),
       state_(CS_CREATED),
       num_skipped_fragments_(0), num_queued_fragments_(0),
       num_decoded_frames_(0), num_done_bitstream_buffers_(0),
       profile_(profile),
       suppress_rendering_(suppress_rendering),
       delay_reuse_after_frame_num_(delay_reuse_after_frame_num) {
   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() {
   DeleteDecoder();  // Clean up in case of expected error.
   CHECK(decoder_deleted());
   STLDeleteValues(&picture_buffers_by_id_);
   SetState(CS_DESTROYED);
 }
 
 static bool DoNothingReturnTrue() { return true; }
@@ skipping 63 matching lines @@
   delete it->second;
   picture_buffers_by_id_.erase(it);
 }
 
 void GLRenderingVDAClient::PictureReady(const media::Picture& picture) {
   // We shouldn't be getting pictures delivered after Reset has completed.
   CHECK_LT(state_, CS_RESET);
 
   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.
   if (remaining_play_throughs_ == 1 &&
       reset_after_frame_num_ == num_decoded_frames_) {
     reset_after_frame_num_ = MID_STREAM_RESET;
     decoder_->Reset();
     // Re-start decoding from the beginning of the stream to avoid needing to
     // know how to find I-frames and so on in this test.
     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,
         base::Unretained(decoder_.get()), picture.picture_buffer_id()),
         base::TimeDelta::FromMilliseconds(kReuseDelayMs));
   } 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() {
   SetState(CS_INITIALIZED);
   initialize_done_ticks_ = base::TimeTicks::Now();
   for (int i = 0; i < num_in_flight_decodes_; ++i)
     DecodeNextFragments();
   DCHECK_EQ(outstanding_decodes_, num_in_flight_decodes_);
 }
 
 void GLRenderingVDAClient::NotifyEndOfBitstreamBuffer(
     int32 bitstream_buffer_id) {
   // TODO(fischman): this test currently relies on this notification to make
   // forward progress during a Reset().  But the VDA::Reset() API doesn't
   // guarantee this, so stop relying on it (and remove the notifications from
   // VaapiVideoDecodeAccelerator::FinishReset()).
   ++num_done_bitstream_buffers_;
   --outstanding_decodes_;
   DecodeNextFragments();
 }
 
 void GLRenderingVDAClient::NotifyFlushDone() {
   if (decoder_deleted())
     return;
   SetState(CS_FLUSHED);
   --remaining_play_throughs_;
   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() {
   if (decoder_deleted())
     return;
 
   if (reset_after_frame_num_ == MID_STREAM_RESET) {
     reset_after_frame_num_ = END_OF_STREAM_RESET;
     DecodeNextFragments();
     return;
@@ skipping 184 matching lines @@
 // - whether to test slow rendering by delaying ReusePictureBuffer().
 class VideoDecodeAcceleratorTest
     : public ::testing::TestWithParam<
   Tuple7<int, int, int, int, ResetPoint, ClientState, bool> > {
 };
 
 // Helper so that gtest failures emit a more readable version of the tuple than
 // 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
 // assert |client| has deleted its decoder.
 static void AssertWaitForStateOrDeleted(ClientStateNotification* note,
                                         GLRenderingVDAClient* client,
                                         ClientState expected_state) {
   ClientState state = note->Wait();
   if (state == expected_state) return;
   ASSERT_TRUE(client->decoder_deleted())
       << "Decoder not deleted but Wait() returned " << state
       << ", instead of " << expected_state;
 }
 
 // We assert a minimal number of concurrent decoders we expect to succeed.
 // Different platforms can support more concurrent decoders, so we don't assert
 // failure above this.
 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;
 
   const int num_fragments_per_decode = GetParam().a;
   const size_t num_concurrent_decoders = GetParam().b;
   const size_t num_in_flight_decodes = GetParam().c;
   const int num_play_throughs = GetParam().d;
   const int reset_point = GetParam().e;
   const int delete_decoder_state = GetParam().f;
   bool test_reuse_delay = GetParam().g;
 
   std::vector<TestVideoFile*> test_video_files;
   ParseAndReadTestVideoData(test_video_data, num_concurrent_decoders,
                             reset_point, &test_video_files);
 
   // 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);
 
   // Initialize the rendering helper.
   base::Thread rendering_thread("GLRenderingVDAClientThread");
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_DEFAULT;
 #if defined(OS_WIN)
   // For windows the decoding thread initializes the media foundation decoder
@@ skipping 29 matching lines @@
     if (test_reuse_delay &&
         kMaxFramesToDelayReuse * 2 < video_file->num_frames) {
       delay_after_frame_num = video_file->num_frames - kMaxFramesToDelayReuse;
     }
 
     GLRenderingVDAClient* client = new GLRenderingVDAClient(
         rendering_helper.get(), index, note, video_file->data_str,
         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(
         FROM_HERE,
         base::Bind(&GLRenderingVDAClient::CreateDecoder,
                    base::Unretained(client)));
 
     ASSERT_EQ(note->Wait(), CS_DECODER_SET);
   }
   // Then wait for all the decodes to finish.
@@ skipping 18 matching lines @@
       if (n > 0) {
         ASSERT_NO_FATAL_FAILURE(
             AssertWaitForStateOrDeleted(note, clients[i], CS_INITIALIZED));
       }
       // InitializeDone kicks off decoding inside the client, so we just need to
       // wait for Flush.
       ASSERT_NO_FATAL_FAILURE(
           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));
   }
   // Finally assert that decoding went as expected.
   for (size_t i = 0; i < num_concurrent_decoders &&
            !skip_performance_and_correctness_checks; ++i) {
     // We can only make performance/correctness assertions if the decoder was
     // allowed to finish.
     if (delete_decoder_state < CS_FLUSHED)
@@ skipping 153 matching lines @@
   for (CommandLine::SwitchMap::const_iterator it = switches.begin();
        it != switches.end(); ++it) {
     if (it->first == "test_video_data") {
       content::test_video_data = it->second.c_str();
       continue;
     }
     if (it->first == "frame_delivery_log") {
       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;
   }
 
   base::ShadowingAtExitManager at_exit_manager;
 
 #if defined(OS_WIN)
   content::DXVAVideoDecodeAccelerator::PreSandboxInitialization();
 #elif defined(OS_CHROMEOS)
 #if defined(ARCH_CPU_ARMEL)
   content::ExynosVideoDecodeAccelerator::PreSandboxInitialization();
 #elif defined(ARCH_CPU_X86_FAMILY)
   content::VaapiWrapper::PreSandboxInitialization();
 #endif  // ARCH_CPU_ARMEL
 #endif  // OS_CHROMEOS
 
   return RUN_ALL_TESTS();
 }