From 6dcef360f8564b2d291e08c4d727e7a595747da6 Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Mon, 29 Oct 2012 16:06:08 +0000 Subject: [PATCH] Follow-up CL for r3007. Fixes an initialization issue introduced with r3007 and adds a test for detecting problems with reordering. Also some cleanup for better code. TEST=remote_bitrate_estimator_unittests BUG=1009 Review URL: https://webrtc-codereview.appspot.com/937007 git-svn-id: http://webrtc.googlecode.com/svn/trunk@3014 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../overuse_detector.cc | 82 +++++++------------ .../overuse_detector.h | 10 ++- .../remote_bitrate_estimator_unittest.cc | 41 ++++++++-- 3 files changed, 72 insertions(+), 61 deletions(-) diff --git a/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc b/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc index 433a552288..dfd6b906a1 100644 --- a/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc +++ b/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc @@ -110,14 +110,6 @@ void OveruseDetector::Update(uint16_t packet_size, #endif bool new_timestamp = (timestamp != current_frame_.timestamp); - if (current_frame_.timestamp_ms == -1) { - const uint32_t timestamp_diff = timestamp - current_frame_.timestamp; - if (timestamp_diff > 0x80000000) { - // Assume that a diff this big must be due to reordering. Don't update - // with reordered samples. - return; - } - } if (timestamp_ms >= 0) { if (prev_frame_.timestamp_ms == -1 && current_frame_.timestamp_ms == -1) { SwitchTimeBase(); @@ -125,24 +117,23 @@ void OveruseDetector::Update(uint16_t packet_size, new_timestamp = (timestamp_ms != current_frame_.timestamp_ms); } if (current_frame_.timestamp == -1) { + // This is the first incoming packet. We don't have enough data to update + // the filter, so we store it until we have two frames of data to process. current_frame_.timestamp = timestamp; current_frame_.timestamp_ms = timestamp_ms; + } else if (!PacketInOrder(timestamp, timestamp_ms)) { + return; } else if (new_timestamp) { - // First packet of a later frame, the previous frame sample is ready - WEBRTC_TRACE(kTraceStream, kTraceRtpRtcp, -1, - "Frame complete at %I64i", current_frame_.complete_time_ms); - if (prev_frame_.complete_time_ms >= 0) { // This is our second frame + // First packet of a later frame, the previous frame sample is ready. + WEBRTC_TRACE(kTraceStream, kTraceRtpRtcp, -1, "Frame complete at %I64i", + current_frame_.complete_time_ms); + if (prev_frame_.complete_time_ms >= 0) { // This is our second frame. int64_t t_delta = 0; double ts_delta = 0; - if (TimeDeltas(current_frame_, prev_frame_, - &t_delta, &ts_delta)) { - // No reordering. - UpdateKalman(t_delta, ts_delta, current_frame_.size, prev_frame_.size); - prev_frame_ = current_frame_; - } - } else { - prev_frame_ = current_frame_; + TimeDeltas(current_frame_, prev_frame_, &t_delta, &ts_delta); + UpdateKalman(t_delta, ts_delta, current_frame_.size, prev_frame_.size); } + prev_frame_ = current_frame_; // The new timestamp is now the current frame. current_frame_.timestamp = timestamp; current_frame_.timestamp_ms = timestamp_ms; @@ -182,7 +173,7 @@ void OveruseDetector::SwitchTimeBase() { prev_frame_ = current_frame_; } -bool OveruseDetector::TimeDeltas(const FrameSample& current_frame, +void OveruseDetector::TimeDeltas(const FrameSample& current_frame, const FrameSample& prev_frame, int64_t* t_delta, double* ts_delta) { @@ -193,26 +184,34 @@ bool OveruseDetector::TimeDeltas(const FrameSample& current_frame, num_of_deltas_ = 1000; } if (current_frame.timestamp_ms == -1) { - const uint32_t timestamp_diff = current_frame.timestamp - - prev_frame.timestamp; - if (timestamp_diff > 0x80000000) { - // Assume that a diff this big must be due to reordering. Don't update - // with reordered samples. - return false; - } + uint32_t timestamp_diff = current_frame.timestamp - prev_frame.timestamp; *ts_delta = timestamp_diff / 90.0; } else { *ts_delta = current_frame.timestamp_ms - prev_frame.timestamp_ms; - if (*ts_delta < 0) { - // Frame reordering. - return false; - } } *t_delta = current_frame.complete_time_ms - prev_frame.complete_time_ms; assert(*ts_delta > 0); +} + +bool OveruseDetector::PacketInOrder(uint32_t timestamp, int64_t timestamp_ms) { + if (current_frame_.timestamp_ms == -1 && current_frame_.timestamp > -1) { + return InOrderTimestamp(timestamp, current_frame_.timestamp); + } else if (current_frame_.timestamp_ms > 0) { + // Using timestamps converted to NTP time. + return timestamp_ms > current_frame_.timestamp_ms; + } + // This is the first packet. return true; } +bool OveruseDetector::InOrderTimestamp(uint32_t timestamp, + uint32_t prev_timestamp) { + uint32_t timestamp_diff = timestamp - prev_timestamp; + // Assume that a diff this big must be due to reordering. Don't update + // with reordered samples. + return (timestamp_diff < 0x80000000); +} + double OveruseDetector::CurrentDrift() { return 1.0; } @@ -406,23 +405,4 @@ BandwidthUsage OveruseDetector::Detect(double ts_delta) { } return hypothesis_; } - -bool OveruseDetector::OldTimestamp(uint32_t new_timestamp, - uint32_t existing_timestamp, - bool* wrapped) { - bool tmpWrapped = - (new_timestamp < 0x0000ffff && existing_timestamp > 0xffff0000) || - (new_timestamp > 0xffff0000 && existing_timestamp < 0x0000ffff); - *wrapped = tmpWrapped; - if (existing_timestamp > new_timestamp && !tmpWrapped) { - return true; - } else if (existing_timestamp <= new_timestamp && !tmpWrapped) { - return false; - } else if (existing_timestamp < new_timestamp && tmpWrapped) { - return true; - } else { - return false; - } -} - } // namespace webrtc diff --git a/webrtc/modules/remote_bitrate_estimator/overuse_detector.h b/webrtc/modules/remote_bitrate_estimator/overuse_detector.h index 67a33508dd..67b998dd8c 100644 --- a/webrtc/modules/remote_bitrate_estimator/overuse_detector.h +++ b/webrtc/modules/remote_bitrate_estimator/overuse_detector.h @@ -59,15 +59,17 @@ class OveruseDetector { #endif }; - static bool OldTimestamp(uint32_t new_timestamp, - uint32_t existing_timestamp, - bool* wrapped); + // Returns true if |timestamp| represent a time which is later than + // |prev_timestamp|. + static bool InOrderTimestamp(uint32_t timestamp, uint32_t prev_timestamp); + + bool PacketInOrder(uint32_t timestamp, int64_t timestamp_ms); // Prepares the overuse detector to start using timestamps in milliseconds // instead of 90 kHz timestamps. void SwitchTimeBase(); - bool TimeDeltas(const FrameSample& current_frame, + void TimeDeltas(const FrameSample& current_frame, const FrameSample& prev_frame, int64_t* t_delta, double* ts_delta); diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest.cc index cdc0a466e1..03c7d60254 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest.cc @@ -277,7 +277,7 @@ class RemoteBitrateEstimatorTest : public ::testing::Test { RemoteBitrateEstimator::Create( bitrate_observer_.get(), over_use_detector_options_, - RemoteBitrateEstimator::kMultiStreamEstimation)); + RemoteBitrateEstimator::kSingleStreamEstimation)); stream_generator_.reset(new StreamGenerator(1e6, // Capacity. time_now_)); } @@ -415,12 +415,41 @@ TEST_F(RemoteBitrateEstimatorTest, TestInitialBehavior) { time_now += 2; bitrate_estimator_->UpdateEstimate(kDefaultSsrc, time_now); EXPECT_TRUE(bitrate_estimator_->LatestEstimate(kDefaultSsrc, &bitrate_bps)); - EXPECT_EQ(20644u, bitrate_bps); + EXPECT_EQ(20607u, bitrate_bps); EXPECT_TRUE(bitrate_observer_->updated()); bitrate_observer_->Reset(); EXPECT_EQ(bitrate_observer_->latest_bitrate(), bitrate_bps); } +TEST_F(RemoteBitrateEstimatorTest, TestRateIncreaseReordering) { + int64_t time_now = 0; + uint32_t timestamp = 0; + const int framerate = 50; // 50 fps to avoid rounding errors. + const int frame_interval_ms = 1000 / framerate; + bitrate_estimator_->IncomingPacket(kDefaultSsrc, 1000, time_now, timestamp); + bitrate_estimator_->UpdateEstimate(kDefaultSsrc, time_now); + EXPECT_FALSE(bitrate_observer_->updated()); // No valid estimate. + // Increase time with 1 second to get a valid estimate. + time_now += 1000; + timestamp += 90 * 1000; + bitrate_estimator_->IncomingPacket(kDefaultSsrc, 1000, time_now, timestamp); + bitrate_estimator_->UpdateEstimate(kDefaultSsrc, time_now); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_EQ(17645u, bitrate_observer_->latest_bitrate()); + for (int i = 0; i < 10; ++i) { + time_now += 2 * frame_interval_ms; + timestamp += 2 * 90 * frame_interval_ms; + bitrate_estimator_->IncomingPacket(kDefaultSsrc, 1000, time_now, timestamp); + bitrate_estimator_->IncomingPacket(kDefaultSsrc, + 1000, + time_now - frame_interval_ms, + timestamp - 90 * frame_interval_ms); + } + bitrate_estimator_->UpdateEstimate(kDefaultSsrc, time_now); + EXPECT_TRUE(bitrate_observer_->updated()); + EXPECT_EQ(18985u, bitrate_observer_->latest_bitrate()); +} + // Make sure we initially increase the bitrate as expected. TEST_F(RemoteBitrateEstimatorTest, TestRateIncreaseRtpTimestamps) { const int kExpectedIterations = 276; @@ -512,7 +541,7 @@ TEST_F(RemoteBitrateEstimatorTest, TestCapacityDropRtpTimestampsWrap) { bitrate_observer_->Reset(); } } - EXPECT_EQ(8366, bitrate_drop_time); + EXPECT_EQ(8299, bitrate_drop_time); } // Verify that the time it takes for the estimator to reduce the bitrate when @@ -552,7 +581,7 @@ TEST_F(RemoteBitrateEstimatorTestAlign, TestCapacityDropRtpTimestampsWrap) { bitrate_observer_->Reset(); } } - EXPECT_EQ(8366, bitrate_drop_time); + EXPECT_EQ(8299, bitrate_drop_time); } // Verify that the time it takes for the estimator to reduce the bitrate when @@ -605,7 +634,7 @@ TEST_F(RemoteBitrateEstimatorTestAlign, TwoStreamsCapacityDropWithWrap) { bitrate_observer_->Reset(); } } - EXPECT_EQ(4966, bitrate_drop_time); + EXPECT_EQ(4933, bitrate_drop_time); } // Verify that the time it takes for the estimator to reduce the bitrate when @@ -666,7 +695,7 @@ TEST_F(RemoteBitrateEstimatorTestAlign, ThreeStreams) { bitrate_observer_->Reset(); } } - EXPECT_EQ(3900, bitrate_drop_time); + EXPECT_EQ(3966, bitrate_drop_time); } } // namespace webrtc