Only create H264 frames if there are no gaps in the packet sequence number.

webrtc/modules/video_coding/packet_buffer.cc

 /*
  *  Copyright (c) 2016 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/packet_buffer.h"
 
 #include <algorithm>
 #include <limits>
 #include <utility>
 
 #include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/common_video/h264/h264_common.h"
 #include "webrtc/modules/video_coding/frame_object.h"
 #include "webrtc/system_wrappers/include/clock.h"
 
 namespace webrtc {
 namespace video_coding {
 
 rtc::scoped_refptr<PacketBuffer> PacketBuffer::Create(
     Clock* clock,
     size_t start_buffer_size,
     size_t max_buffer_size,
@@ skipping 71 matching lines @@
 
     sequence_buffer_[index].frame_begin = packet->is_first_packet_in_frame;
     sequence_buffer_[index].frame_end = packet->markerBit;
     sequence_buffer_[index].seq_num = packet->seqNum;
     sequence_buffer_[index].continuous = false;
     sequence_buffer_[index].frame_created = false;
     sequence_buffer_[index].used = true;
     data_buffer_[index] = *packet;
     packet->dataPtr = nullptr;
 
+    UpdateMissingPackets(packet->seqNum);
+
     int64_t now_ms = clock_->TimeInMilliseconds();
     last_received_packet_ms_ = rtc::Optional<int64_t>(now_ms);
     if (packet->frameType == kVideoFrameKey)
       last_received_keyframe_packet_ms_ = rtc::Optional<int64_t>(now_ms);
 
     found_frames = FindFrames(seq_num);
   }
 
   for (std::unique_ptr<RtpFrameObject>& frame : found_frames)
     received_frame_callback_->OnReceivedFrame(std::move(frame));
 
   return true;
 }
 
 void PacketBuffer::ClearTo(uint16_t seq_num) {
   rtc::CritScope lock(&crit_);
 
   // If the packet buffer was cleared between a frame was created and returned.
   if (!first_packet_received_)
     return;
 
   is_cleared_to_first_seq_num_ = true;
   while (AheadOrAt<uint16_t>(seq_num, first_seq_num_)) {
     size_t index = first_seq_num_ % size_;
     delete[] data_buffer_[index].dataPtr;
     data_buffer_[index].dataPtr = nullptr;
     sequence_buffer_[index].used = false;
     ++first_seq_num_;
   }
+
+  missing_packets_.erase(missing_packets_.begin(),
+                         missing_packets_.upper_bound(seq_num));
 }
 
 void PacketBuffer::Clear() {
   rtc::CritScope lock(&crit_);
   for (size_t i = 0; i < size_; ++i) {
     delete[] data_buffer_[i].dataPtr;
     data_buffer_[i].dataPtr = nullptr;
     sequence_buffer_[i].used = false;
   }
 
   first_packet_received_ = false;
   is_cleared_to_first_seq_num_ = false;
   last_received_packet_ms_ = rtc::Optional<int64_t>();
   last_received_keyframe_packet_ms_ = rtc::Optional<int64_t>();
+  missing_packets_.clear();
+}
+
+void PacketBuffer::PaddingReceived(uint16_t seq_num) {
+  std::vector<std::unique_ptr<RtpFrameObject>> found_frames;
+  {
+    rtc::CritScope lock(&crit_);
+    UpdateMissingPackets(seq_num);
+    found_frames = FindFrames(static_cast<uint16_t>(seq_num + 1));
+  }
+
+  for (std::unique_ptr<RtpFrameObject>& frame : found_frames)
+    received_frame_callback_->OnReceivedFrame(std::move(frame));
 }
 
 rtc::Optional<int64_t> PacketBuffer::LastReceivedPacketMs() const {
   rtc::CritScope lock(&crit_);
   return last_received_packet_ms_;
 }
 
 rtc::Optional<int64_t> PacketBuffer::LastReceivedKeyframePacketMs() const {
   rtc::CritScope lock(&crit_);
   return last_received_keyframe_packet_ms_;
@@ skipping 23 matching lines @@
   LOG(LS_INFO) << "PacketBuffer size expanded to " << new_size;
   return true;
 }
 
 bool PacketBuffer::PotentialNewFrame(uint16_t seq_num) const {
   size_t index = seq_num % size_;
   int prev_index = index > 0 ? index - 1 : size_ - 1;
 
   if (!sequence_buffer_[index].used)
     return false;
+  if (sequence_buffer_[index].seq_num != seq_num)

[stefan-webrtc, 2017/06/07 11:39:16]

Not sure I follow this. If the sequence number received isn't the same as what
was already received it's not a new frame? When would this happen? Seems
unlikely except if we're receiving broken packets?

[philipel, 2017/06/07 14:04:34]

The problem we have now is that we can have received frames in our buffer that
we did not create a frame object for. This means we don't set the frame_created
flat to true for those packets, and in the case of the index wrapping this can
cause problems.

For example, if the packet buffer has a size of 16:
0 KF begin
1 KF end     <-- received keyframe, create frame [0, 1]
2 DROPPED!
3 DF begin
4 DF end     <-- received frame but no frame object due to gap
...
16 KF begin  <-- inserted at index 0
17 KF end    <-- inserted at index 1, created frame [16, 17]
18 DF begin  <-- inserted at index 2

When 18 is inserted it will be continuous (first packet of frame), we will then
check 19 (same index as packet 3), and since frame_created == false we will
check frame_begin, which is true. Lastly we check sequence number 20 (packet 4)
and it has the marker bit set. Now we will try to create a frame containing
packet 18, 3 and 4.

+    return false;
   if (sequence_buffer_[index].frame_created)
     return false;
   if (sequence_buffer_[index].frame_begin)
     return true;
   if (!sequence_buffer_[prev_index].used)
     return false;
   if (sequence_buffer_[prev_index].frame_created)
     return false;
   if (sequence_buffer_[prev_index].seq_num !=
       static_cast<uint16_t>(sequence_buffer_[index].seq_num - 1)) {
@@ skipping 17 matching lines @@
     if (sequence_buffer_[index].frame_end) {
       size_t frame_size = 0;
       int max_nack_count = -1;
       uint16_t start_seq_num = seq_num;
 
       // Find the start index by searching backward until the packet with
       // the |frame_begin| flag is set.
       int start_index = index;
 
       bool is_h264 = data_buffer_[start_index].codec == kVideoCodecH264;
+      bool is_h264_keyframe = false;
       int64_t frame_timestamp = data_buffer_[start_index].timestamp;
 
       // Since packet at |data_buffer_[index]| is already part of the frame
       // we will have at most |size_ - 1| packets left to check.
       for (size_t j = 0; j < size_ - 1; ++j) {
         frame_size += data_buffer_[start_index].sizeBytes;
         max_nack_count =
             std::max(max_nack_count, data_buffer_[start_index].timesNacked);
         sequence_buffer_[start_index].frame_created = true;
 
         if (!is_h264 && sequence_buffer_[start_index].frame_begin)
           break;
 
+        if (is_h264 && !is_h264_keyframe) {
+          const RTPVideoHeaderH264& header =
+              data_buffer_[start_index].video_header.codecHeader.H264;
+          for (size_t i = 0; i < header.nalus_length; ++i) {
+            if (header.nalus[i].type == H264::NaluType::kIdr) {
+              is_h264_keyframe = true;
+              break;
+            }
+          }
+        }
+
         start_index = start_index > 0 ? start_index - 1 : size_ - 1;
 
         // In the case of H264 we don't have a frame_begin bit (yes,
         // |frame_begin| might be set to true but that is a lie). So instead
         // we traverese backwards as long as we have a previous packet and
         // the timestamp of that packet is the same as this one. This may cause
         // the PacketBuffer to hand out incomplete frames.
         // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=7106
         if (is_h264 &&
             (!sequence_buffer_[start_index].used ||
              data_buffer_[start_index].timestamp != frame_timestamp)) {
           break;
         }
 
         --start_seq_num;
       }
 
+      // If this is H264 but not a keyframe, make sure there are no gaps in the
+      // packet sequence numbers up until this point.
+      if (is_h264 && !is_h264_keyframe &&
+          missing_packets_.upper_bound(start_seq_num) !=
+              missing_packets_.begin()) {
+        uint16_t stop_index = (index + 1) % size_;
+        while (start_index != stop_index) {
+          sequence_buffer_[start_index].frame_created = false;
+          start_index = (start_index + 1) % size_;
+        }
+
+        return found_frames;
+      }
+
+      // Clearing missing packets this way can cause H264 frames older than
+      // the last keyframe to be considered received.

[stefan-webrtc, 2017/06/07 11:39:17]

What does this imply? Are those packets even interesting to us after a keyframe
has been received?

[philipel, 2017/06/07 14:04:35]

I don't understand my own comment :)

What I wanted to say was that we can still create old (relative to the most
newest keyframe) incomplete H264 delta frames. Maybe it's easier to just remove
the comment all together.

[stefan-webrtc, 2017/06/12 08:56:21]

Is this a better comment?
// Frames prior to this key frame are no longer relevant.

In theory though, isn't there a possibility that the frames prior to this key
frame actually will arrive, meaning that we potentially could decode them on
time?

+      missing_packets_.erase(missing_packets_.begin(),
+                             missing_packets_.upper_bound(seq_num));
+
       found_frames.emplace_back(
           new RtpFrameObject(this, start_seq_num, seq_num, frame_size,
                              max_nack_count, clock_->TimeInMilliseconds()));
     }
     ++seq_num;
   }
   return found_frames;
 }
 
 void PacketBuffer::ReturnFrame(RtpFrameObject* frame) {
@@ skipping 50 matching lines @@
 }
 
 int PacketBuffer::Release() const {
   int count = rtc::AtomicOps::Decrement(&ref_count_);
   if (!count) {
     delete this;
   }
   return count;
 }
 
+void PacketBuffer::UpdateMissingPackets(uint16_t seq_num) {
+  if (!last_inserted_seq_num_)
+    last_inserted_seq_num_ = rtc::Optional<uint16_t>(seq_num);
+
+  const int kMaxPaddingAge = 1000;
+  if (AheadOf(seq_num, *last_inserted_seq_num_)) {
+    auto erase_to =
+        missing_packets_.lower_bound(*last_inserted_seq_num_ - kMaxPaddingAge);
+    missing_packets_.erase(missing_packets_.begin(), erase_to);
+
+    ++*last_inserted_seq_num_;
+    while (AheadOf(seq_num, *last_inserted_seq_num_)) {
+      missing_packets_.insert(*last_inserted_seq_num_);

[stefan-webrtc, 2017/06/07 11:39:16]

Should we guard for not inserting 65000 sequence numbers here in some way?

[philipel, 2017/06/07 14:04:35]

We can insert at most 2^15 missing packets, otherwise |seq_num| would be older
than |last_inserted_seq_num_|. We should still guard against inserting a large
amount of missing packets though...

+      ++*last_inserted_seq_num_;
+    }
+  } else {
+    missing_packets_.erase(seq_num);
+  }
+}
+
 }  // namespace video_coding
 }  // namespace webrtc