Fixes two bugs related to padding in the jitter buffer.

- Pad packets (empty) were often NACKed even though they were received.
- Padding only frames (empty) were didn't properly update the decoding state,
  and would therefore be NACKed even though they were received.

This is a recommit of r3183. Extensive testing suggest that this may have been caused by virtual machine flakiness.

TBR=mflodman@webrtc.org

BUG=1150

Review URL: https://webrtc-codereview.appspot.com/971011

git-svn-id: http://webrtc.googlecode.com/svn/trunk@3200 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/video_coding/main/interface/mock/mock_vcm_callbacks.h

@@ -12,7 +12,8 @@
 #define WEBRTC_MODULES_VIDEO_CODING_MAIN_INTERFACE_MOCK_MOCK_VCM_CALLBACKS_H_
 
 #include "gmock/gmock.h"
-#include "typedefs.h"
+#include "webrtc/modules/video_coding/main/interface/video_coding_defines.h"
+#include "webrtc/typedefs.h"
 
 namespace webrtc {
 

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

@@ -94,6 +94,13 @@ void VCMDecodingState::SetStateOneBack(const VCMFrameBuffer* frame) {
   init_ = false;
 }
 
+void VCMDecodingState::UpdateEmptyFrame(const VCMFrameBuffer* frame) {
+  if (ContinuousFrame(frame) && frame->GetState() == kStateEmpty) {
+    time_stamp_ = frame->TimeStamp();
+    sequence_num_ = frame->GetHighSeqNum();
+  }
+}
+
 void VCMDecodingState::UpdateOldPacket(const VCMPacket* packet) {
   assert(packet != NULL);
   if (packet->timestamp == time_stamp_) {

webrtc/modules/video_coding/main/source/decoding_state.h

@@ -33,6 +33,7 @@ class VCMDecodingState {
   void SetState(const VCMFrameBuffer* frame);
   // Set the decoding state one frame back.
   void SetStateOneBack(const VCMFrameBuffer* frame);
+  void UpdateEmptyFrame(const VCMFrameBuffer* frame);
   // Update the sequence number if the timestamp matches current state and the
   // sequence number is higher than the current one. This accounts for packets
   // arriving late.

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

@@ -185,7 +185,8 @@ WebRtc_Word32 VCMGenericDecoder::Decode(const VCMEncodedFrame& frame,
         _callback->Pop(frame.TimeStamp());
     }
     // Update the key frame decoded variable so that we know whether or not we've decoded a key frame since reset.
-    _keyFrameDecoded = (frame.FrameType() == kVideoFrameKey || frame.FrameType() == kVideoFrameGolden);
+    _keyFrameDecoded = (_keyFrameDecoded ||
+        frame.FrameType() == kVideoFrameKey);
     return ret;
 }
 

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

@@ -933,8 +933,7 @@ uint16_t* VCMJitterBuffer::CreateNackList(uint16_t* nack_list_size,
     // We don't need to check if frame is decoding since low_seq_num is based
     // on the last decoded sequence number.
     VCMFrameBufferStateEnum state = frame_buffers_[i]->GetState();
-    if ((kStateFree != state) &&
-        (kStateEmpty != state)) {
+    if (kStateFree != state) {
       // Reaching thus far means we are going to update the NACK list
       // When in hybrid mode, we use the soft NACKing feature.
       if (nack_mode_ == kNackHybrid) {
@@ -1271,11 +1270,11 @@ FrameList::iterator VCMJitterBuffer::FindOldestCompleteContinuousFrame(
 void VCMJitterBuffer::CleanUpOldFrames() {
   while (frame_list_.size() > 0) {
     VCMFrameBuffer* oldest_frame = frame_list_.front();
-    bool next_frame_empty =
-        (last_decoded_state_.ContinuousFrame(oldest_frame) &&
-         oldest_frame->GetState() == kStateEmpty);
-    if (last_decoded_state_.IsOldFrame(oldest_frame) ||
-        (next_frame_empty && frame_list_.size() > 1)) {
+    if (oldest_frame->GetState() == kStateEmpty && frame_list_.size() > 1) {
+      // This frame is empty, mark it as decoded, thereby making it old.
+      last_decoded_state_.UpdateEmptyFrame(oldest_frame);
+    }
+    if (last_decoded_state_.IsOldFrame(oldest_frame)) {
       ReleaseFrameIfNotDecoding(frame_list_.front());
       frame_list_.erase(frame_list_.begin());
     } else {

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

@@ -357,38 +357,44 @@ int VCMSessionInfo::BuildHardNackList(int* seq_num_list,
   if (NULL == seq_num_list || seq_num_list_length < 1) {
     return -1;
   }
-  if (packets_.empty()) {
+  if (packets_.empty() && empty_seq_num_low_ == -1) {
     return 0;
   }
 
   // Find end point (index of entry equals the sequence number of the first
   // packet).
   int index = 0;
+  int low_seq_num = (packets_.empty()) ? empty_seq_num_low_:
+      packets_.front().seqNum;
   for (; index < seq_num_list_length; ++index) {
-    if (seq_num_list[index] == packets_.front().seqNum) {
+    if (seq_num_list[index] == low_seq_num) {
       seq_num_list[index] = -1;
       ++index;
       break;
     }
   }
 
-  // Zero out between the first entry and the end point.
-  PacketIterator it = packets_.begin();
-  PacketIterator prev_it = it;
-  ++it;
-  while (it != packets_.end() && index < seq_num_list_length) {
-    if (!InSequence(it, prev_it)) {
-      // Found a sequence number gap due to packet loss.
-      index += PacketsMissing(it, prev_it);
-      session_nack_ = true;
-    }
-    seq_num_list[index] = -1;
-    ++index;
-    prev_it = it;
+  if (!packets_.empty()) {
+    // Zero out between the first entry and the end point.
+    PacketIterator it = packets_.begin();
+    PacketIterator prev_it = it;
     ++it;
+    while (it != packets_.end() && index < seq_num_list_length) {
+      if (!InSequence(it, prev_it)) {
+        // Found a sequence number gap due to packet loss.
+        index += PacketsMissing(it, prev_it);
+        session_nack_ = true;
+      }
+      seq_num_list[index] = -1;
+      ++index;
+      prev_it = it;
+      ++it;
+    }
+    if (!packets_.front().isFirstPacket)
+        session_nack_ = true;
   }
-  if (!packets_.front().isFirstPacket)
-    session_nack_ = true;
+  index = ClearOutEmptyPacketSequenceNumbers(seq_num_list, seq_num_list_length,
+                                             index);
   return 0;
 }
 
@@ -485,27 +491,36 @@ int VCMSessionInfo::BuildSoftNackList(int* seq_num_list,
     }
   }
 
+  index = ClearOutEmptyPacketSequenceNumbers(seq_num_list, seq_num_list_length,
+                                             index);
+
+  session_nack_ = allow_nack;
+  return 0;
+}
+
+int VCMSessionInfo::ClearOutEmptyPacketSequenceNumbers(
+    int* seq_num_list,
+    int seq_num_list_length,
+    int index) const {
   // Empty packets follow the data packets, and therefore have a higher
   // sequence number. We do not want to NACK empty packets.
-  if ((empty_seq_num_low_ != -1) && (empty_seq_num_high_ != -1) &&
-      (index < seq_num_list_length)) {
+  if (empty_seq_num_low_ != -1 && empty_seq_num_high_ != -1) {
     // First make sure that we are at least at the minimum value (if not we are
     // missing last packet(s)).
-    while (seq_num_list[index] < empty_seq_num_low_ &&
-        index < seq_num_list_length) {
+    while (index < seq_num_list_length &&
+           seq_num_list[index] < empty_seq_num_low_) {
       ++index;
     }
 
     // Mark empty packets.
-    while (seq_num_list[index] <= empty_seq_num_high_ &&
-        index < seq_num_list_length) {
+    while (index < seq_num_list_length &&
+           seq_num_list[index] >= 0 &&
+           seq_num_list[index] <= empty_seq_num_high_) {
       seq_num_list[index] = -2;
       ++index;
     }
   }
-
-  session_nack_ = allow_nack;
-  return 0;
+  return index;
 }
 
 int VCMSessionInfo::PacketsMissing(const PacketIterator& packet_it,

webrtc/modules/video_coding/main/source/session_info.h

@@ -115,6 +115,14 @@ class VCMSessionInfo {
   // would be sent to the decoder.
   void UpdateDecodableSession(int rtt_ms);
 
+  // Clears the sequence numbers in |seq_num_list| of any empty packets received
+  // in this session. |index| is an index in the list at which we start looking
+  // for the sequence numbers. When done this function returns the index of the
+  // next element in the list.
+  int ClearOutEmptyPacketSequenceNumbers(int* seq_num_list,
+                                         int seq_num_list_length,
+                                         int index) const;
+
   // If this session has been NACKed by the jitter buffer.
   bool session_nack_;
   bool complete_;

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

@@ -10,9 +10,11 @@
 
 #include <vector>
 
-#include "modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h"
-#include "modules/video_coding/main/interface/video_coding.h"
-#include "system_wrappers/interface/scoped_ptr.h"
+#include "webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h"
+#include "webrtc/modules/video_coding/main/interface/mock/mock_vcm_callbacks.h"
+#include "webrtc/modules/video_coding/main/interface/video_coding.h"
+#include "webrtc/system_wrappers/interface/scoped_ptr.h"
+#include "webrtc/system_wrappers/interface/tick_util.h"
 
 #include "gtest/gtest.h"
 
@@ -36,9 +38,14 @@ class TestVideoCodingModule : public ::testing::Test {
   static const int kUnusedPayloadType = 10;
 
   virtual void SetUp() {
+    TickTime::UseFakeClock(0);
     vcm_ = VideoCodingModule::Create(0);
+    EXPECT_EQ(0, vcm_->InitializeReceiver());
+    EXPECT_EQ(0, vcm_->InitializeSender());
     EXPECT_EQ(0, vcm_->RegisterExternalEncoder(&encoder_, kUnusedPayloadType,
                                                false));
+    EXPECT_EQ(0, vcm_->RegisterExternalDecoder(&decoder_, kUnusedPayloadType,
+                                               true));
     memset(&settings_, 0, sizeof(settings_));
     EXPECT_EQ(0, vcm_->Codec(kVideoCodecVP8, &settings_));
     settings_.numberOfSimulcastStreams = kNumberOfStreams;
@@ -50,6 +57,7 @@ class TestVideoCodingModule : public ::testing::Test {
                     &settings_.simulcastStream[2]);
     settings_.plType = kUnusedPayloadType;  // Use the mocked encoder.
     EXPECT_EQ(0, vcm_->RegisterSendCodec(&settings_, 1, 1200));
+    EXPECT_EQ(0, vcm_->RegisterReceiveCodec(&settings_, 1, true));
   }
 
   virtual void TearDown() {
@@ -85,10 +93,41 @@ class TestVideoCodingModule : public ::testing::Test {
     stream->qpMax = 45;
   }
 
+  void InsertAndVerifyPaddingFrame(const uint8_t* payload, int length,
+                                   WebRtcRTPHeader* header) {
+    ASSERT_TRUE(header != NULL);
+    for (int j = 0; j < 5; ++j) {
+      // Padding only packets are passed to the VCM with payload size 0.
+      EXPECT_EQ(0, vcm_->IncomingPacket(payload, 0, *header));
+      ++header->header.sequenceNumber;
+    }
+    EXPECT_CALL(packet_request_callback_, ResendPackets(_, _))
+        .Times(0);
+    EXPECT_EQ(0, vcm_->Process());
+    EXPECT_CALL(decoder_, Decode(_, _, _, _, _))
+        .Times(0);
+    EXPECT_EQ(VCM_FRAME_NOT_READY, vcm_->Decode(0));
+  }
+
+  void InsertAndVerifyDecodableFrame(const uint8_t* payload, int length,
+                                     WebRtcRTPHeader* header) {
+    ASSERT_TRUE(header != NULL);
+    EXPECT_EQ(0, vcm_->IncomingPacket(payload, length, *header));
+    ++header->header.sequenceNumber;
+    EXPECT_CALL(packet_request_callback_, ResendPackets(_, _))
+        .Times(0);
+    EXPECT_EQ(0, vcm_->Process());
+    EXPECT_CALL(decoder_, Decode(_, _, _, _, _))
+        .Times(1);
+    EXPECT_EQ(0, vcm_->Decode(0));
+  }
+
   VideoCodingModule* vcm_;
+  NiceMock<MockVideoDecoder> decoder_;
   NiceMock<MockVideoEncoder> encoder_;
   I420VideoFrame input_frame_;
   VideoCodec settings_;
+  NiceMock<MockPacketRequestCallback> packet_request_callback_;
 };
 
 TEST_F(TestVideoCodingModule, TestIntraRequests) {
@@ -137,4 +176,112 @@ TEST_F(TestVideoCodingModule, TestIntraRequestsInternalCapture) {
   EXPECT_EQ(-1, vcm_->IntraFrameRequest(-1));
 }
 
+TEST_F(TestVideoCodingModule, PaddingOnlyFrames) {
+  EXPECT_EQ(0, vcm_->SetVideoProtection(kProtectionNack, true));
+  EXPECT_EQ(0, vcm_->RegisterPacketRequestCallback(&packet_request_callback_));
+  const unsigned int kPaddingSize = 220;
+  const uint8_t payload[kPaddingSize] = {0};
+  WebRtcRTPHeader header;
+  memset(&header, 0, sizeof(header));
+  header.frameType = kFrameEmpty;
+  header.header.markerBit = false;
+  header.header.paddingLength = kPaddingSize;
+  header.header.payloadType = kUnusedPayloadType;
+  header.header.ssrc = 1;
+  header.header.headerLength = 12;
+  header.type.Video.codec = kRTPVideoVP8;
+  for (int i = 0; i < 10; ++i) {
+    InsertAndVerifyPaddingFrame(payload, 0, &header);
+    TickTime::AdvanceFakeClock(33);
+    header.header.timestamp += 3000;
+  }
+}
+
+TEST_F(TestVideoCodingModule, PaddingOnlyFramesWithLosses) {
+  EXPECT_EQ(0, vcm_->SetVideoProtection(kProtectionNack, true));
+  EXPECT_EQ(0, vcm_->RegisterPacketRequestCallback(&packet_request_callback_));
+  const unsigned int kFrameSize = 1200;
+  const unsigned int kPaddingSize = 220;
+  const uint8_t payload[kPaddingSize] = {0};
+  WebRtcRTPHeader header;
+  memset(&header, 0, sizeof(header));
+  header.frameType = kFrameEmpty;
+  header.header.markerBit = false;
+  header.header.paddingLength = kPaddingSize;
+  header.header.payloadType = kUnusedPayloadType;
+  header.header.ssrc = 1;
+  header.header.headerLength = 12;
+  header.type.Video.codec = kRTPVideoVP8;
+  // Insert one video frame to get one frame decoded.
+  header.frameType = kVideoFrameKey;
+  header.type.Video.isFirstPacket = true;
+  header.header.markerBit = true;
+  InsertAndVerifyDecodableFrame(payload, kFrameSize, &header);
+  TickTime::AdvanceFakeClock(33);
+  header.header.timestamp += 3000;
+
+  header.frameType = kFrameEmpty;
+  header.type.Video.isFirstPacket = false;
+  header.header.markerBit = false;
+  // Insert padding frames.
+  for (int i = 0; i < 10; ++i) {
+    // Lose the 4th frame.
+    if (i == 3) {
+      header.header.sequenceNumber += 5;
+      ++i;
+    }
+    // Lose one packet from the 6th frame.
+    if (i == 5) {
+      ++header.header.sequenceNumber;
+    }
+    InsertAndVerifyPaddingFrame(payload, 0, &header);
+    TickTime::AdvanceFakeClock(33);
+    header.header.timestamp += 3000;
+  }
+}
+
+TEST_F(TestVideoCodingModule, PaddingOnlyAndVideo) {
+  EXPECT_EQ(0, vcm_->SetVideoProtection(kProtectionNack, true));
+  EXPECT_EQ(0, vcm_->RegisterPacketRequestCallback(&packet_request_callback_));
+  const unsigned int kFrameSize = 1200;
+  const unsigned int kPaddingSize = 220;
+  const uint8_t payload[kPaddingSize] = {0};
+  WebRtcRTPHeader header;
+  memset(&header, 0, sizeof(header));
+  header.frameType = kFrameEmpty;
+  header.type.Video.isFirstPacket = false;
+  header.header.markerBit = false;
+  header.header.paddingLength = kPaddingSize;
+  header.header.payloadType = kUnusedPayloadType;
+  header.header.ssrc = 1;
+  header.header.headerLength = 12;
+  header.type.Video.codec = kRTPVideoVP8;
+  header.type.Video.codecHeader.VP8.pictureId = -1;
+  header.type.Video.codecHeader.VP8.tl0PicIdx = -1;
+  for (int i = 0; i < 3; ++i) {
+    // Insert 2 video frames.
+    for (int j = 0; j < 2; ++j) {
+      if (i == 0 && j == 0)  // First frame should be a key frame.
+        header.frameType = kVideoFrameKey;
+      else
+        header.frameType = kVideoFrameDelta;
+      header.type.Video.isFirstPacket = true;
+      header.header.markerBit = true;
+      InsertAndVerifyDecodableFrame(payload, kFrameSize, &header);
+      TickTime::AdvanceFakeClock(33);
+      header.header.timestamp += 3000;
+    }
+
+    // Insert 2 padding only frames.
+    header.frameType = kFrameEmpty;
+    header.type.Video.isFirstPacket = false;
+    header.header.markerBit = false;
+    for (int j = 0; j < 2; ++j) {
+      InsertAndVerifyPaddingFrame(payload, 0, &header);
+      TickTime::AdvanceFakeClock(33);
+      header.header.timestamp += 3000;
+    }
+  }
+}
+
 }  // namespace webrtc