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..1689203d9cf61e0b82290dd3e76be2734c25cdb0 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 "--rendering_fps".
+double rendering_fps = 0;
+
+// Disable rendering, the value is set by the switch "--disable_rendering".
+bool disable_rendering = false;

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

This is not mentioned in the CL description.

[Owen Lin, 2013/07/31 10:01:36]

Done.

+
 // Magic constants for differentiating the reasons for NotifyResetDone being
 // called.
 enum ResetPoint {
@@ -187,11 +193,10 @@ enum ClientState {
   CS_INITIALIZED = 2,
   CS_FLUSHING = 3,
   CS_FLUSHED = 4,
-  CS_DONE = 5,
-  CS_RESETTING = 6,
-  CS_RESET = 7,
-  CS_ERROR = 8,
-  CS_DESTROYED = 9,
+  CS_RESETTING = 5,
+  CS_RESET = 6,
+  CS_ERROR = 7,
+  CS_DESTROYED = 8,
   CS_MAX,  // Must be last entry.
 };
 
@@ -233,6 +238,140 @@ ClientState ClientStateNotification::Wait() {
   return ret;
 }
 
+class ThrottlingVDAClient : public VideoDecodeAccelerator::Client {

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

doco the point of this class.

[Owen Lin, 2013/07/31 10:01:36]

Done.

+ public:
+  // Callback invoked whan the picture is dropped and should be reused for
+  // the decoder again.
+  typedef base::Callback<void(int32 picture_buffer_id)> ReusePictureCB;
+
+  ThrottlingVDAClient(VideoDecodeAccelerator::Client*,

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

missing param name

[Owen Lin, 2013/07/31 10:01:36]

Done.

+                      double fps,
+                      ReusePictureCB reuse_pic_cb);

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

s/pic/picture/

[Owen Lin, 2013/07/31 10:01:36]

Done.

+  virtual ~ThrottlingVDAClient();
+
+  virtual void ProvidePictureBuffers(uint32 requested_num_of_buffers,
+                                     const gfx::Size& dimensions,
+                                     uint32 texture_target) OVERRIDE;
+  virtual void DismissPictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  virtual void PictureReady(const media::Picture& picture) OVERRIDE;
+
+  // Simple state changes.

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

What's so "simple" about them? :)
Generally chrome code docos overridden virtuals by grouping them all together
(in the order of declaration in the base class) and commenting 
// FooInterface implementation.
like 
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/codec/vid...

[Owen Lin, 2013/07/31 10:01:36]

I just copy the description from GLRenderingVDAClient.
Thanks.

+  virtual void NotifyInitializeDone() OVERRIDE;
+  virtual void NotifyEndOfBitstreamBuffer(int32 bitstream_buffer_id) OVERRIDE;
+  virtual void NotifyFlushDone() OVERRIDE;
+  virtual void NotifyResetDone() OVERRIDE;
+  virtual void NotifyError(VideoDecodeAccelerator::Error error) OVERRIDE;
+
+  int num_decoded_frames() { return num_decoded_frames_; }
+
+ private:
+  VideoDecodeAccelerator::Client* client_;
+  ReusePictureCB reuse_pic_cb_;
+  base::TimeTicks last_frame_delivered_time_;
+  base::TimeDelta frame_duration_;
+
+  int num_decoded_frames_;
+  int pending_pictures_;
+
+  void CallClientPictureReady(const media::Picture& picture);

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

style: methods go above members

[Owen Lin, 2013/07/31 10:01:36]

Done.

+};

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

DISALLOW_IMPLICIT_CONSTRUCTORS

[Owen Lin, 2013/07/31 10:01:36]

Done.

+
+ThrottlingVDAClient::ThrottlingVDAClient(
+    VideoDecodeAccelerator::Client* client,
+    double fps,
+    ReusePictureCB reuse_pic_cb)
+    : client_(client),
+      reuse_pic_cb_(reuse_pic_cb),
+      num_decoded_frames_(0),
+      pending_pictures_(0) {
+  CHECK(client_);
+  CHECK_GT(fps, 0);
+  frame_duration_ = base::TimeDelta::FromSeconds(1) / fps;
+}
+
+ThrottlingVDAClient::~ThrottlingVDAClient() {}
+
+void ThrottlingVDAClient::ProvidePictureBuffers(
+    uint32 requested_num_of_buffers,
+    const gfx::Size& dimensions,
+    uint32 texture_target) {
+  client_->ProvidePictureBuffers(requested_num_of_buffers,
+                                 dimensions,
+                                 texture_target);
+}
+
+void ThrottlingVDAClient::DismissPictureBuffer(int32 picture_buffer_id) {
+  client_->DismissPictureBuffer(picture_buffer_id);
+}
+
+void ThrottlingVDAClient::PictureReady(const media::Picture& picture) {
+    base::TimeTicks now = base::TimeTicks::Now();

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

indent is off here and elsewhere.

[Owen Lin, 2013/07/31 10:01:36]

Done.

+    if (num_decoded_frames_ == 0)
+      last_frame_delivered_time_ = now;
+    else
+      last_frame_delivered_time_ += frame_duration_;

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

This var name doesn't make sense.
Is it supposed to embody "time of beginning of duration of the latest frame
scheduled for rendering"?  Because that's what I _think_ it's computing.  But
then the test at l.316 below is wrong, because if "now" falls in the middle of
the frame's duration you wouldn't drop it, you'd still want to show it (for less
than its scheduled duration if the next frame is delivered on time).

Basically I think you want to rename some things and probably rejigger some
other things in this method :)

[Owen Lin, 2013/07/31 10:01:36]

The code has been changed to address the NotifyResetDone() issue you mentioned.

+
+    ++num_decoded_frames_;
+
+    if (last_frame_delivered_time_ < now) {
+      // Frame dropped, skip rendering.
+      LOG(INFO) << "Frame dropped";
+      last_frame_delivered_time_ += frame_duration_;
+      reuse_pic_cb_.Run(picture.picture_buffer_id());
+      return;
+    }
+    ++pending_pictures_;
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&ThrottlingVDAClient::CallClientPictureReady,
+                   base::Unretained(this),
+                   picture),
+        last_frame_delivered_time_ - now);
+}
+
+void ThrottlingVDAClient::CallClientPictureReady(
+    const media::Picture& picture) {
+  --pending_pictures_;
+  client_->PictureReady(picture);
+}
+
+void ThrottlingVDAClient::NotifyInitializeDone() {
+  client_->NotifyInitializeDone();
+}
+
+void ThrottlingVDAClient::NotifyEndOfBitstreamBuffer(
+    int32 bitstream_buffer_id) {
+  client_->NotifyEndOfBitstreamBuffer(bitstream_buffer_id);
+}
+
+void ThrottlingVDAClient::NotifyFlushDone() {
+  if (pending_pictures_ > 0) {
+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&ThrottlingVDAClient::NotifyFlushDone,
+                   base::Unretained(this)),
+        last_frame_delivered_time_ - base::TimeTicks::Now());
+    return;
+  }
+  client_->NotifyFlushDone();
+}
+
+void ThrottlingVDAClient::NotifyResetDone() {
+  if (pending_pictures_ > 0) {

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

This delays reset for rendering, which is unlikely to mirror what a real
VDA::Client does; real clients drop queued frames when they request a Reset(). 
I think you want to do the same here.

[Owen Lin, 2013/07/31 10:01:36]

Done.

+    base::MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&ThrottlingVDAClient::NotifyResetDone,
+                   base::Unretained(this)),
+        last_frame_delivered_time_ - base::TimeTicks::Now());
+    return;
+  }
+  client_->NotifyResetDone();
+}
+
+void ThrottlingVDAClient::NotifyError(VideoDecodeAccelerator::Error error) {
+  client_->NotifyError(error);
+}
+
 // Client that can accept callbacks from a VideoDecodeAccelerator and is used by
 // the TESTs below.
 class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
@@ -249,6 +388,10 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
   // calls have been made, N>=0 means interpret as ClientState.
   // Both |reset_after_frame_num| & |delete_decoder_state| apply only to the
   // last play-through (governed by |num_play_throughs|).
+  // |rendering_fps| indicates the target rendering fps. 0 means no target fps
+  // and it would render as fast as possible.
+  // Use |suppress_rendering| to skip the rendering part, so it only decodes

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

Rewrite this line to match the rest of the paragraph

[Owen Lin, 2013/07/31 10:01:36]

Done.

+  // the frames and won't shown on display.
   // After |delay_reuse_after_frame_num| frame has been delivered, the client
   // will start delaying the call to ReusePictureBuffer() for kReuseDelayMs.
   GLRenderingVDAClient(RenderingHelper* rendering_helper,
@@ -263,6 +406,7 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
                        int frame_width,
                        int frame_height,
                        int profile,
+                       double rendering_fps,
                        bool suppress_rendering,
                        int delay_reuse_after_frame_num);
   virtual ~GLRenderingVDAClient();
@@ -284,6 +428,8 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
 
   void OutputFrameDeliveryTimes(base::PlatformFile output);
 
+  void NotifyFrameDropped(int32 picture_buffer_id);
+
   // Simple getters for inspecting the state of the Client.
   int num_done_bitstream_buffers() { return num_done_bitstream_buffers_; }
   int num_skipped_fragments() { return num_skipped_fragments_; }
@@ -340,6 +486,7 @@ class GLRenderingVDAClient : public VideoDecodeAccelerator::Client {
   bool suppress_rendering_;
   std::vector<base::TimeTicks> frame_delivery_times_;
   int delay_reuse_after_frame_num_;
+  scoped_ptr<ThrottlingVDAClient> throttling_client_;
 };
 
 GLRenderingVDAClient::GLRenderingVDAClient(
@@ -355,6 +502,7 @@ GLRenderingVDAClient::GLRenderingVDAClient(
     int frame_width,
     int frame_height,
     int profile,
+    double rendering_fps,
     bool suppress_rendering,
     int delay_reuse_after_frame_num)
     : rendering_helper_(rendering_helper),
@@ -376,6 +524,13 @@ GLRenderingVDAClient::GLRenderingVDAClient(
   CHECK_GT(num_fragments_per_decode, 0);
   CHECK_GT(num_in_flight_decodes, 0);
   CHECK_GT(num_play_throughs, 0);
+  CHECK_GE(rendering_fps, 0);
+  if (rendering_fps > 0)
+    throttling_client_.reset(new ThrottlingVDAClient(
+        this,
+        rendering_fps,
+        base::Bind(&GLRenderingVDAClient::NotifyFrameDropped,
+                   base::Unretained(this))));
 }
 
 GLRenderingVDAClient::~GLRenderingVDAClient() {
@@ -390,21 +545,25 @@ static bool DoNothingReturnTrue() { return true; }
 void GLRenderingVDAClient::CreateDecoder() {
   CHECK(decoder_deleted());
   CHECK(!decoder_.get());
+
+  VideoDecodeAccelerator::Client* client = this;
+  if (throttling_client_)
+      client = throttling_client_.get();
 #if defined(OS_WIN)
   decoder_.reset(new DXVAVideoDecodeAccelerator(
-      this, base::Bind(&DoNothingReturnTrue)));
+      client, base::Bind(&DoNothingReturnTrue)));
 #elif defined(OS_CHROMEOS)
 #if defined(ARCH_CPU_ARMEL)
   decoder_.reset(
       new ExynosVideoDecodeAccelerator(
           static_cast<EGLDisplay>(rendering_helper_->GetGLDisplay()),
           static_cast<EGLContext>(rendering_helper_->GetGLContext()),
-          this, base::Bind(&DoNothingReturnTrue)));
+          client, base::Bind(&DoNothingReturnTrue)));
 #elif defined(ARCH_CPU_X86_FAMILY)
   decoder_.reset(new VaapiVideoDecodeAccelerator(
       static_cast<Display*>(rendering_helper_->GetGLDisplay()),
       static_cast<GLXContext>(rendering_helper_->GetGLContext()),
-      this, base::Bind(&DoNothingReturnTrue)));
+      client, base::Bind(&DoNothingReturnTrue)));
 #endif  // ARCH_CPU_ARMEL
 #endif  // OS_WIN
   CHECK(decoder_.get());
@@ -459,10 +618,8 @@ 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_;
+  frame_delivery_times_.push_back(base::TimeTicks::Now());
 
   // Mid-stream reset applies only to the last play-through per constructor
   // comment.
@@ -475,17 +632,19 @@ void GLRenderingVDAClient::PictureReady(const media::Picture& picture) {
     encoded_data_next_pos_to_decode_ = 0;
   }
 
+  CHECK_LE(picture.bitstream_buffer_id(), next_bitstream_buffer_id_);
+  ++num_decoded_frames_;

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

Why did you move these two lines?

[Owen Lin, 2013/07/31 10:01:36]

Move them back.

+
   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()),
+        decoder_->AsWeakPtr(), picture.picture_buffer_id()),

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

Should be gone after rebase?

[Owen Lin, 2013/07/31 10:01:36]

Yes. It should be gone.

         base::TimeDelta::FromMilliseconds(kReuseDelayMs));
   } else {
     decoder_->ReusePictureBuffer(picture.picture_buffer_id());
@@ -508,7 +667,9 @@ void GLRenderingVDAClient::NotifyEndOfBitstreamBuffer(
   // VaapiVideoDecodeAccelerator::FinishReset()).
   ++num_done_bitstream_buffers_;
   --outstanding_decodes_;
-  DecodeNextFragments();
+  // Don't decode if it is in mid-stream resetting
+  if (reset_after_frame_num_ != MID_STREAM_RESET)

[Owen Lin, 2013/07/24 06:30:45]

This is for the MidStreamReset test case.

In the new code, we delay the "NotifyResetDone" until all PictureReady. While we
are waiting for it, we still keep feeding data to the decoder. Eventually, it
will call flush() and change the state to FLUSHING. This is not what we want. I
think we should stop feeding data here.

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

See comment above; I think you don't want this and instead want to change how
TVC-based Resetting works.

[Owen Lin, 2013/07/31 10:01:36]

Done.

+    DecodeNextFragments();

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

AFAICT the point of this CL is to slow down decode to match a target rendering
FPS, to evaluate the CPU power needed for a realistic decode (i.e. not a
full-speed, non-rendering decode) and to ensure that the decoder can keep up
with the desired frame-rate (i.e. that there are no dropped frames).

IWBN if the CL description was updated to reflect the result of these goals on
at least one platform.  Something like: "using test video XXX with target FPS
YYY CPU rate goes from xx to yy and dropped frames are at zzz".

[Owen Lin, 2013/07/31 10:01:36]

Done.

 }
 
 void GLRenderingVDAClient::NotifyFlushDone() {
@@ -517,8 +678,6 @@ void GLRenderingVDAClient::NotifyFlushDone() {
   SetState(CS_FLUSHED);
   --remaining_play_throughs_;
   DCHECK_GE(remaining_play_throughs_, 0);
-  if (decoder_deleted())
-    return;
   decoder_->Reset();

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

Why do you no longer need the decoder_deleted() guard?

[Owen Lin, 2013/07/31 10:01:36]

Oops, reverted. I didn't notice that decoder would be deleted in SetState().

   SetState(CS_RESETTING);
 }
@@ -562,6 +721,10 @@ void GLRenderingVDAClient::OutputFrameDeliveryTimes(base::PlatformFile output) {
   }
 }
 
+void GLRenderingVDAClient::NotifyFrameDropped(int32 picture_buffer_id) {
+  decoder_->ReusePictureBuffer(picture_buffer_id);

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

Shouldn't this be counting dropped frames or something?

(more useful would probably be to track max _dropped frames per second_, but if
the total count of dropped frames is zero then that matters less)

[Owen Lin, 2013/07/31 10:01:36]

I plan to derive the number of dropped frame from frame's delivery times. 
So, I don't need to test or log it here.

+}
+
 static bool LookingAtNAL(const std::string& encoded, size_t pos) {
   return encoded[pos] == 0 && encoded[pos + 1] == 0 &&
       encoded[pos + 2] == 0 && encoded[pos + 3] == 1;
@@ -725,9 +888,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 +913,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 +931,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;

[Ami GONE FROM CHROMIUM, 2013/07/24 18:23:16]

You dropped
frame_delivery_log != NULL
from the condition, but didn't update the comment.

I'm not sure whether the right answer is to update the comment or to restore the
condition.  Can you choose one and explain your choice?

[Owen Lin, 2013/07/31 10:01:36]

A new switch "--disable_rendering" was add in this change. 
So that we can control it when we have different type of test cases. 

 
   std::vector<ClientStateNotification*> notes(num_concurrent_decoders, NULL);
   std::vector<GLRenderingVDAClient*> clients(num_concurrent_decoders, NULL);
@@ -821,7 +981,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);
+        rendering_fps, suppress_rendering, delay_after_frame_num);
     clients[index] = client;
 
     rendering_thread.message_loop()->PostTask(
@@ -860,12 +1020,12 @@ 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_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 +1199,14 @@ int main(int argc, char **argv) {
       content::frame_delivery_log = it->second.c_str();
       continue;
     }
+    if (it->first == "rendering_fps") {
+      CHECK(base::StringToDouble(it->second, &content::rendering_fps));
+      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;