Request keyframe if too many packets are missing and NACK is disabled.

webrtc/modules/video_coding/main/source/jitter_buffer.cc

Index: webrtc/modules/video_coding/main/source/jitter_buffer.cc
diff --git a/webrtc/modules/video_coding/main/source/jitter_buffer.cc b/webrtc/modules/video_coding/main/source/jitter_buffer.cc
index 49c2325d80a852971edb1dff629b9c5a997e27e7..152d4bcecc891f3d173f73cfd9126fc277b35d0a 100644
--- a/webrtc/modules/video_coding/main/source/jitter_buffer.cc
+++ b/webrtc/modules/video_coding/main/source/jitter_buffer.cc
@@ -33,6 +33,10 @@ namespace webrtc {
 // Use this rtt if no value has been reported.
 static const int64_t kDefaultRtt = 200;
 
+// Request a keyframe if no continuous frame has been received for this
+// number of milliseconds and NACKs are disabled.
+static const int64_t kMaxDiscontinuousFramesTime = 10000;

[joachim, 2015/09/03 00:05:01]

I based the timeout on the behaviour of
"EndToEndTest.ReceivesPliAndRecoversWithNack" which requests a keyframe after
about 10 seconds.

[stefan-webrtc, 2015/09/18 14:22:41]

Acknowledged.

+
 typedef std::pair<uint32_t, VCMFrameBuffer*> FrameListPair;
 
 bool IsKeyFrame(FrameListPair pair) {
@@ -137,6 +141,7 @@ VCMJitterBuffer::VCMJitterBuffer(Clock* clock,
       num_packets_(0),
       num_duplicated_packets_(0),
       num_discarded_packets_(0),
+      time_last_decodable_frame_(-1),
       time_first_packet_ms_(0),
       jitter_estimate_(clock),
       inter_frame_delay_(clock_->TimeInMilliseconds()),
@@ -213,6 +218,7 @@ void VCMJitterBuffer::Start() {
   num_packets_ = 0;
   num_duplicated_packets_ = 0;
   num_discarded_packets_ = 0;
+  time_last_decodable_frame_ = -1;
   time_first_packet_ms_ = 0;
 
   // Start in a non-signaled state.
@@ -264,6 +270,7 @@ void VCMJitterBuffer::Flush() {
   last_decoded_state_.Reset();  // TODO(mikhal): sync reset.
   last_gof_valid_ = false;
   num_consecutive_old_packets_ = 0;
+  time_last_decodable_frame_ = -1;
   // Also reset the jitter and delay estimates
   jitter_estimate_.Reset();
   inter_frame_delay_.Reset(clock_->TimeInMilliseconds());
@@ -693,6 +700,17 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet,
 
   // Is the frame already in the decodable list?
   bool continuous = IsContinuous(*frame);
+  bool continuous_for_decoding;
+  if (continuous &&
+      (decode_error_mode_ != kWithErrors || nack_mode_ != kNoNack)) {

[stefan-webrtc, 2015/09/18 14:22:41]

Can you comment on this if-statement? Especially I don't understand why we are
checking decode_error_mode_ or nack_mode_.

[joachim, 2015/09/18 20:23:47]

If "IsContinuous" returns true and decode_error_mode_ is not kWithErrors, the
frame is always continuous for decoding (already checked in "IsContinuous").

Also if nack_mode_ is enabled, NACKs will be used to report missing data to the
sender and the value of "continuous_for_decoding" is not used below. Added
comment about that.

+    continuous_for_decoding = true;
+    time_last_decodable_frame_ = now_ms;
+  } else {
+    continuous_for_decoding = IsContinuousForDecoding(*frame);
+    if (continuous_for_decoding) {
+      time_last_decodable_frame_ = now_ms;
+    }
+  }
   switch (buffer_state) {
     case kGeneralError:
     case kTimeStampError:
@@ -720,6 +738,16 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet,
       } else {
         incomplete_frames_.InsertFrame(frame);
       }
+      if (nack_mode_ == kNoNack && continuous && !continuous_for_decoding) {

[stefan-webrtc, 2015/09/18 14:22:42]

Why do you want to do this only for kDecodableSession? Is it not worth doing for
kIncomplete too?

[joachim, 2015/09/18 20:23:47]

Right, added there too.

+        // If we didn't receive a continuous decodable frame for too long and
+        // can't report through NACKs to the sender, request a keyframe.
+        if (time_last_decodable_frame_ >= 0 &&
+            now_ms - time_last_decodable_frame_ > kMaxDiscontinuousFramesTime) {
+          // Update to current time so we don't request keyframes too often.
+          time_last_decodable_frame_ = now_ms;
+          return kFlushIndicator;
+        }
+      }
       break;
     }
     case kIncomplete: {
@@ -754,8 +782,14 @@ VCMFrameBufferEnum VCMJitterBuffer::InsertPacket(const VCMPacket& packet,
 
 bool VCMJitterBuffer::IsContinuousInState(const VCMFrameBuffer& frame,

[stefan-webrtc, 2015/09/18 14:22:42]

Should this method have a different name? I'd say it rather checks if a frame is
considered decodable or not? Doesn't have to be Continuous

[joachim, 2015/09/18 20:23:47]

As the functionality of "IsContinuousInState" is the same as before, I didn't
want to change the name.

     const VCMDecodingState& decoding_state) const {
-  if (decode_error_mode_ == kWithErrors)
+  if (decode_error_mode_ == kWithErrors) {
     return true;
+  }
+  return IsContinuousInStateForDecoding(frame, decoding_state);

[stefan-webrtc, 2015/09/18 14:22:41]

return decode_error_mode_ == kWithErrors || IsContinuous...();

[joachim, 2015/09/18 20:23:47]

Refactored, so that doesn't apply any more.

+}
+
+bool VCMJitterBuffer::IsContinuousInStateForDecoding(
+    const VCMFrameBuffer& frame, const VCMDecodingState& decoding_state) const {
   // Is this frame (complete or decodable) and continuous?
   // kStateDecodable will never be set when decode_error_mode_ is false
   // as SessionInfo determines this state based on the error mode (and frame
@@ -785,6 +819,27 @@ bool VCMJitterBuffer::IsContinuous(const VCMFrameBuffer& frame) const {
   return false;
 }
 
+bool VCMJitterBuffer::IsContinuousForDecoding(

[stefan-webrtc, 2015/09/18 14:22:41]

I don't like how this duplicates the code of IsContinuous()

[joachim, 2015/09/18 20:23:47]

Updated to take a parameter for the two modes.

+    const VCMFrameBuffer& frame) const {
+  if (IsContinuousInStateForDecoding(frame, last_decoded_state_)) {
+    return true;
+  }
+  VCMDecodingState decoding_state;
+  decoding_state.CopyFrom(last_decoded_state_);
+  for (FrameList::const_iterator it = decodable_frames_.begin();
+       it != decodable_frames_.end(); ++it)  {
+    VCMFrameBuffer* decodable_frame = it->second;
+    if (IsNewerTimestamp(decodable_frame->TimeStamp(), frame.TimeStamp())) {
+      break;
+    }
+    decoding_state.SetState(decodable_frame);
+    if (IsContinuousInStateForDecoding(frame, decoding_state)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void VCMJitterBuffer::FindAndInsertContinuousFrames(
     const VCMFrameBuffer& new_frame) {
   VCMDecodingState decoding_state;