vea_unittest: Calculate per-frame encode latency

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 52be56b5c3541aaacf2a9b6f103c7a48ea0eecef..6ed041d03a1fd5fb1c66d58c7be6e0eca6204825 100644
--- a/content/common/gpu/media/video_encode_accelerator_unittest.cc
+++ b/content/common/gpu/media/video_encode_accelerator_unittest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
+
 #include "base/at_exit.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -568,9 +570,13 @@ class VEAClient : public VideoEncodeAccelerator::Client {
   void CreateEncoder();
   void DestroyEncoder();
 
-  // Return the number of encoded frames per second.
+  // Returns the number of encoded frames per second.

[Owen Lin, 2015/05/04 09:37:12]

why Return"s"? Most comments in this file use "Return". I think we should just
follow it.

[Justin Chuang, 2015/05/04 10:25:09]

OK. The correct way should be "returns" for first person comments in docstring,
but "return" in other comment for code block. (it's defined somewhere in coding
convention, and I was corrected by others before) 
But consistency is more important than rules. Will follow it.

[Justin Chuang, 2015/05/06 10:35:32]

Done.

   double frames_per_second();
 
+  // Returns a list of encode latency of the frames at the percentages in

[Owen Lin, 2015/05/04 09:37:12]

Why make it public? it is only used in VEAClient.

In fact, I would suggest just have a helper function to calculate the
percentile.

static float percentile(
    const vector<float>& sorted_values, float percentage);

[Justin Chuang, 2015/05/04 10:25:09]

I put this close to frames_per_second(). Or maybe I should move both methods to
private?
Sounds good. Will do.

[Owen Lin, 2015/05/05 06:03:13]

I see. It sounds we should move frames_per_second() to private as well.

[Justin Chuang, 2015/05/06 10:35:32]

Done.

+  // |ratios|. The values in |ratios| must be between 0 and 1.0.
+  std::vector<base::TimeDelta> encode_latency(const std::vector<float>& ratios);
+
   // VideoDecodeAccelerator::Client implementation.
   void RequireBitstreamBuffers(unsigned int input_count,
                                const gfx::Size& input_coded_size,
@@ -622,9 +628,11 @@ class VEAClient : public VideoEncodeAccelerator::Client {
   // Write an IVF frame header to test_stream_->out_filename.
   void WriteIvfFrameHeader(int frame_index, size_t frame_size);
 
-  // Prepare and return a frame wrapping the data at |position| bytes in
-  // the input stream, ready to be sent to encoder.
-  scoped_refptr<media::VideoFrame> PrepareInputFrame(off_t position);
+  // Prepare and return a frame wrapping the data at |position| bytes in the
+  // input stream, ready to be sent to encoder. Returns a pair of video frame
+  // and the input frame id.
+  std::pair<scoped_refptr<media::VideoFrame>, int32> PrepareInputFrame(

[Owen Lin, 2015/05/04 09:37:12]

We already has the next_input_id_ using around the code. How about just using it
instead of changing the interface?

[Justin Chuang, 2015/05/04 10:25:09]

But the value is incremented by 1 than the value I need after the function call.
I can pick the next_input_id_ value *before* calling PrepareInputFrame, or
decrement the value by one *after* calling it, etc.
IMO either way lacks encapsulation (why should the caller know this function
controls next_input_id_?). We can argue variable is in the context of this
class, but this class is big. 
Returning the input id of the frame to be encoded doesn't seem too terrible
compared to alternative methods.

[Owen Lin, 2015/05/05 06:03:13]

I got your point. See my reply below.

+      off_t position);
 
   // Update the parameters according to |mid_stream_bitrate_switch| and
   // |mid_stream_framerate_switch|.
@@ -642,11 +650,16 @@ class VEAClient : public VideoEncodeAccelerator::Client {
   // Used to notify another thread about the state. VEAClient does not own this.
   ClientStateNotification<ClientState>* note_;
 
-  // Ids assigned to VideoFrames (start at 1 for easy comparison with
-  // num_encoded_frames_).
+  // Ids assigned to VideoFrames.
   std::set<int32> inputs_at_client_;
   int32 next_input_id_;
 
+  typedef std::map<int32, base::TimeTicks> IdToTicks;
+  // A map from input frame id to the encode start time of the frame.
+  IdToTicks encode_start_time_per_frame_;
+  // The encode latencies of all encoded frames.
+  std::vector<base::TimeDelta> encode_latencies_;
+
   // Ids for output BitstreamBuffers.
   typedef std::map<int32, base::SharedMemory*> IdToSHM;
   ScopedVector<base::SharedMemory> output_shms_;
@@ -915,6 +928,24 @@ double VEAClient::frames_per_second() {
   return num_encoded_frames_ / duration.InSecondsF();
 }
 
+std::vector<base::TimeDelta> VEAClient::encode_latency(
+    const std::vector<float>& ratios) {
+  std::sort(encode_latencies_.begin(), encode_latencies_.end());
+  size_t size = encode_latencies_.size();
+  std::vector<base::TimeDelta> latencies;
+  for (auto ratio : ratios) {
+    if (size == 0) {
+      latencies.push_back(base::TimeDelta());
+    } else {
+      CHECK_GE(ratio, 0.0f);
+      CHECK_LE(ratio, 1.0f);
+      size_t index = base::checked_cast<size_t>(ratio * (size - 1));

[Owen Lin, 2015/05/04 09:37:12]

Can we either implement the Nearest Rank method or the Linear Interpolation
Between Closest Ranks method ?

http://en.wikipedia.org/wiki/Percentile

[Justin Chuang, 2015/05/04 10:25:09]

Yeah... I thought about this. But I think the real value of outlier is more
useful than the interpolated value on the dashboard. 

How do you think? I'll change to interpolation if you feel it's a better
measurement.

[Owen Lin, 2015/05/05 06:03:13]

First, it won't do extrapolation for outliers (for p near 0 or 1). The
percentile value is always between two real values.

But I am also fine with the nearest rank method. It seems the used formula
doesn't match. I thought it should be 

index = std::max(base::checked_cast<size_t>(ratio * size), size - 1)

[Justin Chuang, 2015/05/06 10:35:32]

Changed to your formula (but s/max/min/). Thanks!

+      latencies.push_back(encode_latencies_[index]);
+    }
+  }
+  return latencies;
+}
+
 void VEAClient::RequireBitstreamBuffers(unsigned int input_count,
                                         const gfx::Size& input_coded_size,
                                         size_t output_size) {
@@ -1038,7 +1069,8 @@ void VEAClient::InputNoLongerNeededCallback(int32 input_id) {
     FeedEncoderWithOneInput();
 }
 
-scoped_refptr<media::VideoFrame> VEAClient::PrepareInputFrame(off_t position) {
+std::pair<scoped_refptr<media::VideoFrame>, int32> VEAClient::PrepareInputFrame(
+    off_t position) {
   CHECK_LE(position + test_stream_->aligned_buffer_size,
            test_stream_->mapped_aligned_in_file.length());
 
@@ -1069,9 +1101,8 @@ scoped_refptr<media::VideoFrame> VEAClient::PrepareInputFrame(off_t position) {
                          next_input_id_)));
 
   CHECK(inputs_at_client_.insert(next_input_id_).second);
-  ++next_input_id_;
 
-  return frame;
+  return std::make_pair(frame, next_input_id_++);
 }
 
 void VEAClient::OnInputTimer() {
@@ -1105,11 +1136,12 @@ void VEAClient::FeedEncoderWithOneInput() {
     ++num_keyframes_requested_;
   }
 
-  scoped_refptr<media::VideoFrame> video_frame =
-      PrepareInputFrame(pos_in_input_stream_);
+  auto ret = PrepareInputFrame(pos_in_input_stream_);

[Owen Lin, 2015/05/04 09:37:12]

we can get the input id by:

int32 input_id = next_input_id_;

[Justin Chuang, 2015/05/04 10:25:09]

I don't like this. See above.

[Justin Chuang, 2015/05/06 10:35:32]

Changed the prototype and remove reference to next_input_id_ in caller. Thx

   pos_in_input_stream_ += test_stream_->aligned_buffer_size;
 
-  encoder_->Encode(video_frame, force_keyframe);
+  CHECK(encode_start_time_per_frame_.insert(
+      std::make_pair(ret.second, base::TimeTicks::Now())).second);
+  encoder_->Encode(ret.first, force_keyframe);
 }
 
 void VEAClient::FeedEncoderWithOutput(base::SharedMemory* shm) {
@@ -1134,11 +1166,18 @@ bool VEAClient::HandleEncodedFrame(bool keyframe) {
   // return value from this method.
   CHECK_LE(num_encoded_frames_, num_frames_to_encode_);
 
+  last_frame_ready_time_ = base::TimeTicks::Now();
+
+  // Record encode latency of this frame.
+  IdToTicks::iterator it =
+      encode_start_time_per_frame_.find(num_encoded_frames_);
+  CHECK_NE(it, encode_start_time_per_frame_.end());
+  encode_latencies_.push_back(last_frame_ready_time_ - it->second);
+  encode_start_time_per_frame_.erase(it);
+
   ++num_encoded_frames_;
   ++num_frames_since_last_check_;
 
-  last_frame_ready_time_ = base::TimeTicks::Now();
-
   // Because the keyframe behavior requirements are loose, we give
   // the encoder more freedom here. It could either deliver a keyframe
   // immediately after we requested it, which could be for a frame number
@@ -1189,6 +1228,21 @@ void VEAClient::VerifyPerf() {
                    base::StringPrintf("%.3f", measured_fps));
   if (test_perf_)
     EXPECT_GE(measured_fps, kMinPerfFPS);
+
+  std::vector<float> ratios;

[Owen Lin, 2015/05/04 09:37:12]

Define them as constant, e.g. 

static float kLoggedLatencyPercentages[] = {0.50, 0.75, 0.95};

[Justin Chuang, 2015/05/04 10:25:09]

Will do. 

[Justin Chuang, 2015/05/06 10:35:32]

Done.

+  ratios.push_back(0.50f);
+  ratios.push_back(0.75f);
+  ratios.push_back(0.95f);
+  auto latencies = encode_latency(ratios);
+  for (size_t i = 0; i < ratios.size(); i++) {
+    std::string ratio_str =
+        base::StringPrintf("%d%%", static_cast<int>(ratios[i] * 100));
+    LOG(INFO) << "Encode latency at " << ratio_str << ": "

[Owen Lin, 2015/05/04 09:37:12]

Why using StringPrintf? The following code won't work?

LOG(INFO) << "Encode latency at "
          << static_cast<int>(ratios[i] * 100) << "%: "
          .....
          

[Justin Chuang, 2015/05/04 10:25:09]

This string used in two places.

[Owen Lin, 2015/05/05 06:03:13]

OK, it's fine. But I think you just need the percentage.

int percentage = static_cast<int>(ratios[i] * 100 + .5);
Then you can use it at both places.

[Justin Chuang, 2015/05/06 10:35:32]

Done. Thanks. Nice tip. +0.5 is good, I added it, although normally the FP
library won't have such issue. 

+              << latencies[i].InMicroseconds() << " us";
+    g_env->LogToFile(
+        base::StringPrintf("Encode latency at %s", ratio_str.c_str()),
+        base::StringPrintf("%" PRId64 " us", latencies[i].InMicroseconds()));
+  }
 }
 
 void VEAClient::VerifyStreamProperties() {