Revert 3181 - 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) didn't properly update the decoding state,
  and would therefore be NACKed even though they were received.

Broke [Builder Win32Debug] (http://webrtc-cb-linux-master.cbf.corp.google.com:8010/builders/Win32Debug/builds/1728)

TEST=trybots

BUG=1150

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

TBR=stefan@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/939031

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

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

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

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

@@ -94,13 +94,6 @@ 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,7 +33,6 @@ 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,8 +185,7 @@ 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 = (_keyFrameDecoded ||
+    _keyFrameDecoded = (frame.FrameType() == kVideoFrameKey || frame.FrameType() == kVideoFrameGolden);
-        frame.FrameType() == kVideoFrameKey);
     return ret;
 }
 

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

@@ -933,7 +933,8 @@ 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) {
+    if ((kStateFree != state) &&
+        (kStateEmpty != 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) {
@@ -1270,11 +1271,11 @@ FrameList::iterator VCMJitterBuffer::FindOldestCompleteContinuousFrame(
 void VCMJitterBuffer::CleanUpOldFrames() {
   while (frame_list_.size() > 0) {
     VCMFrameBuffer* oldest_frame = frame_list_.front();
-    if (oldest_frame->GetState() == kStateEmpty && frame_list_.size() > 1) {
+    bool next_frame_empty =
-      // This frame is empty, mark it as decoded, thereby making it old.
+        (last_decoded_state_.ContinuousFrame(oldest_frame) &&
-      last_decoded_state_.UpdateEmptyFrame(oldest_frame);
+         oldest_frame->GetState() == kStateEmpty);
-    }
+    if (last_decoded_state_.IsOldFrame(oldest_frame) ||
-    if (last_decoded_state_.IsOldFrame(oldest_frame)) {
+        (next_frame_empty && frame_list_.size() > 1)) {
       ReleaseFrameIfNotDecoding(frame_list_.front());
       frame_list_.erase(frame_list_.begin());
     } else {

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

@@ -357,44 +357,38 @@ int VCMSessionInfo::BuildHardNackList(int* seq_num_list,
   if (NULL == seq_num_list || seq_num_list_length < 1) {
     return -1;
   }
-  if (packets_.empty() && empty_seq_num_low_ == -1) {
+  if (packets_.empty()) {
     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] == low_seq_num) {
+    if (seq_num_list[index] == packets_.front().seqNum) {
       seq_num_list[index] = -1;
       ++index;
       break;
     }
   }
 
-  if (!packets_.empty()) {
+  // Zero out between the first entry and the end point.
-    // Zero out between the first entry and the end point.
+  PacketIterator it = packets_.begin();
-    PacketIterator it = packets_.begin();
+  PacketIterator prev_it = it;
-    PacketIterator prev_it = it;
+  ++it;
-    ++it;
+  while (it != packets_.end() && index < seq_num_list_length) {
-    while (it != packets_.end() && index < seq_num_list_length) {
+    if (!InSequence(it, prev_it)) {
-      if (!InSequence(it, prev_it)) {
+      // Found a sequence number gap due to packet loss.
-        // Found a sequence number gap due to packet loss.
+      index += PacketsMissing(it, prev_it);
-        index += PacketsMissing(it, prev_it);
+      session_nack_ = true;
-        session_nack_ = true;
-      }
-      seq_num_list[index] = -1;
-      ++index;
-      prev_it = it;
-      ++it;
     }
-    if (!packets_.front().isFirstPacket)
+    seq_num_list[index] = -1;
-        session_nack_ = true;
+    ++index;
+    prev_it = it;
+    ++it;
   }
-  index = ClearOutEmptyPacketSequenceNumbers(seq_num_list, seq_num_list_length,
+  if (!packets_.front().isFirstPacket)
-                                             index);
+    session_nack_ = true;
   return 0;
 }
 
@@ -491,17 +485,6 @@ 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) &&
@@ -514,14 +497,15 @@ int VCMSessionInfo::ClearOutEmptyPacketSequenceNumbers(
     }
 
     // Mark empty packets.
-    while (seq_num_list[index] >= 0 &&
+    while (seq_num_list[index] <= empty_seq_num_high_ &&
-        seq_num_list[index] <= empty_seq_num_high_ &&
         index < seq_num_list_length) {
       seq_num_list[index] = -2;
       ++index;
     }
   }
-  return index;
+
+  session_nack_ = allow_nack;
+  return 0;
 }
 
 int VCMSessionInfo::PacketsMissing(const PacketIterator& packet_it,

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

@@ -115,14 +115,6 @@ 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,11 +10,9 @@
 
 #include <vector>
 
-#include "webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h"
+#include "modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h"
-#include "webrtc/modules/video_coding/main/interface/mock/mock_vcm_callbacks.h"
+#include "modules/video_coding/main/interface/video_coding.h"
-#include "webrtc/modules/video_coding/main/interface/video_coding.h"
+#include "system_wrappers/interface/scoped_ptr.h"
-#include "webrtc/system_wrappers/interface/scoped_ptr.h"
-#include "webrtc/system_wrappers/interface/tick_util.h"
 
 #include "gtest/gtest.h"
 
@@ -38,14 +36,9 @@ 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;
@@ -57,7 +50,6 @@ 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() {
@@ -93,41 +85,10 @@ 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) {
@@ -176,112 +137,4 @@ 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