Wire up VCMTiming in FrameBuffer2.

webrtc/modules/video_coding/frame_buffer2.cc

Index: webrtc/modules/video_coding/frame_buffer2.cc
diff --git a/webrtc/modules/video_coding/frame_buffer2.cc b/webrtc/modules/video_coding/frame_buffer2.cc
index 53b30c9240558aa763aad19411ebf0af782c8384..973a76c350804f91937427131e2c5736d61d836f 100644
--- a/webrtc/modules/video_coding/frame_buffer2.cc
+++ b/webrtc/modules/video_coding/frame_buffer2.cc
@@ -42,11 +42,12 @@ bool FrameBuffer::FrameComp::operator()(const FrameKey& f1,
 
 FrameBuffer::FrameBuffer(Clock* clock,
                          VCMJitterEstimator* jitter_estimator,
-                         const VCMTiming* timing)
+                         VCMTiming* timing)
     : clock_(clock),
       frame_inserted_event_(false, false),
       jitter_estimator_(jitter_estimator),
       timing_(timing),
+      inter_frame_delay_(clock_->TimeInMilliseconds()),
       newest_picture_id_(-1),
       stopped_(false) {}
 
@@ -56,7 +57,7 @@ std::unique_ptr<FrameObject> FrameBuffer::NextFrame(int64_t max_wait_time_ms) {
   int64_t wait_ms = max_wait_time_ms;
   while (true) {
     std::map<FrameKey, std::unique_ptr<FrameObject>, FrameComp>::iterator
-        next_frame;
+        next_frame_it;
     {
       rtc::CritScope lock(&crit_);
       frame_inserted_event_.Reset();
@@ -65,14 +66,18 @@ std::unique_ptr<FrameObject> FrameBuffer::NextFrame(int64_t max_wait_time_ms) {
 
       now = clock_->TimeInMilliseconds();
       wait_ms = max_wait_time_ms;
-      next_frame = frames_.end();
+      next_frame_it = frames_.end();
       for (auto frame_it = frames_.begin(); frame_it != frames_.end();
            ++frame_it) {
         const FrameObject& frame = *frame_it->second;
         if (IsContinuous(frame)) {
-          next_frame = frame_it;
-          int64_t render_time = timing_->RenderTimeMs(frame.timestamp, now);
+          next_frame_it = frame_it;
+          int64_t render_time =
+              next_frame_it->second->RenderTime() == -1
+                  ? timing_->RenderTimeMs(frame.timestamp, now)
+                  : next_frame_it->second->RenderTime();
           wait_ms = timing_->MaxWaitingTime(render_time, now);
+          frame_it->second->SetRenderTime(render_time);
 
           // This will cause the frame buffer to prefer high framerate rather
           // than high resolution in the case of the decoder not decoding fast
@@ -85,19 +90,29 @@ std::unique_ptr<FrameObject> FrameBuffer::NextFrame(int64_t max_wait_time_ms) {
       }
     }
 
-    // If the timout occures, return. Otherwise a new frame has been inserted
-    // and the best frame to decode next will be selected again.
     wait_ms = std::min<int64_t>(wait_ms, latest_return_time - now);
     wait_ms = std::max<int64_t>(wait_ms, 0);
+    // If the timout occures, return. Otherwise a new frame has been inserted

[stefan-webrtc, 2016/07/12 16:55:38]

If the timeout occurs

[philipel, 2016/07/13 09:21:49]

Done.

+    // and the best frame to decode next will be selected again.
     if (!frame_inserted_event_.Wait(wait_ms)) {
       rtc::CritScope lock(&crit_);
-      if (next_frame != frames_.end()) {
-        // TODO(philipel): update jitter estimator with correct values.
-        jitter_estimator_->UpdateEstimate(100, 100);
+      if (next_frame_it != frames_.end()) {
+        int64_t received_timestamp = next_frame_it->second->ReceivedTime();
+        uint32_t timestamp = next_frame_it->second->Timestamp();
+
+        int64_t frame_delay;
+        if (inter_frame_delay_.CalculateDelay(timestamp, &frame_delay,
+                                              received_timestamp)) {
+          jitter_estimator_->UpdateEstimate(frame_delay,
+                                            next_frame_it->second->size);
+        }
+        timing_->SetJitterDelay(jitter_estimator_->GetJitterEstimate(1.0));
+        timing_->UpdateCurrentDelay(next_frame_it->second->RenderTime(),
+                                    clock_->TimeInMilliseconds());
 
-        decoded_frames_.insert(next_frame->first);
-        std::unique_ptr<FrameObject> frame = std::move(next_frame->second);
-        frames_.erase(frames_.begin(), ++next_frame);
+        decoded_frames_.insert(next_frame_it->first);
+        std::unique_ptr<FrameObject> frame = std::move(next_frame_it->second);
+        frames_.erase(frames_.begin(), ++next_frame_it);
         return frame;

[stefan-webrtc, 2016/07/12 16:55:39]

Should we update the render time of this frame before we return it? The render
time could be stale if we have been waiting.

[philipel, 2016/07/13 09:21:49]

It could? If that's the case then it's going to be very hard to know for how
long we should wait before returning...

[stefan-webrtc, 2016/07/14 12:17:30]

It can since the inter arrival class might have updated, or the decode time may
have changed.

Check this code for an idea of how it can be done:
https://cs.corp.google.com/piper///depot/google3/third_party/webrtc/files/sta...

I think we're handling it there?

[philipel, 2016/07/14 15:39:39]

Yes and no. That part of the code only waits for as long as possible before
returning a NULL frame in order to avoid a busy loop.

Since the max wait time is 50 ms (defined here
https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_receive_s...)
it is possible that it returns a NULL frame instead of the next frame and the
next time this function is called the rendertime is updated.

It is also possible that the wait time is shorter than the 50 ms, in which case
it will wait for |wait_time_ms| and then still use the render time that was
calculated before the wait occurred.

So if the wait is longer than 50 ms then the render time will be updated,
otherwise it will not.

I guess this is a coarse way to continuously update the render time, but I
wonder if it is necessary or if we gain anything by doing so. 

       } else {
         return std::unique_ptr<FrameObject>();
@@ -119,8 +134,11 @@ void FrameBuffer::Stop() {
 
 void FrameBuffer::InsertFrame(std::unique_ptr<FrameObject> frame) {
   rtc::CritScope lock(&crit_);
-  if (newest_picture_id_ == -1)
+  // If |newest_picture_id_ is -1 then this is the first frame we received.

[stefan-webrtc, 2016/07/12 16:55:38]

|newest_picture_id_|

[philipel, 2016/07/13 09:21:49]

Done.

+  if (newest_picture_id_ == -1) {
     newest_picture_id_ = frame->picture_id;
+    inter_frame_delay_.Reset(clock_->TimeInMilliseconds());
+  }
 
   if (AheadOf<uint16_t>(frame->picture_id, newest_picture_id_))
     newest_picture_id_ = frame->picture_id;
@@ -129,7 +147,7 @@ void FrameBuffer::InsertFrame(std::unique_ptr<FrameObject> frame) {
   while (decoded_frames_.size() > kMaxNumHistoryFrames)
     decoded_frames_.erase(decoded_frames_.begin());
 
-  // Remove frames that are too old, |kMaxNumHistoryFrames|.
+  // Remove frames that are too old.
   uint16_t old_picture_id = Subtract<1 << 16>(newest_picture_id_, kMaxFrameAge);
   auto old_decoded_it =
       decoded_frames_.lower_bound(FrameKey(old_picture_id, 0));