From d42e51ce7cd5673ef573bf852e656920c17d9e20 Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Wed, 28 Nov 2012 16:40:28 +0000 Subject: [PATCH] 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. TEST=trybots BUG=1150 Review URL: https://webrtc-codereview.appspot.com/966026 git-svn-id: http://webrtc.googlecode.com/svn/trunk@3181 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../main/interface/mock/mock_vcm_callbacks.h | 3 +- .../main/source/decoding_state.cc | 7 + .../video_coding/main/source/decoding_state.h | 1 + .../main/source/generic_decoder.cc | 3 +- .../video_coding/main/source/jitter_buffer.cc | 13 +- .../video_coding/main/source/session_info.cc | 58 ++++--- .../video_coding/main/source/session_info.h | 8 + .../main/source/video_coding_impl_unittest.cc | 153 +++++++++++++++++- 8 files changed, 213 insertions(+), 33 deletions(-) diff --git a/webrtc/modules/video_coding/main/interface/mock/mock_vcm_callbacks.h b/webrtc/modules/video_coding/main/interface/mock/mock_vcm_callbacks.h index c84d5e71c..4d6567ea5 100644 --- a/webrtc/modules/video_coding/main/interface/mock/mock_vcm_callbacks.h +++ b/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 { diff --git a/webrtc/modules/video_coding/main/source/decoding_state.cc b/webrtc/modules/video_coding/main/source/decoding_state.cc index e347a2c76..c58ef9288 100644 --- a/webrtc/modules/video_coding/main/source/decoding_state.cc +++ b/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_) { diff --git a/webrtc/modules/video_coding/main/source/decoding_state.h b/webrtc/modules/video_coding/main/source/decoding_state.h index 136bae6b6..cdfcf9fff 100644 --- a/webrtc/modules/video_coding/main/source/decoding_state.h +++ b/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. diff --git a/webrtc/modules/video_coding/main/source/generic_decoder.cc b/webrtc/modules/video_coding/main/source/generic_decoder.cc index edb6a64d8..3139ba883 100644 --- a/webrtc/modules/video_coding/main/source/generic_decoder.cc +++ b/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; } diff --git a/webrtc/modules/video_coding/main/source/jitter_buffer.cc b/webrtc/modules/video_coding/main/source/jitter_buffer.cc index 0af298446..c80b84a07 100644 --- a/webrtc/modules/video_coding/main/source/jitter_buffer.cc +++ b/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 { diff --git a/webrtc/modules/video_coding/main/source/session_info.cc b/webrtc/modules/video_coding/main/source/session_info.cc index 726140386..4d41f0f23 100644 --- a/webrtc/modules/video_coding/main/source/session_info.cc +++ b/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,6 +491,17 @@ 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) && @@ -497,15 +514,14 @@ int VCMSessionInfo::BuildSoftNackList(int* seq_num_list, } // Mark empty packets. - while (seq_num_list[index] <= empty_seq_num_high_ && + while (seq_num_list[index] >= 0 && + seq_num_list[index] <= empty_seq_num_high_ && index < seq_num_list_length) { seq_num_list[index] = -2; ++index; } } - - session_nack_ = allow_nack; - return 0; + return index; } int VCMSessionInfo::PacketsMissing(const PacketIterator& packet_it, diff --git a/webrtc/modules/video_coding/main/source/session_info.h b/webrtc/modules/video_coding/main/source/session_info.h index 27533cee0..bf7e543fa 100644 --- a/webrtc/modules/video_coding/main/source/session_info.h +++ b/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_; diff --git a/webrtc/modules/video_coding/main/source/video_coding_impl_unittest.cc b/webrtc/modules/video_coding/main/source/video_coding_impl_unittest.cc index 27297ac4f..4f89d3ee7 100644 --- a/webrtc/modules/video_coding/main/source/video_coding_impl_unittest.cc +++ b/webrtc/modules/video_coding/main/source/video_coding_impl_unittest.cc @@ -10,9 +10,11 @@ #include -#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 decoder_; NiceMock encoder_; I420VideoFrame input_frame_; VideoCodec settings_; + NiceMock 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