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

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

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 #include "webrtc/modules/video_coding/main/source/jitter_buffer.h"
@@ skipping 20 matching lines @@
 #include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
 
 // Interval for updating SS data.
 static const uint32_t kSsCleanupIntervalSec = 60;
 
 // 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;
+
 typedef std::pair<uint32_t, VCMFrameBuffer*> FrameListPair;
 
 bool IsKeyFrame(FrameListPair pair) {
   return pair.second->FrameType() == kVideoFrameKey;
 }
 
 bool HasNonEmptyState(FrameListPair pair) {
   return pair.second->GetState() != kStateEmpty;
 }
 
@@ skipping 469 matching lines @@
   if (!running_) {
     return false;
   }
   if (decode_error_mode_ == kNoErrors) {
     // No point to continue, as we are not decoding with errors.
     return false;
   }
 
   CleanUpOldOrEmptyFrames();
 
+  VCMFrameBuffer* oldest_frame;
   if (decodable_frames_.empty()) {
-    return false;
-  }
-  VCMFrameBuffer* oldest_frame = decodable_frames_.Front();
-  // If we have exactly one frame in the buffer, release it only if it is
-  // complete. We know decodable_frames_ is  not empty due to the previous
-  // check.
-  if (decodable_frames_.size() == 1 && incomplete_frames_.empty()
-      && oldest_frame->GetState() != kStateComplete) {
-    return false;
+    if (nack_mode_ != kNoNack || incomplete_frames_.empty()) {
+      return false;
+    }
+    oldest_frame = incomplete_frames_.Front();
+    RTC_DCHECK(oldest_frame->GetState() != kStateComplete);
+    // Last frame will only be removed from buffer if it is complete.
+    if (incomplete_frames_.size() == 1 ||
+        oldest_frame->GetState() != kStateDecodable) {
+      return false;
+    }
+  } else {
+    oldest_frame = decodable_frames_.Front();
+    // If we have exactly one frame in the buffer, release it only if it is
+    // complete. We know decodable_frames_ is  not empty due to the previous
+    // check.
+    if (decodable_frames_.size() == 1 && incomplete_frames_.empty()
+        && oldest_frame->GetState() != kStateComplete) {

[stefan-webrtc, 2015/11/16 17:52:15]

Can we end up here without oldest_frame being complete?

[joachim, 2015/11/16 21:58:11]

Well, the condition was like that before, I just moved it. Removing the state
comparison causes "TestJitterBufferNack.NormalOperation" to fail.

[stefan-webrtc, 2015/11/18 00:02:17]

Acknowledged.

+      return false;
+    }
   }
 
   *timestamp = oldest_frame->TimeStamp();
   return true;
 }
 
 VCMEncodedFrame* VCMJitterBuffer::ExtractAndSetDecode(uint32_t timestamp) {
   CriticalSectionScoped cs(crit_sect_);
   if (!running_) {
     return NULL;
@@ skipping 200 matching lines @@
           packet.frameType != kVideoFrameKey) {
         buffer_state = kFlushIndicator;
       }
 
       latest_received_sequence_number_ = LatestSequenceNumber(
           latest_received_sequence_number_, packet.seqNum);
     }
   }
 
   // Is the frame already in the decodable list?
-  bool continuous = IsContinuous(*frame);
+  bool continuous = IsContinuous(*frame, false);
+  bool continuous_for_decoding;
+  if (continuous &&
+      (decode_error_mode_ != kWithErrors || nack_mode_ != kNoNack)) {
+    // If NACKs are enabled missing data will be handled through them so it's
+    // fine to always assume frames that are continuous for decoding below.
+    continuous_for_decoding = true;

[stefan-webrtc, 2015/11/16 17:52:15]

Does this make sure that we actually wait for the packets to be
nacked/retransmitted before we decode the frame though? Or can this cause us to
decode the frame before the retransmissions have arrived?

[joachim, 2015/11/16 21:58:11]

I don't think so as the behavior for enabled NACKs was not modified with this
change. Frames with "IsContinuous = true" were also added to decodable_frames_
before.

+  } else {
+    continuous_for_decoding = IsContinuous(*frame, true);

[stefan-webrtc, 2015/11/16 17:52:15]

Does this mean we require frames to be complete also for decode_error_mode_ ==
kWithErrors? It's not entirely clear to me in what situations we get in to this
"else", and why it should ignore the error mode. Do we even need an error mode
check in IsContinuous?

[joachim, 2015/11/16 21:58:11]

For "decode_error_mode_ == kWithErrors", the actual state of the frame is not
checked ("IsContinuous" immediately returns "true" unless the error mode is
ignored). That's why I added the second mode to "IsContinuous" that always
checks the state of the frame (stored in "continuous_for_decoding_").

The "else" path is taken when NACKs are disabled and/or the decode_error_mode_
is "kWithErrors" because in these cases either missing packets won't be reported
(NACKs disabled) or "IsContinuous" didn't already check the state of the frame
(decode_error_mode_ == kWithErrors).

[stefan-webrtc, 2015/11/18 00:02:17]

I'm trying to understand when and why it is necessary to call IsContinuous with
ignore_error_mode = false. Do we need to go down that code path?
So continuous_for_decoding means that the frame is complete (all packets
received) and in sequence (i.e., continuous) if we have nack and not decoding
with errors. And if we are decoding with errors it means the frame is in
sequence (continuous) but not necessarily that it is complete?

Do we need to check both continuous and continuous_for_decoding at lines 806 and
812, or is continuous_for_decoding enough?

[joachim, 2015/11/18 00:35:14]

The problem is, that with ignore_error_mode = true the actual frame is never
checked if decoding with errors is allowed (which often is the case). This
however is solved by this CL, so that with disabled NACKs (and decoding with
errors allowed), the frame contents are still checked and if no continuous frame
for decoding are received for too long, a keyframe can be requested.
Line 806 can be changed (verified with the Jitter and EndToEnd tests). Thanks
for spotting this.

Changing line 812 causes this test to fail:
[ RUN      ] TestRunningJitterBuffer.Full
../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139:
Failure
Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= (kNoError),
actual: -3 vs 0
[  FAILED  ] TestRunningJitterBuffer.Full (2 ms)

This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of
frames are added and so a keyframe is requested. All other tests (Jitter and
EndToEnd) are running without error.

What do you think, should this still be changed and the test adjusted (how?), or
should I keep the condition as is right now?

[joachim, 2015/11/18 00:50:26]

Btw, I did some more testing and you are right: if I change both (always
checking with ignore_error_mode = true and also updating the conditions below),
all Jitter/EndToEnd tests but the "TestRunningJitterBuffer.Full" are running
without errors.

However in the (I think) common case (NACKs enabled and decoding with errors
allowed), this will always check the frame contents.

[stefan-webrtc, 2015/11/18 20:51:57]

Yes, but doesn't that mean that we don't decode with errors any longer? That may
be the right decision, but then maybe the code also shouldn't have a
decode_error_mode_ mode? I think it would be fine to only decode frames that are
in sequence and complete, as long as we make sure we request a key frame if we
have failed to decode for too long. That is what you are trying to do here,
right?

[stefan-webrtc, 2015/11/18 20:51:57]

I'd say the common case is NACKs enabled and decoding with errors not allowed.
Right, that seems like correct behavior. You may want to change the test to do
the DropFrame(1) at the end, so that the non-decodable frames time is shorter,
and then simply verify that it's getting full and causes a kFlushIndicator.

[joachim, 2015/11/18 23:46:07]

Acknowledged.
Sorry, I don't get that. If DropFrame(1) is moved after InsertFrames(kMax..),
the frames are not added to "incomplete_frames_", the new code doesn't trigger
and the frames are decodable afterwards (causing the rest of the test to fail).

[joachim, 2015/11/18 23:46:07]

Hmm. With the latest changes, decoding with errors indeed will be no longer
working. That functionality was removed with patch 5 and the change to reuse
existing code. Before I was still adding these frames to "decodable_frames_" and
manually measuring the time since the last continuous and decodable frame to
detect when to request keyframes.
Reusing "NonContinuousOrIncompleteDuration" required them to be stored in
"incomplete_frames_" so they don't get decoded any more.

I'm not sure if that's the way to go, now a keyframe will be requested either
through the existing NACK code or through the new code if no frame was decoded
for too long.

[stefan-webrtc, 2015/11/18 23:51:03]

Right. What behavior is it that you want? We typically don't use this mode and
would be OK with only decoding frames without error, but making sure we send
PLI/key frame requests often when there is packet loss.

If you however do want to decode with errors your previous version is probably
more correct... Sorry in that case if I led you down the wrong path.

If we decide not to decode with errors, I think we should clean that code up
too.

[stefan-webrtc, 2015/11/19 00:04:17]

I see, maybe if you increase the framerate (decrease the time between frames)
you can verify the behavior if the buffer gets full without triggering the new
functionality.

[stefan-webrtc, 2015/11/19 00:06:54]

OK, in that case I suggest that we clean up the "decode with errors = true"
pieces and remove the ability to decode with errors, and instead freeze the
video while waiting for the key frame. Maybe it would make sense to reduce the
kMaxDiscontinuousFramesTime when NACK isn't enabled too?

[joachim, 2015/11/19 00:44:50]

Another possibility would be to enable NACK for this test, so the new codepath
is not executed and the jitter can be filled without requesting keyframes.

+  }
   switch (buffer_state) {
     case kGeneralError:
     case kTimeStampError:
     case kSizeError: {
       free_frames_.push_back(frame);
       break;
     }
     case kCompleteSession: {
       if (previous_state != kStateDecodable &&
           previous_state != kStateComplete) {
         CountFrame(*frame);
         if (continuous) {
           // Signal that we have a complete session.
           frame_event_->Set();
         }
       }
       FALLTHROUGH();
     }
     // Note: There is no break here - continuing to kDecodableSession.
     case kDecodableSession: {
       *retransmitted = (frame->GetNackCount() > 0);
-      if (continuous) {
+      if (continuous && continuous_for_decoding) {
         decodable_frames_.InsertFrame(frame);
         FindAndInsertContinuousFrames(*frame);
       } else {
         incomplete_frames_.InsertFrame(frame);
+        // If NACKs are enabled, keyframes are triggered by |GetNackList|.
+        if (nack_mode_ == kNoNack && continuous && !continuous_for_decoding &&
+            NonContinuousOrIncompleteDuration() >
+            90 * kMaxDiscontinuousFramesTime) {
+          return kFlushIndicator;
+        }
       }
       break;
     }
     case kIncomplete: {
       if (frame->GetState() == kStateEmpty &&
           last_decoded_state_.UpdateEmptyFrame(frame)) {
         free_frames_.push_back(frame);
         return kNoError;
       } else {
         incomplete_frames_.InsertFrame(frame);
+        // If NACKs are enabled, keyframes are triggered by |GetNackList|.
+        if (nack_mode_ == kNoNack && NonContinuousOrIncompleteDuration() >
+            90 * kMaxDiscontinuousFramesTime) {
+          return kFlushIndicator;
+        }
       }
       break;
     }
     case kNoError:
     case kOutOfBoundsPacket:
     case kDuplicatePacket: {
       // Put back the frame where it came from.
       if (frame_list != NULL) {
         frame_list->InsertFrame(frame);
       } else {
         free_frames_.push_back(frame);
       }
       ++num_duplicated_packets_;
       break;
     }
     case kFlushIndicator:
       free_frames_.push_back(frame);
       return kFlushIndicator;
     default: assert(false);
   }
   return buffer_state;
 }
 
 bool VCMJitterBuffer::IsContinuousInState(const VCMFrameBuffer& frame,
-    const VCMDecodingState& decoding_state) const {
-  if (decode_error_mode_ == kWithErrors)
+    const VCMDecodingState& decoding_state, bool ignore_error_mode) const {
+  if (!ignore_error_mode && decode_error_mode_ == kWithErrors) {
     return true;
+  }
+
   // 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
   // completeness).
   return (frame.GetState() == kStateComplete ||
           frame.GetState() == kStateDecodable) &&
          decoding_state.ContinuousFrame(&frame);
 }
 
-bool VCMJitterBuffer::IsContinuous(const VCMFrameBuffer& frame) const {
-  if (IsContinuousInState(frame, last_decoded_state_)) {
+bool VCMJitterBuffer::IsContinuous(const VCMFrameBuffer& frame,
+    bool ignore_error_mode) const {
+  if (IsContinuousInState(frame, last_decoded_state_, ignore_error_mode)) {
     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 (IsContinuousInState(frame, decoding_state)) {
+    if (IsContinuousInState(frame, decoding_state, ignore_error_mode)) {
       return true;
     }
   }
   return false;
 }
 
 void VCMJitterBuffer::FindAndInsertContinuousFrames(
     const VCMFrameBuffer& new_frame) {
   VCMDecodingState decoding_state;
   decoding_state.CopyFrom(last_decoded_state_);
@@ skipping 13 matching lines @@
   // 1. Continuous base or sync layer.
   // 2. The end of the list was reached.
   for (FrameList::iterator it = incomplete_frames_.begin();
        it != incomplete_frames_.end();)  {
     VCMFrameBuffer* frame = it->second;
     if (IsNewerTimestamp(original_decoded_state.time_stamp(),
                          frame->TimeStamp())) {
       ++it;
       continue;
     }
-    if (IsContinuousInState(*frame, decoding_state)) {
+    if (IsContinuousInState(*frame, decoding_state, false)) {
       decodable_frames_.InsertFrame(frame);
       incomplete_frames_.erase(it++);
       decoding_state.SetState(frame);
     } else if (frame->TemporalId() <= 0) {
       break;
     } else {
       ++it;
     }
   }
 }
@@ skipping 427 matching lines @@
   }
   // Evaluate if the RTT is higher than |high_rtt_nack_threshold_ms_|, and in
   // that case we don't wait for retransmissions.
   if (high_rtt_nack_threshold_ms_ >= 0 &&
       rtt_ms_ >= high_rtt_nack_threshold_ms_) {
     return false;
   }
   return true;
 }
 }  // namespace webrtc