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

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;
 

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;

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;
   }

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,

src/modules/rtp_rtcp/source/transmission_bucket.cc

@@ -12,16 +12,17 @@
 
 #include <assert.h>
 #include "critical_section_wrapper.h"
+#include "rtp_utility.h"
 
 namespace webrtc {
 
-TransmissionBucket::TransmissionBucket()
+TransmissionBucket::TransmissionBucket(RtpRtcpClock* clock)
-  : critsect_(CriticalSectionWrapper::CreateCriticalSection()),
+  : 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,
+void TransmissionBucket::Fill(uint16_t seq_num,
-                              const uint32_t num_bytes) {
+                              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,
+    uint32_t delta_time_ms,
-    const uint16_t target_bitrate_kbps) {
+    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<Packet>::const_iterator it_begin = packets_.begin();
-  const uint16_t num_bytes = (*it_begin).length_;
+  const uint16_t num_bytes = (*it_begin).length;
-  const uint16_t seq_num = (*it_begin).sequence_number_;
+  const uint16_t seq_num = (*it_begin).sequence_number;
 
-  if (first_) {
+  if (bytes_rem_interval_ <= 0 &&
-    // Ok to transmit first packet.
+      !SameFrameAndPacketIntervalTimeElapsed(*it_begin) &&
-    first_ = false;
+      !NewFrameAndFrameIntervalTimeElapsed(*it_begin)) {
-    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) {
     // 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

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,
+  void UpdateBytesPerInterval(uint32_t delta_time_in_ms,
-                              const uint16_t target_bitrate_kbps);
+                              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 {
+  struct Packet {
-     Packet(uint16_t sequence_number, uint16_t length_in_bytes)
+     Packet(uint16_t seq_number,
-       : sequence_number_(sequence_number),
+            uint32_t time_stamp,
-         length_(length_in_bytes) {
+            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 sequence_number;
-     uint16_t length_;
+     uint32_t timestamp;
+     uint16_t length;
+     int64_t stored_ms;
+     int64_t transmitted_ms;
    };
 
-   CriticalSectionWrapper* critsect_;
+  bool SameFrameAndPacketIntervalTimeElapsed(const Packet& current_packet);
-   uint32_t accumulator_;
+
-   int32_t bytes_rem_total_;
+  bool NewFrameAndFrameIntervalTimeElapsed(const Packet& current_packet);
-   int32_t bytes_rem_interval_;
+
-   std::vector<Packet> packets_;
+  RtpRtcpClock* clock_;
-   bool first_;
+  CriticalSectionWrapper* critsect_;
+  uint32_t accumulator_;
+  int32_t bytes_rem_interval_;
+  std::vector<Packet> packets_;
+  Packet last_transmitted_packet_;
 };
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_RTP_RTCP_TRANSMISSION_BUCKET_H_

src/modules/rtp_rtcp/source/transmission_bucket_unittest.cc

@@ -14,51 +14,118 @@
 
 #include <gtest/gtest.h>
 
+#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:  
+ protected:
-  TransmissionBucket send_bucket_;
+  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());
+  EXPECT_TRUE(send_bucket_->Empty());
-  send_bucket_.Fill(1, 100);
+  send_bucket_->Fill(1, 3000, 100);
-  EXPECT_FALSE(send_bucket_.Empty());
+  EXPECT_FALSE(send_bucket_->Empty());
 }
 
 TEST_F(TransmissionBucketTest, Reset) {
-  send_bucket_.Fill(1, 100);
+  send_bucket_->Fill(1, 3000, 100);
-  EXPECT_FALSE(send_bucket_.Empty());
+  EXPECT_FALSE(send_bucket_->Empty());
-  send_bucket_.Reset();
+  send_bucket_->Reset();
-  EXPECT_TRUE(send_bucket_.Empty());
+  EXPECT_TRUE(send_bucket_->Empty());
 }
 
 TEST_F(TransmissionBucketTest, GetNextPacket) {
-  EXPECT_EQ(-1, send_bucket_.GetNextPacket());    // empty
+  EXPECT_EQ(-1, send_bucket_->GetNextPacket());    // empty
-  send_bucket_.Fill(1234, 100);
+
-  EXPECT_EQ(1234, send_bucket_.GetNextPacket());  // first packet ok
+  const int delta_time_ms = 1;
-  send_bucket_.Fill(1235, 100);
+  const int target_bitrate_kbps = 800;  // 150 bytes per interval
-  EXPECT_EQ(-1, send_bucket_.GetNextPacket());    // packet does not fit
+  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;
+  const int target_bitrate_kbps = 800;  // 150 bytes per interval
-  send_bucket_.UpdateBytesPerInterval(delta_time_ms, target_bitrate_kbps);
+  send_bucket_->UpdateBytesPerInterval(delta_time_ms, target_bitrate_kbps);
 
-  send_bucket_.Fill(1234, 50);
+  send_bucket_->Fill(1235, 3000, 75);
-  send_bucket_.Fill(1235, 50);
+  send_bucket_->Fill(1236, 3000, 75);
-  send_bucket_.Fill(1236, 50);
 
-  EXPECT_EQ(1234, send_bucket_.GetNextPacket());  // first packet ok
+  EXPECT_EQ(1235, send_bucket_->GetNextPacket());  // ok
-  EXPECT_EQ(1235, send_bucket_.GetNextPacket());  // ok
+  EXPECT_EQ(1236, send_bucket_->GetNextPacket());  // ok
-  EXPECT_EQ(1236, send_bucket_.GetNextPacket());  // ok
+  EXPECT_TRUE(send_bucket_->Empty());
-  EXPECT_TRUE(send_bucket_.Empty());
 
-  send_bucket_.Fill(1237, 50);
+  fake_clock_.IncrementTime(4);
-  EXPECT_EQ(-1, send_bucket_.GetNextPacket());    // packet does not fit
+  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