Break backwards traversal loop if we have looped around all packet in the PacketBuffer for H264 fra…

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.
  */
 
@@ skipping 180 matching lines @@
   if (sequence_buffer_[prev_index].continuous)
     return true;
 
   return false;
 }
 
 std::vector<std::unique_ptr<RtpFrameObject>> PacketBuffer::FindFrames(
     uint16_t seq_num) {
   std::vector<std::unique_ptr<RtpFrameObject>> found_frames;
   size_t packets_tested = 0;
   while (packets_tested < size_ && PotentialNewFrame(seq_num)) {

[stefan-webrtc, 2017/05/08 11:47:27]

Can this perhaps also be a for loop?

[philipel, 2017/05/08 12:56:23]

Done.

     size_t index = seq_num % size_;
     sequence_buffer_[index].continuous = true;
 
     // If all packets of the frame is continuous, find the first packet of the
     // frame and create an RtpFrameObject.
     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;
       int64_t frame_timestamp = data_buffer_[start_index].timestamp;
       while (true) {

[stefan-webrtc, 2017/05/08 11:47:27]

Try to get rid of this while loop as it's hard to be sure it doesn't result in
infinite loops. Should be enough with only looping through the packet buffer
once, right?

[philipel, 2017/05/08 12:56:23]

Agree that a loop is cleaner, fixed.

         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;
 
         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
         //
         // Since we ignore the |frame_begin| flag of the inserted packets
-        // we check that |start_index != static_cast<int>(index)| to make sure
+        // we check if |start_index == static_cast<int>(index)| to make sure
         // that we don't get stuck in a loop if the packet buffer is filled
         // with packets of the same timestamp.
-        if (is_h264 && start_index != static_cast<int>(index) &&
+        if ((is_h264 && start_index == static_cast<int>(index)) ||
             (!sequence_buffer_[start_index].used ||
              data_buffer_[start_index].timestamp != frame_timestamp)) {
           break;
         }
 
         --start_seq_num;
       }
 
       found_frames.emplace_back(
           new RtpFrameObject(this, start_seq_num, seq_num, frame_size,
@@ skipping 61 matching lines @@
 int PacketBuffer::Release() const {
   int count = rtc::AtomicOps::Decrement(&ref_count_);
   if (!count) {
     delete this;
   }
   return count;
 }
 
 }  // namespace video_coding
 }  // namespace webrtc