From 23fd559bcd2ffb272927ee4f2d91b52c21db85a9 Mon Sep 17 00:00:00 2001 From: "asapersson@webrtc.org" Date: Mon, 24 Sep 2012 12:07:13 +0000 Subject: [PATCH] Increased bytes per interval factor. Added limits for the delay between packets. Review URL: https://webrtc-codereview.appspot.com/828005 git-svn-id: http://webrtc.googlecode.com/svn/trunk@2809 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../rtp_rtcp/source/rtp_packet_history.cc | 3 +- .../source/rtp_packet_history_unittest.cc | 24 +++- src/modules/rtp_rtcp/source/rtp_sender.cc | 6 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 1 + .../rtp_rtcp/source/transmission_bucket.cc | 93 ++++++++------ .../rtp_rtcp/source/transmission_bucket.h | 46 ++++--- .../source/transmission_bucket_unittest.cc | 119 ++++++++++++++---- 7 files changed, 211 insertions(+), 81 deletions(-) diff --git a/src/modules/rtp_rtcp/source/rtp_packet_history.cc b/src/modules/rtp_rtcp/source/rtp_packet_history.cc index 3f09f6dc74..df5caf113c 100644 --- a/src/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/src/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -139,7 +139,8 @@ int32_t RTPPacketHistory::PutRTPPacket(const uint8_t* packet, stored_seq_nums_[prev_index_] = seq_num; stored_lengths_[prev_index_] = packet_length; - stored_times_[prev_index_] = capture_time_ms; + stored_times_[prev_index_] = + (capture_time_ms > 0) ? capture_time_ms : clock_.GetTimeInMS(); stored_resend_times_[prev_index_] = 0; // packet not resent stored_types_[prev_index_] = type; diff --git a/src/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/src/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index 47f3df5d76..7058fd705a 100644 --- a/src/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/src/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -160,7 +160,7 @@ TEST_F(RtpPacketHistoryTest, PutRtpPacket) { TEST_F(RtpPacketHistoryTest, GetRtpPacket) { hist_->SetStorePacketsStatus(true, 10); uint16_t len = 0; - int64_t capture_time_ms = fake_clock_.GetTimeInMS(); + int64_t capture_time_ms = 1; CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len); EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength, capture_time_ms, kAllowRetransmission)); @@ -178,6 +178,28 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) { } } +TEST_F(RtpPacketHistoryTest, NoCaptureTime) { + hist_->SetStorePacketsStatus(true, 10); + uint16_t len = 0; + fake_clock_.IncrementTime(1); + int64_t capture_time_ms = fake_clock_.GetTimeInMS(); + CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len); + EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength, + -1, kAllowRetransmission)); + + uint16_t len_out = kMaxPacketLength; + int64_t time; + StorageType type; + EXPECT_TRUE(hist_->GetRTPPacket(kSeqNum, 0, packet_out_, &len_out, &time, + &type)); + EXPECT_EQ(len, len_out); + EXPECT_EQ(kAllowRetransmission, type); + EXPECT_EQ(capture_time_ms, time); + for (int i = 0; i < len; i++) { + EXPECT_EQ(packet_[i], packet_out_[i]); + } +} + TEST_F(RtpPacketHistoryTest, DontRetransmit) { hist_->SetStorePacketsStatus(true, 10); uint16_t len = 0; diff --git a/src/modules/rtp_rtcp/source/rtp_sender.cc b/src/modules/rtp_rtcp/source/rtp_sender.cc index f70f0dfb9e..d61acdc003 100644 --- a/src/modules/rtp_rtcp/source/rtp_sender.cc +++ b/src/modules/rtp_rtcp/source/rtp_sender.cc @@ -51,7 +51,7 @@ RTPSender::RTPSender(const WebRtc_Word32 id, _nackBitrate(clock), _packetHistory(new RTPPacketHistory(clock)), - _sendBucket(), + _sendBucket(clock), _timeLastSendToNetworkUpdate(clock->GetTimeInMS()), _transmissionSmoothing(false), @@ -882,7 +882,9 @@ RTPSender::SendToNetwork(WebRtc_UWord8* buffer, if (_transmissionSmoothing) { const WebRtc_UWord16 sequenceNumber = (buffer[2] << 8) + buffer[3]; - _sendBucket.Fill(sequenceNumber, rtpLength + length); + const WebRtc_UWord32 timestamp = (buffer[4] << 24) + (buffer[5] << 16) + + (buffer[6] << 8) + buffer[7]; + _sendBucket.Fill(sequenceNumber, timestamp, rtpLength + length); // Packet will be sent at a later time. return 0; } diff --git a/src/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/src/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 7bbb1908c4..f1ef5945e6 100644 --- a/src/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/src/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -221,6 +221,7 @@ TEST_F(RtpSenderTest, TrafficSmoothing) { EXPECT_EQ(0, rtp_sender_->SetStorePacketsStatus(true, 10)); EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(kType, kId)); EXPECT_EQ(0, rtp_sender_->RegisterSendTransport(&transport_)); + rtp_sender_->SetTargetSendBitrate(300000); WebRtc_Word32 rtp_length = rtp_sender_->BuildRTPheader(packet_, kPayload, diff --git a/src/modules/rtp_rtcp/source/transmission_bucket.cc b/src/modules/rtp_rtcp/source/transmission_bucket.cc index e79d227a4e..0d223b44ae 100644 --- a/src/modules/rtp_rtcp/source/transmission_bucket.cc +++ b/src/modules/rtp_rtcp/source/transmission_bucket.cc @@ -12,16 +12,17 @@ #include #include "critical_section_wrapper.h" +#include "rtp_utility.h" namespace webrtc { -TransmissionBucket::TransmissionBucket() - : critsect_(CriticalSectionWrapper::CreateCriticalSection()), +TransmissionBucket::TransmissionBucket(RtpRtcpClock* clock) + : clock_(clock), + critsect_(CriticalSectionWrapper::CreateCriticalSection()), accumulator_(0), - bytes_rem_total_(0), bytes_rem_interval_(0), packets_(), - first_(true) { + last_transmitted_packet_(0, 0, 0, 0) { } TransmissionBucket::~TransmissionBucket() { @@ -32,18 +33,17 @@ TransmissionBucket::~TransmissionBucket() { void TransmissionBucket::Reset() { webrtc::CriticalSectionScoped cs(*critsect_); accumulator_ = 0; - bytes_rem_total_ = 0; bytes_rem_interval_ = 0; packets_.clear(); - first_ = true; } -void TransmissionBucket::Fill(const uint16_t seq_num, - const uint32_t num_bytes) { +void TransmissionBucket::Fill(uint16_t seq_num, + uint32_t timestamp, + uint16_t num_bytes) { webrtc::CriticalSectionScoped cs(*critsect_); accumulator_ += num_bytes; - Packet p(seq_num, num_bytes); + Packet p(seq_num, timestamp, num_bytes, clock_->GetTimeInMS()); packets_.push_back(p); } @@ -53,11 +53,11 @@ bool TransmissionBucket::Empty() { } void TransmissionBucket::UpdateBytesPerInterval( - const uint32_t delta_time_ms, - const uint16_t target_bitrate_kbps) { + uint32_t delta_time_ms, + uint16_t target_bitrate_kbps) { webrtc::CriticalSectionScoped cs(*critsect_); - const float kMargin = 1.05f; + const float kMargin = 1.5f; uint32_t bytes_per_interval = kMargin * (target_bitrate_kbps * delta_time_ms / 8); @@ -66,12 +66,6 @@ void TransmissionBucket::UpdateBytesPerInterval( } else { bytes_rem_interval_ = bytes_per_interval; } - - if (accumulator_) { - bytes_rem_total_ += bytes_per_interval; - return; - } - bytes_rem_total_ = bytes_per_interval; } int32_t TransmissionBucket::GetNextPacket() { @@ -83,35 +77,64 @@ int32_t TransmissionBucket::GetNextPacket() { } std::vector::const_iterator it_begin = packets_.begin(); - const uint16_t num_bytes = (*it_begin).length_; - const uint16_t seq_num = (*it_begin).sequence_number_; + const uint16_t num_bytes = (*it_begin).length; + const uint16_t seq_num = (*it_begin).sequence_number; - if (first_) { - // Ok to transmit first packet. - first_ = false; - packets_.erase(packets_.begin()); - return seq_num; - } - - const float kFrameComplete = 0.80f; - if (num_bytes * kFrameComplete > bytes_rem_total_) { - // Packet does not fit. - return -1; - } - - if (bytes_rem_interval_ <= 0) { + if (bytes_rem_interval_ <= 0 && + !SameFrameAndPacketIntervalTimeElapsed(*it_begin) && + !NewFrameAndFrameIntervalTimeElapsed(*it_begin)) { // All bytes consumed for this interval. return -1; } // Ok to transmit packet. - bytes_rem_total_ -= num_bytes; bytes_rem_interval_ -= num_bytes; assert(accumulator_ >= num_bytes); accumulator_ -= num_bytes; + last_transmitted_packet_ = packets_[0]; + last_transmitted_packet_.transmitted_ms = clock_->GetTimeInMS(); packets_.erase(packets_.begin()); return seq_num; } + +bool TransmissionBucket::SameFrameAndPacketIntervalTimeElapsed( + const Packet& current_packet) { + if (last_transmitted_packet_.length == 0) { + // Not stored. + return false; + } + if (current_packet.timestamp != last_transmitted_packet_.timestamp) { + // Not same frame. + return false; + } + const int kPacketLimitMs = 5; + if ((clock_->GetTimeInMS() - last_transmitted_packet_.transmitted_ms) < + kPacketLimitMs) { + // Time has not elapsed. + return false; + } + return true; +} + +bool TransmissionBucket::NewFrameAndFrameIntervalTimeElapsed( + const Packet& current_packet) { + if (last_transmitted_packet_.length == 0) { + // Not stored. + return false; + } + if (current_packet.timestamp == last_transmitted_packet_.timestamp) { + // Not a new frame. + return false; + } + const float kFrameLimitFactor = 1.2f; + if ((clock_->GetTimeInMS() - last_transmitted_packet_.transmitted_ms) < + kFrameLimitFactor * + (current_packet.stored_ms - last_transmitted_packet_.stored_ms)) { + // Time has not elapsed. + return false; + } + return true; +} } // namespace webrtc diff --git a/src/modules/rtp_rtcp/source/transmission_bucket.h b/src/modules/rtp_rtcp/source/transmission_bucket.h index 79e45d8ca9..dd59f8a921 100644 --- a/src/modules/rtp_rtcp/source/transmission_bucket.h +++ b/src/modules/rtp_rtcp/source/transmission_bucket.h @@ -18,24 +18,25 @@ namespace webrtc { class CriticalSectionWrapper; +class RtpRtcpClock; class TransmissionBucket { public: - TransmissionBucket(); + TransmissionBucket(RtpRtcpClock* clock); ~TransmissionBucket(); // Resets members to initial state. void Reset(); // Adds packet to be sent. - void Fill(const uint16_t seq_num, const uint32_t num_bytes); + void Fill(uint16_t seq_num, uint32_t timestamp, uint16_t num_bytes); // Returns true if there is no packet to be sent. bool Empty(); // Updates the number of bytes that can be sent for the next time interval. - void UpdateBytesPerInterval(const uint32_t delta_time_in_ms, - const uint16_t target_bitrate_kbps); + void UpdateBytesPerInterval(uint32_t delta_time_in_ms, + uint16_t target_bitrate_kbps); // Checks if next packet in line can be transmitted. Returns the sequence // number of the packet on success, -1 otherwise. The packet is removed from @@ -43,21 +44,34 @@ class TransmissionBucket { int32_t GetNextPacket(); private: - struct Packet { - Packet(uint16_t sequence_number, uint16_t length_in_bytes) - : sequence_number_(sequence_number), - length_(length_in_bytes) { + struct Packet { + Packet(uint16_t seq_number, + uint32_t time_stamp, + uint16_t length_in_bytes, + int64_t now) + : sequence_number(seq_number), + timestamp(time_stamp), + length(length_in_bytes), + stored_ms(now), + transmitted_ms(0) { } - uint16_t sequence_number_; - uint16_t length_; + uint16_t sequence_number; + uint32_t timestamp; + uint16_t length; + int64_t stored_ms; + int64_t transmitted_ms; }; - CriticalSectionWrapper* critsect_; - uint32_t accumulator_; - int32_t bytes_rem_total_; - int32_t bytes_rem_interval_; - std::vector packets_; - bool first_; + bool SameFrameAndPacketIntervalTimeElapsed(const Packet& current_packet); + + bool NewFrameAndFrameIntervalTimeElapsed(const Packet& current_packet); + + RtpRtcpClock* clock_; + CriticalSectionWrapper* critsect_; + uint32_t accumulator_; + int32_t bytes_rem_interval_; + std::vector packets_; + Packet last_transmitted_packet_; }; } // namespace webrtc #endif // WEBRTC_MODULES_RTP_RTCP_TRANSMISSION_BUCKET_H_ diff --git a/src/modules/rtp_rtcp/source/transmission_bucket_unittest.cc b/src/modules/rtp_rtcp/source/transmission_bucket_unittest.cc index a8c9247615..32684972c2 100644 --- a/src/modules/rtp_rtcp/source/transmission_bucket_unittest.cc +++ b/src/modules/rtp_rtcp/source/transmission_bucket_unittest.cc @@ -14,51 +14,118 @@ #include +#include "rtp_rtcp_defines.h" #include "transmission_bucket.h" +#include "typedefs.h" namespace webrtc { +// TODO(asapersson): This class has been introduced in several test files. +// Break out into a unittest helper file. +class FakeClock : public RtpRtcpClock { + public: + FakeClock() { + time_in_ms_ = 123456; + } + // Return a timestamp in milliseconds relative to some arbitrary + // source; the source is fixed for this clock. + virtual WebRtc_Word64 GetTimeInMS() { + return time_in_ms_; + } + // Retrieve an NTP absolute timestamp. + virtual void CurrentNTP(WebRtc_UWord32& secs, WebRtc_UWord32& frac) { + secs = time_in_ms_ / 1000; + frac = (time_in_ms_ % 1000) * 4294967; + } + void IncrementTime(WebRtc_UWord32 time_increment_ms) { + time_in_ms_ += time_increment_ms; + } + private: + int64_t time_in_ms_; +}; + class TransmissionBucketTest : public ::testing::Test { - protected: - TransmissionBucket send_bucket_; + protected: + TransmissionBucketTest() + : send_bucket_(new TransmissionBucket(&fake_clock_)) { + } + ~TransmissionBucketTest() { + delete send_bucket_; + } + FakeClock fake_clock_; + TransmissionBucket* send_bucket_; }; TEST_F(TransmissionBucketTest, Fill) { - EXPECT_TRUE(send_bucket_.Empty()); - send_bucket_.Fill(1, 100); - EXPECT_FALSE(send_bucket_.Empty()); + EXPECT_TRUE(send_bucket_->Empty()); + send_bucket_->Fill(1, 3000, 100); + EXPECT_FALSE(send_bucket_->Empty()); } TEST_F(TransmissionBucketTest, Reset) { - send_bucket_.Fill(1, 100); - EXPECT_FALSE(send_bucket_.Empty()); - send_bucket_.Reset(); - EXPECT_TRUE(send_bucket_.Empty()); + send_bucket_->Fill(1, 3000, 100); + EXPECT_FALSE(send_bucket_->Empty()); + send_bucket_->Reset(); + EXPECT_TRUE(send_bucket_->Empty()); } TEST_F(TransmissionBucketTest, GetNextPacket) { - EXPECT_EQ(-1, send_bucket_.GetNextPacket()); // empty - send_bucket_.Fill(1234, 100); - EXPECT_EQ(1234, send_bucket_.GetNextPacket()); // first packet ok - send_bucket_.Fill(1235, 100); - EXPECT_EQ(-1, send_bucket_.GetNextPacket()); // packet does not fit + EXPECT_EQ(-1, send_bucket_->GetNextPacket()); // empty + + const int delta_time_ms = 1; + const int target_bitrate_kbps = 800; // 150 bytes per interval + send_bucket_->UpdateBytesPerInterval(delta_time_ms, target_bitrate_kbps); + + send_bucket_->Fill(1235, 3000, 75); + send_bucket_->Fill(1236, 3000, 75); + + EXPECT_EQ(1235, send_bucket_->GetNextPacket()); // ok + EXPECT_EQ(1236, send_bucket_->GetNextPacket()); // ok + EXPECT_TRUE(send_bucket_->Empty()); + + send_bucket_->Fill(1237, 3000, 75); + EXPECT_EQ(-1, send_bucket_->GetNextPacket()); // packet does not fit } -TEST_F(TransmissionBucketTest, UpdateBytesPerInterval) { +TEST_F(TransmissionBucketTest, SameFrameAndPacketIntervalTimeElapsed) { const int delta_time_ms = 1; - const int target_bitrate_kbps = 800; - send_bucket_.UpdateBytesPerInterval(delta_time_ms, target_bitrate_kbps); + const int target_bitrate_kbps = 800; // 150 bytes per interval + send_bucket_->UpdateBytesPerInterval(delta_time_ms, target_bitrate_kbps); - send_bucket_.Fill(1234, 50); - send_bucket_.Fill(1235, 50); - send_bucket_.Fill(1236, 50); + send_bucket_->Fill(1235, 3000, 75); + send_bucket_->Fill(1236, 3000, 75); - EXPECT_EQ(1234, send_bucket_.GetNextPacket()); // first packet ok - EXPECT_EQ(1235, send_bucket_.GetNextPacket()); // ok - EXPECT_EQ(1236, send_bucket_.GetNextPacket()); // ok - EXPECT_TRUE(send_bucket_.Empty()); + EXPECT_EQ(1235, send_bucket_->GetNextPacket()); // ok + EXPECT_EQ(1236, send_bucket_->GetNextPacket()); // ok + EXPECT_TRUE(send_bucket_->Empty()); - send_bucket_.Fill(1237, 50); - EXPECT_EQ(-1, send_bucket_.GetNextPacket()); // packet does not fit + fake_clock_.IncrementTime(4); + send_bucket_->Fill(1237, 3000, 75); + EXPECT_EQ(-1, send_bucket_->GetNextPacket()); // packet does not fit + + // Time limit (5ms) elapsed. + fake_clock_.IncrementTime(1); + EXPECT_EQ(1237, send_bucket_->GetNextPacket()); +} + +TEST_F(TransmissionBucketTest, NewFrameAndFrameIntervalTimeElapsed) { + const int delta_time_ms = 1; + const int target_bitrate_kbps = 800; // 150 bytes per interval + send_bucket_->UpdateBytesPerInterval(delta_time_ms, target_bitrate_kbps); + + send_bucket_->Fill(1235, 3000, 75); + send_bucket_->Fill(1236, 3000, 75); + + EXPECT_EQ(1235, send_bucket_->GetNextPacket()); // ok + EXPECT_EQ(1236, send_bucket_->GetNextPacket()); // ok + EXPECT_TRUE(send_bucket_->Empty()); + + fake_clock_.IncrementTime(4); + send_bucket_->Fill(1237, 6000, 75); + EXPECT_EQ(-1, send_bucket_->GetNextPacket()); // packet does not fit + + // Time limit elapsed (4*1.2=4.8ms). + fake_clock_.IncrementTime(1); + EXPECT_EQ(1237, send_bucket_->GetNextPacket()); } } // namespace webrtc