vea_unittest - Control the rate of input frames.

content/common/gpu/media/video_encode_accelerator_unittest.cc

Index: content/common/gpu/media/video_encode_accelerator_unittest.cc
diff --git a/content/common/gpu/media/video_encode_accelerator_unittest.cc b/content/common/gpu/media/video_encode_accelerator_unittest.cc
index b1fb59226668a32f193cab0b3021bc9ac5b04df7..4f698518c8dbdf8575594eccfafb544052a4ced2 100644
--- a/content/common/gpu/media/video_encode_accelerator_unittest.cc
+++ b/content/common/gpu/media/video_encode_accelerator_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "content/common/gpu/media/video_accelerator_unittest_helpers.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/bitstream_buffer.h"
@@ -463,6 +464,7 @@ scoped_ptr<StreamValidator> StreamValidator::Create(
 class VEAClient : public VideoEncodeAccelerator::Client {
  public:
   VEAClient(TestStream* test_stream,
+            double input_fps,

[Pawel Osciak, 2014/12/16 10:56:40]

Would it make more sense to make it an argument per test stream?

[Owen Lin, 2014/12/17 05:27:29]

Sound reasonable. Done.

             ClientStateNotification<ClientState>* note,
             bool save_to_file,
             unsigned int keyframe_period,
@@ -497,9 +499,10 @@ class VEAClient : public VideoEncodeAccelerator::Client {
   // Called when encoder is done with a VideoFrame.
   void InputNoLongerNeededCallback(int32 input_id);
 
-  // Ensure encoder has at least as many inputs as it asked for
-  // via RequireBitstreamBuffers().
-  void FeedEncoderWithInputs();
+  // Feed the encoder with one input frame unless it has more input buffers
+  // than it asked for via RequireBitstreamBuffers(). Return false if no input

[Pawel Osciak, 2014/12/16 10:56:40]

I think you meant unless it has as many buffers as it asked, not more.

[Owen Lin, 2014/12/17 05:27:29]

Done.

+  // frame was fed.
+  bool FeedEncoderWithOneInput();
 
   // Provide the encoder with a new output buffer.
   void FeedEncoderWithOutput(base::SharedMemory* shm);
@@ -532,6 +535,7 @@ class VEAClient : public VideoEncodeAccelerator::Client {
   scoped_ptr<VideoEncodeAccelerator> encoder_;
 
   TestStream* test_stream_;
+
   // Used to notify another thread about the state. VEAClient does not own this.
   ClientStateNotification<ClientState>* note_;
 
@@ -613,9 +617,16 @@ class VEAClient : public VideoEncodeAccelerator::Client {
 
   // Framerate to switch to in the middle of the stream.
   unsigned int requested_subsequent_framerate_;
+
+  // The timer used to feed the encoder with the input frames.
+  scoped_ptr<base::RepeatingTimer<VEAClient>> input_timer_;
+
+  // The interval between two input frames.
+  base::TimeDelta input_duration_;
 };
 
 VEAClient::VEAClient(TestStream* test_stream,
+                     double input_fps,
                      ClientStateNotification<ClientState>* note,
                      bool save_to_file,
                      unsigned int keyframe_period,
@@ -656,6 +667,9 @@ VEAClient::VEAClient(TestStream* test_stream,
 
   CHECK(validator_.get());
 
+  if (input_fps > 0)
+    input_duration_ = base::TimeDelta::FromSeconds(1) / input_fps;
+
   if (save_to_file_) {
     CHECK(!test_stream_->out_filename.empty());
     base::FilePath out_filename(test_stream_->out_filename);
@@ -706,6 +720,7 @@ void VEAClient::DestroyEncoder() {
   if (!has_encoder())
     return;
   encoder_.reset();
+  input_timer_.reset();
 }
 
 void VEAClient::UpdateTestStreamData(bool mid_stream_bitrate_switch,
@@ -792,7 +807,16 @@ void VEAClient::RequireBitstreamBuffers(unsigned int input_count,
   }
 
   encode_start_time_ = base::TimeTicks::Now();
-  FeedEncoderWithInputs();
+  if (input_duration_ == base::TimeDelta()) {
+    while (FeedEncoderWithOneInput())
+      ;
+  } else {
+    input_timer_.reset(new base::RepeatingTimer<VEAClient>());
+    input_timer_->Start(
+        FROM_HERE, input_duration_,
+        base::Bind(base::IgnoreResult(&VEAClient::FeedEncoderWithOneInput),
+                   base::Unretained(this)));
+  }
 }
 
 void VEAClient::BitstreamBufferReady(int32 bitstream_buffer_id,
@@ -856,7 +880,8 @@ void VEAClient::InputNoLongerNeededCallback(int32 input_id) {
   std::set<int32>::iterator it = inputs_at_client_.find(input_id);
   ASSERT_NE(it, inputs_at_client_.end());
   inputs_at_client_.erase(it);
-  FeedEncoderWithInputs();
+  if (input_duration_ == base::TimeDelta())
+    DCHECK(FeedEncoderWithOneInput());

[Pawel Osciak, 2014/12/16 10:56:40]

Isn't this basically a DCHECK on whether the encoder is fast enough or not? This
makes it pretty indeterministic. I would prefer not having this.

[Owen Lin, 2014/12/17 05:27:29]

No, this callback called when the encoder free an input buffer. So, it is
expected we can feed one input buffer to the encoder. However, I have refactored
the code and FeedEncoderWithOneInput no longer return bool, so I removed the
DCHECK as well.

 }
 
 scoped_refptr<media::VideoFrame> VEAClient::PrepareInputFrame(off_t position) {
@@ -895,39 +920,43 @@ scoped_refptr<media::VideoFrame> VEAClient::PrepareInputFrame(off_t position) {
   return frame;
 }
 
-void VEAClient::FeedEncoderWithInputs() {
-  if (!has_encoder())
-    return;
-
-  if (state_ != CS_ENCODING)
-    return;
-
-  while (inputs_at_client_.size() <
-         num_required_input_buffers_ + kNumExtraInputFrames) {
-    size_t bytes_left =
-        test_stream_->mapped_aligned_in_file.length() - pos_in_input_stream_;
-    if (bytes_left < test_stream_->aligned_buffer_size) {
-      DCHECK_EQ(bytes_left, 0UL);
-      // Rewind if at the end of stream and we are still encoding.
-      // This is to flush the encoder with additional frames from the beginning
-      // of the stream, or if the stream is shorter that the number of frames
-      // we require for bitrate tests.
-      pos_in_input_stream_ = 0;
-      continue;
-    }
+bool VEAClient::FeedEncoderWithOneInput() {
+  if (!has_encoder() || state_ != CS_ENCODING) {
+    // Destructor will also stop the timer.

[Pawel Osciak, 2014/12/16 10:56:40]

If so, do we need to do this here as well?

[Owen Lin, 2014/12/17 05:27:29]

I would prefer to cancel the timer as early as we don't need it. But it should
make no harm of removing it excepts this function will continue being called
until the encoder gets destroyed.

[Pawel Osciak, 2014/12/18 04:54:10]

Ok, then the comment is confusing. It suggests that we are doing something
superficial here. It would have been better to say what you explained above, or
not have a comment at all.

[Owen Lin, 2014/12/18 08:38:11]

Done.

+    input_timer_.reset();
+    return false;
+  }
 
-    bool force_keyframe = false;
-    if (keyframe_period_ && next_input_id_ % keyframe_period_ == 0) {
-      keyframe_requested_at_ = next_input_id_;
-      force_keyframe = true;
-    }
+  size_t bytes_left =
+      test_stream_->mapped_aligned_in_file.length() - pos_in_input_stream_;
+  if (bytes_left < test_stream_->aligned_buffer_size) {
+    DCHECK_EQ(bytes_left, 0UL);
+    // Rewind if at the end of stream and we are still encoding.
+    // This is to flush the encoder with additional frames from the beginning
+    // of the stream, or if the stream is shorter that the number of frames
+    // we require for bitrate tests.
+    pos_in_input_stream_ = 0;
+  }
 
-    scoped_refptr<media::VideoFrame> video_frame =
-        PrepareInputFrame(pos_in_input_stream_);
+  if (inputs_at_client_.size() >=

[Pawel Osciak, 2014/12/16 10:56:41]

I'd extract this out from here and have the while loop in the caller.

[Owen Lin, 2014/12/17 05:27:29]

Done. I have moved this out to the while loop in the caller. Due to this change
we need another callback function for the |input_timer_|.

+      num_required_input_buffers_ + kNumExtraInputFrames) {
+    DVLOG(1) << "Drop input frame: " + pos_in_input_stream_;

[Pawel Osciak, 2014/12/16 10:56:40]

s/Drop/Dropping/

[Owen Lin, 2014/12/17 05:27:29]

Done.

     pos_in_input_stream_ += test_stream_->aligned_buffer_size;
+    return false;
+  }
 
-    encoder_->Encode(video_frame, force_keyframe);
+  bool force_keyframe = false;
+  if (keyframe_period_ && next_input_id_ % keyframe_period_ == 0) {
+    keyframe_requested_at_ = next_input_id_;
+    force_keyframe = true;
   }
+
+  scoped_refptr<media::VideoFrame> video_frame =
+      PrepareInputFrame(pos_in_input_stream_);
+  pos_in_input_stream_ += test_stream_->aligned_buffer_size;
+
+  encoder_->Encode(video_frame, force_keyframe);
+  return true;
 }
 
 void VEAClient::FeedEncoderWithOutput(base::SharedMemory* shm) {
@@ -1024,8 +1053,10 @@ void VEAClient::VerifyStreamProperties() {
 class VideoEncodeAcceleratorTestEnvironment : public ::testing::Environment {
  public:
   VideoEncodeAcceleratorTestEnvironment(
-      scoped_ptr<base::FilePath::StringType> data) {
+      scoped_ptr<base::FilePath::StringType> data,
+      double input_fps) {
     test_stream_data_ = data.Pass();
+    input_fps_ = input_fps;
   }
 
   virtual void SetUp() {
@@ -1040,6 +1071,9 @@ class VideoEncodeAcceleratorTestEnvironment : public ::testing::Environment {
 
   ScopedVector<TestStream> test_streams_;
 
+  // The input frame rate to the encoder.
+  double input_fps_;
+
  private:
   scoped_ptr<base::FilePath::StringType> test_stream_data_;
 };
@@ -1080,14 +1114,10 @@ TEST_P(VideoEncodeAcceleratorTest, TestSimpleEncode) {
          !g_env->test_streams_[test_stream_index]->out_filename.empty());
 
     notes.push_back(new ClientStateNotification<ClientState>());
-    clients.push_back(new VEAClient(g_env->test_streams_[test_stream_index],
-                                    notes.back(),
-                                    encoder_save_to_file,
-                                    keyframe_period,
-                                    force_bitrate,
-                                    test_perf,
-                                    mid_stream_bitrate_switch,
-                                    mid_stream_framerate_switch));
+    clients.push_back(new VEAClient(
+        g_env->test_streams_[test_stream_index], g_env->input_fps_,

[Pawel Osciak, 2014/12/16 10:56:40]

Uh... did git cl format do this? The previous version was much more readable...

[Owen Lin, 2014/12/17 05:27:29]

I feel the same way, but it's git cl format. Due to refactoring, this code is no
longer changed.

+        notes.back(), encoder_save_to_file, keyframe_period, force_bitrate,
+        test_perf, mid_stream_bitrate_switch, mid_stream_framerate_switch));
 
     encoder_thread.message_loop()->PostTask(
         FROM_HERE,
@@ -1184,6 +1214,7 @@ int main(int argc, char** argv) {
   const base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
   DCHECK(cmd_line);
 
+  double input_fps = 0;
   base::CommandLine::SwitchMap switches = cmd_line->GetSwitches();
   for (base::CommandLine::SwitchMap::const_iterator it = switches.begin();
        it != switches.end();
@@ -1192,6 +1223,13 @@ int main(int argc, char** argv) {
       test_stream_data->assign(it->second.c_str());
       continue;
     }
+    if (it->first == "input_fps") {
+      // On Windows, CommandLine::StringType is wstring. We need to convert
+      // it to std::string first
+      std::string input(it->second.begin(), it->second.end());
+      CHECK(base::StringToDouble(input, &input_fps));
+      continue;
+    }
     if (it->first == "v" || it->first == "vmodule")
       continue;
     LOG(FATAL) << "Unexpected switch: " << it->first << ":" << it->second;
@@ -1201,7 +1239,7 @@ int main(int argc, char** argv) {
       reinterpret_cast<content::VideoEncodeAcceleratorTestEnvironment*>(
           testing::AddGlobalTestEnvironment(
               new content::VideoEncodeAcceleratorTestEnvironment(
-                  test_stream_data.Pass())));
+                  test_stream_data.Pass(), input_fps)));
 
   return RUN_ALL_TESTS();
 }