Experiment for the nack module.

webrtc/modules/video_coding/jitter_buffer.cc

Index: webrtc/modules/video_coding/jitter_buffer.cc
diff --git a/webrtc/modules/video_coding/jitter_buffer.cc b/webrtc/modules/video_coding/jitter_buffer.cc
index ca8c0d957148233f56ca07debc8dbf8aad155bb8..d38a5cfeca75fc01eb0f5a35bbddbca78d3bf4dc 100644
--- a/webrtc/modules/video_coding/jitter_buffer.cc
+++ b/webrtc/modules/video_coding/jitter_buffer.cc
@@ -28,10 +28,10 @@
 #include "webrtc/system_wrappers/include/clock.h"
 #include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 #include "webrtc/system_wrappers/include/event_wrapper.h"
+#include "webrtc/system_wrappers/include/field_trial.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
-
 // Interval for updating SS data.
 static const uint32_t kSsCleanupIntervalSec = 60;
 
@@ -215,7 +215,10 @@ void Vp9SsMap::UpdateFrames(FrameList* frames) {
 }
 
 VCMJitterBuffer::VCMJitterBuffer(Clock* clock,
-                                 std::unique_ptr<EventWrapper> event)
+                                 std::unique_ptr<EventWrapper> event,
+                                 ProcessThread* module_process_thread,
+                                 NackSender* nack_sender,
+                                 KeyFrameRequestSender* keyframe_request_sender)
     : clock_(clock),
       running_(false),
       crit_sect_(CriticalSectionWrapper::CreateCriticalSection()),
@@ -249,7 +252,17 @@ VCMJitterBuffer::VCMJitterBuffer(Clock* clock,
       max_incomplete_time_ms_(0),
       decode_error_mode_(kNoErrors),
       average_packets_per_frame_(0.0f),
-      frame_counter_(0) {
+      frame_counter_(0),
+      module_process_thread_(module_process_thread) {
+  if (field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled") {
+    RTC_DCHECK(module_process_thread);
+    RTC_DCHECK(nack_sender);
+    RTC_DCHECK(keyframe_request_sender);
+
+    nack_module_.reset(
+        new NackModule(clock, nack_sender, keyframe_request_sender));

[stefan-webrtc, 2016/03/10 14:20:40]

Set in initializer list instead so that you can make nack_module_ const.

[philipel, 2016/03/10 16:39:27]

Can't do that since we have to check first if we are running the experiment.

[stefan-webrtc, 2016/03/10 17:06:29]

You can still do it:

nack_module_(IsInExperiment() ? new NackModule() : nullptr),

I think that's preferable

[philipel, 2016/03/11 09:24:14]

Ok, it is possible, but don't you think it would be really ugly to include this
block of code in the initialization list?

[stefan-webrtc, 2016/03/11 09:57:11]

I think that's fine, just break it out to a static helper function. It makes it
a lot more clear that the nack module doesn't have to be protected by critical
sections.

[philipel, 2016/03/11 10:12:59]

Done.

+    module_process_thread_->RegisterModule(nack_module_.get());
+  }
   for (int i = 0; i < kStartNumberOfFrames; i++)
     free_frames_.push_back(new VCMFrameBuffer());
 }
@@ -268,6 +281,8 @@ VCMJitterBuffer::~VCMJitterBuffer() {
        it != decodable_frames_.end(); ++it) {
     delete it->second;
   }
+  if (field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")

[stefan-webrtc, 2016/03/10 14:20:40]

if (nack_module_) instead, here and all places below.

[philipel, 2016/03/10 16:39:27]

Done.

+    module_process_thread_->DeRegisterModule(nack_module_.get());
   delete crit_sect_;
 }
 
@@ -329,6 +344,8 @@ void VCMJitterBuffer::Stop() {
   UpdateHistograms();
   running_ = false;
   last_decoded_state_.Reset();
+  if (field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")
+    nack_module_->Stop();
 
   // Make sure all frames are free and reset.
   for (FrameList::iterator it = decodable_frames_.begin();
@@ -665,6 +682,9 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet,
                                                  bool* retransmitted) {
   CriticalSectionScoped cs(crit_sect_);
 
+  if (field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")
+    nack_module_->OnReceivedPacket(packet);
+
   ++num_packets_;
   if (num_packets_ == 1) {
     time_first_packet_ms_ = clock_->TimeInMilliseconds();
@@ -927,6 +947,8 @@ void VCMJitterBuffer::UpdateRtt(int64_t rtt_ms) {
   CriticalSectionScoped cs(crit_sect_);
   rtt_ms_ = rtt_ms;
   jitter_estimate_.UpdateRtt(rtt_ms);
+  if (field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")
+    nack_module_->UpdateRtt(rtt_ms);
 }
 
 void VCMJitterBuffer::SetNackMode(VCMNackMode mode,
@@ -1046,6 +1068,10 @@ std::vector<uint16_t> VCMJitterBuffer::GetNackList(bool* request_key_frame) {
       }
     }
   }

[stefan-webrtc, 2016/03/10 14:20:40]

Should we really run lines 1017-1070 if using the nack module?

[philipel, 2016/03/10 16:39:27]

Yes, the GetNackList interface does more than just return the nack list, it also
is a way for the jitter buffer to signal that it wants to request a keyframe.

[stefan-webrtc, 2016/03/10 17:06:29]

Ok, comment on why we don't return earlier to avoid future problems. And maybe
add a TODO to remove the GetNackList() method entirely. Btw, where is the code
above going to move when that is done?

[philipel, 2016/03/11 09:24:14]

Expanded my comment.

This code won't be necessary as the jitter buffer will have a
KeyframeRequestSender available, so whenever the jitter buffer wants to request
a keyframe according to some rule, then it can simply request the keyframe
immediately and not having to set a variable that will be picked up later by
some other modules Process function.

+  // The experiment is running, the nack module will send Nacks.
+  if (field_trial::FindFullName("WebRTC-NewVideoJitterBuffer") == "Enabled")
+    return std::vector<uint16_t>();
+
   std::vector<uint16_t> nack_list(missing_sequence_numbers_.begin(),
                                   missing_sequence_numbers_.end());
   return nack_list;