New jitter buffer experiment.

webrtc/video/video_receive_stream.cc

Index: webrtc/video/video_receive_stream.cc
diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc
index 5173b2d4796aa5e8e974bfef5046001f671f77ff..44c9011da6c30c9d1b8bbd7ecd7d347ab4ff0ef7 100644
--- a/webrtc/video/video_receive_stream.cc
+++ b/webrtc/video/video_receive_stream.cc
@@ -18,16 +18,21 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/base/optional.h"
 #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h"
 #include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/utility/include/process_thread.h"
 #include "webrtc/modules/video_coding/include/video_coding.h"
+#include "webrtc/modules/video_coding/jitter_estimator.h"
+#include "webrtc/modules/video_coding/timing.h"
 #include "webrtc/modules/video_coding/utility/ivf_file_writer.h"
 #include "webrtc/system_wrappers/include/clock.h"
+#include "webrtc/system_wrappers/include/field_trial.h"
 #include "webrtc/video/call_stats.h"
 #include "webrtc/video/receive_statistics_proxy.h"
 #include "webrtc/video_receive_stream.h"
 #include "webrtc/voice_engine/include/voe_video_sync.h"
+#include "webrtc/modules/video_coding/frame_object.h"

[stefan-webrtc, 2016/11/08 10:41:35]

Sort

[philipel, 2016/11/08 12:28:39]

Done.

 
 namespace webrtc {
 
@@ -197,6 +202,7 @@ VideoReceiveStream::VideoReceiveStream(
       call_stats_(call_stats),
       video_receiver_(clock_, nullptr, this, this, this),
       stats_proxy_(&config_, clock_),
+      timing_(new VCMTiming(clock_)),
       rtp_stream_receiver_(
           &video_receiver_,
           congestion_controller_->GetRemoteBitrateEstimator(
@@ -209,7 +215,11 @@ VideoReceiveStream::VideoReceiveStream(
           &config_,
           &stats_proxy_,
           process_thread_,
-          congestion_controller_->GetRetransmissionRateLimiter()),
+          congestion_controller_->GetRetransmissionRateLimiter(),
+          this,  // NackSender
+          this,  // KeyFrameRequestSender
+          this,  // OnCompleteFrameCallback

[stefan-webrtc, 2016/11/08 10:41:35]

Should we consolidate these interfaces so that we only have to register 'this'
once? Or do they make sense to have separate?

[philipel, 2016/11/08 12:28:39]

To me it doesn't make much sense to have them in one interface as that interface
would be extremely specific to just initialize the RtpStreamReceiver. I think
this is more modular/clear.

[stefan-webrtc, 2016/11/08 14:49:27]

Acknowledged.

+          timing_.get()),
       rtp_stream_sync_(&video_receiver_, &rtp_stream_receiver_) {
   LOG(LS_INFO) << "VideoReceiveStream: " << config_.ToString();
 
@@ -230,6 +240,15 @@ VideoReceiveStream::VideoReceiveStream(
 
   video_receiver_.SetRenderDelay(config.render_delay_ms);
 
+  jitter_buffer_experiment_ =
+      field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled";
+
+  if (jitter_buffer_experiment_) {
+    jitter_estimator_.reset(new VCMJitterEstimator(clock_));
+    frame_buffer_.reset(new video_coding::FrameBuffer(
+        clock_, jitter_estimator_.get(), timing_.get()));
+  }
+
   process_thread_->RegisterModule(&video_receiver_);
   process_thread_->RegisterModule(&rtp_stream_sync_);
 }
@@ -268,6 +287,15 @@ bool VideoReceiveStream::OnRecoveredPacket(const uint8_t* packet,
 void VideoReceiveStream::Start() {
   if (decode_thread_.IsRunning())
     return;
+  if (jitter_buffer_experiment_) {
+    frame_buffer_->Start();
+    call_stats_->RegisterStatsObserver(&rtp_stream_receiver_);
+
+    if (rtp_stream_receiver_.IsRetransmissionsEnabled() &&
+        rtp_stream_receiver_.IsFecEnabled()) {
+      frame_buffer_->SetProtectionMode(kProtectionNackFEC);

[stefan-webrtc, 2016/11/08 10:41:35]

No need to set kProtectionNack if fec is disabled?

[philipel, 2016/11/08 12:28:39]

The FrameBuffer only change its behavior if the protection mode is
kProtectionNackFEC.

[stefan-webrtc, 2016/11/08 14:49:27]

Acknowledged.

+    }
+  }
   transport_adapter_.Enable();
   rtc::VideoSinkInterface<VideoFrame>* renderer = nullptr;
   if (config_.renderer) {
@@ -310,6 +338,12 @@ void VideoReceiveStream::Stop() {
   // stop immediately, instead of waiting for a timeout. Needs to be called
   // before joining the decoder thread thread.
   video_receiver_.TriggerDecoderShutdown();
+
+  if (jitter_buffer_experiment_) {
+    frame_buffer_->Stop();
+    call_stats_->DeregisterStatsObserver(&rtp_stream_receiver_);
+  }
+
   if (decode_thread_.IsRunning()) {
     decode_thread_.Stop();
     // Deregister external decoders so they are no longer running during
@@ -319,6 +353,7 @@ void VideoReceiveStream::Stop() {
     for (const Decoder& decoder : config_.decoders)
       video_receiver_.RegisterExternalDecoder(nullptr, decoder.payload_type);
   }
+
   call_stats_->DeregisterStatsObserver(video_stream_decoder_.get());
   video_stream_decoder_.reset();
   incoming_video_stream_.reset();
@@ -365,6 +400,13 @@ void VideoReceiveStream::OnFrame(const VideoFrame& video_frame) {
   stats_proxy_.OnRenderedFrame(video_frame);
 }
 
+void VideoReceiveStream::OnCompleteFrame(
+    std::unique_ptr<video_coding::FrameObject> frame) {
+  int last_continuous_pid = frame_buffer_->InsertFrame(std::move(frame));
+  if (last_continuous_pid != -1)

[nisse-webrtc, 2016/11/08 11:56:30]

When using magic values like this, you have to make sure that the magic value
can't occur except for the exceptional cases it is intended for. What happens if
-1 is encoded on the wire (using rtp or quartc transport)? When typing the
picture idd as int, it's not obvious that negative values are invalid.

Other code where 0 or -1 was magic, has been converted to use rtc::Optional.

[philipel, 2016/11/08 12:28:39]

Good point, but I have a few other changes I would like to do as well and I
don't want to do that in this CL.

+    rtp_stream_receiver_.FrameContinuous(last_continuous_pid);
+}
+
 // TODO(asapersson): Consider moving callback from video_encoder.h or
 // creating a different callback.
 EncodedImageCallback::Result VideoReceiveStream::OnEncodedImage(
@@ -397,7 +439,26 @@ bool VideoReceiveStream::DecodeThreadFunction(void* ptr) {
 
 void VideoReceiveStream::Decode() {
   static const int kMaxDecodeWaitTimeMs = 50;
-  video_receiver_.Decode(kMaxDecodeWaitTimeMs);
+  if (jitter_buffer_experiment_) {
+    static const int kMaxWaitForFrameMs = 3000;
+    std::unique_ptr<video_coding::FrameObject> frame;
+    video_coding::FrameBuffer::ReturnReason res =
+        frame_buffer_->NextFrame(kMaxWaitForFrameMs, &frame);
+
+    if (res == video_coding::FrameBuffer::ReturnReason::kStopped)
+      return;
+
+    if (frame) {
+      if (video_receiver_.Decode(frame.get()) == VCM_OK)
+        rtp_stream_receiver_.FrameDecoded(frame->picture_id);
+    } else {
+      LOG(LS_WARNING) << "No decodable frame in " << kMaxWaitForFrameMs
+                      << " ms, requesting keyframe.";
+      RequestKeyFrame();
+    }
+  } else {
+    video_receiver_.Decode(kMaxDecodeWaitTimeMs);
+  }
 }
 
 void VideoReceiveStream::SendNack(