From 5779ca478dd93193663b25a624749fe31e83de7a Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Tue, 1 Jul 2014 12:14:49 +0000 Subject: [PATCH] Fixes a potential BWE clock mismatch bug. Since libjingle provides a packet arrival timestamp to webrtc, and the clock in remote bitrate estimator and the clock used for packet arrival timestamp can be different. This can cause the bandwidth estimator to malfunction. This CL changes the remote bitrate estimator so that packet arrival timestamps never are compared to the time taken from the internal clock. BUG=3527 R=mflodman@webrtc.org, pbos@webrtc.org Review URL: https://webrtc-codereview.appspot.com/20789004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6571 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../include/remote_bitrate_estimator.h | 1 + .../overuse_detector.cc | 12 +--- .../overuse_detector.h | 4 +- .../remote_bitrate_estimator_single_stream.cc | 65 ++++++++++++------- ...emote_bitrate_estimator_unittest_helper.cc | 9 ++- ...remote_bitrate_estimator_unittest_helper.h | 1 + 6 files changed, 54 insertions(+), 38 deletions(-) diff --git a/webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h b/webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h index 7dc4f270f7..e61e903558 100644 --- a/webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h +++ b/webrtc/modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h @@ -71,6 +71,7 @@ class RemoteBitrateEstimator : public CallStatsObserver, public Module { // estimate and the over-use detector. If an over-use is detected the // remote bitrate estimate will be updated. Note that |payload_size| is the // packet size excluding headers. + // Note that |arrival_time_ms| can be of an arbitrary time base. virtual void IncomingPacket(int64_t arrival_time_ms, int payload_size, const RTPHeader& header) = 0; diff --git a/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc b/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc index 56a6baa827..9baaa9c913 100644 --- a/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc +++ b/webrtc/modules/remote_bitrate_estimator/overuse_detector.cc @@ -36,8 +36,7 @@ OveruseDetector::OveruseDetector(const OverUseDetectorOptions& options) prev_offset_(0.0), time_over_using_(-1), over_use_counter_(0), - hypothesis_(kBwNormal), - time_of_last_received_packet_(-1) { + hypothesis_(kBwNormal) { memcpy(E_, options_.initial_e, sizeof(E_)); memcpy(process_noise_, options_.initial_process_noise, sizeof(process_noise_)); @@ -50,8 +49,7 @@ OveruseDetector::~OveruseDetector() { void OveruseDetector::Update(uint16_t packet_size, int64_t timestamp_ms, uint32_t timestamp, - const int64_t now_ms) { - time_of_last_received_packet_ = now_ms; + const int64_t arrival_time_ms) { bool new_timestamp = (timestamp != current_frame_.timestamp); if (timestamp_ms >= 0) { if (prev_frame_.timestamp_ms == -1 && current_frame_.timestamp_ms == -1) { @@ -82,7 +80,7 @@ void OveruseDetector::Update(uint16_t packet_size, } // Accumulate the frame size current_frame_.size += packet_size; - current_frame_.complete_time_ms = now_ms; + current_frame_.complete_time_ms = arrival_time_ms; } BandwidthUsage OveruseDetector::State() const { @@ -107,10 +105,6 @@ void OveruseDetector::SetRateControlRegion(RateControlRegion region) { } } -int64_t OveruseDetector::time_of_last_received_packet() const { - return time_of_last_received_packet_; -} - void OveruseDetector::SwitchTimeBase() { current_frame_.size = 0; current_frame_.complete_time_ms = -1; diff --git a/webrtc/modules/remote_bitrate_estimator/overuse_detector.h b/webrtc/modules/remote_bitrate_estimator/overuse_detector.h index a7e59cc6b3..9c565e45f1 100644 --- a/webrtc/modules/remote_bitrate_estimator/overuse_detector.h +++ b/webrtc/modules/remote_bitrate_estimator/overuse_detector.h @@ -28,11 +28,10 @@ class OveruseDetector { void Update(uint16_t packet_size, int64_t timestamp_ms, uint32_t rtp_timestamp, - int64_t now_ms); + int64_t arrival_time_ms); BandwidthUsage State() const; double NoiseVar() const; void SetRateControlRegion(RateControlRegion region); - int64_t time_of_last_received_packet() const; private: struct FrameSample { @@ -89,7 +88,6 @@ class OveruseDetector { double time_over_using_; uint16_t over_use_counter_; BandwidthUsage hypothesis_; - int64_t time_of_last_received_packet_; }; } // namespace webrtc diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc index 577912eb11..e825b51fc1 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc @@ -59,10 +59,27 @@ class RemoteBitrateEstimatorSingleStream : public RemoteBitrateEstimator { ReceiveBandwidthEstimatorStats* output) const OVERRIDE; private: - typedef std::map SsrcOveruseDetectorMap; + // Map from SSRC to over-use detector and last incoming packet time in + // milliseconds, taken from clock_. + typedef std::map > + SsrcOveruseDetectorMap; + + static OveruseDetector* GetDetector( + const SsrcOveruseDetectorMap::iterator it) { + return &it->second.first; + } + + static int64_t GetPacketTimeMs(const SsrcOveruseDetectorMap::iterator it) { + return it->second.second; + } + + static void SetPacketTimeMs(SsrcOveruseDetectorMap::iterator it, + int64_t time_ms) { + it->second.second = time_ms; + } // Triggers a new estimate calculation. - void UpdateEstimate(int64_t time_now); + void UpdateEstimate(int64_t now_ms); void GetSsrcs(std::vector* ssrcs) const; @@ -95,6 +112,7 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( uint32_t ssrc = header.ssrc; uint32_t rtp_timestamp = header.timestamp + header.extension.transmissionTimeOffset; + int64_t now_ms = clock_->TimeInMilliseconds(); CriticalSectionScoped cs(crit_sect_.get()); SsrcOveruseDetectorMap::iterator it = overuse_detectors_.find(ssrc); if (it == overuse_detectors_.end()) { @@ -105,22 +123,23 @@ void RemoteBitrateEstimatorSingleStream::IncomingPacket( // automatically cleaned up when we have one RemoteBitrateEstimator per REMB // group. std::pair insert_result = - overuse_detectors_.insert(std::make_pair(ssrc, OveruseDetector( - OverUseDetectorOptions()))); + overuse_detectors_.insert(std::make_pair(ssrc, + std::make_pair(OveruseDetector(OverUseDetectorOptions()), now_ms))); it = insert_result.first; } - OveruseDetector* overuse_detector = &it->second; - incoming_bitrate_.Update(payload_size, arrival_time_ms); + SetPacketTimeMs(it, now_ms); + OveruseDetector* overuse_detector = GetDetector(it); + incoming_bitrate_.Update(payload_size, now_ms); const BandwidthUsage prior_state = overuse_detector->State(); overuse_detector->Update(payload_size, -1, rtp_timestamp, arrival_time_ms); if (overuse_detector->State() == kBwOverusing) { - unsigned int incoming_bitrate = incoming_bitrate_.Rate(arrival_time_ms); + unsigned int incoming_bitrate = incoming_bitrate_.Rate(now_ms); if (prior_state != kBwOverusing || - remote_rate_.TimeToReduceFurther(arrival_time_ms, incoming_bitrate)) { + remote_rate_.TimeToReduceFurther(now_ms, incoming_bitrate)) { // The first overuse should immediately trigger a new estimate. // We also have to update the estimate immediately if we are overusing // and the target bitrate is too high compared to what we are receiving. - UpdateEstimate(arrival_time_ms); + UpdateEstimate(now_ms); } } } @@ -129,8 +148,9 @@ int32_t RemoteBitrateEstimatorSingleStream::Process() { if (TimeUntilNextProcess() > 0) { return 0; } - UpdateEstimate(clock_->TimeInMilliseconds()); - last_process_time_ = clock_->TimeInMilliseconds(); + int64_t now_ms = clock_->TimeInMilliseconds(); + UpdateEstimate(now_ms); + last_process_time_ = now_ms; return 0; } @@ -141,25 +161,24 @@ int32_t RemoteBitrateEstimatorSingleStream::TimeUntilNextProcess() { return last_process_time_ + kProcessIntervalMs - clock_->TimeInMilliseconds(); } -void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t time_now) { +void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t now_ms) { CriticalSectionScoped cs(crit_sect_.get()); BandwidthUsage bw_state = kBwNormal; double sum_noise_var = 0.0; SsrcOveruseDetectorMap::iterator it = overuse_detectors_.begin(); while (it != overuse_detectors_.end()) { - const int64_t time_of_last_received_packet = - it->second.time_of_last_received_packet(); - if (time_of_last_received_packet >= 0 && - time_now - time_of_last_received_packet > kStreamTimeOutMs) { + if (GetPacketTimeMs(it) >= 0 && + now_ms - GetPacketTimeMs(it) > kStreamTimeOutMs) { // This over-use detector hasn't received packets for |kStreamTimeOutMs| // milliseconds and is considered stale. overuse_detectors_.erase(it++); } else { - sum_noise_var += it->second.NoiseVar(); + OveruseDetector* overuse_detector = GetDetector(it); + sum_noise_var += overuse_detector->NoiseVar(); // Make sure that we trigger an over-use if any of the over-use detectors // is detecting over-use. - if (it->second.State() > bw_state) { - bw_state = it->second.State(); + if (overuse_detector->State() > bw_state) { + bw_state = overuse_detector->State(); } ++it; } @@ -172,17 +191,17 @@ void RemoteBitrateEstimatorSingleStream::UpdateEstimate(int64_t time_now) { double mean_noise_var = sum_noise_var / static_cast(overuse_detectors_.size()); const RateControlInput input(bw_state, - incoming_bitrate_.Rate(time_now), + incoming_bitrate_.Rate(now_ms), mean_noise_var); - const RateControlRegion region = remote_rate_.Update(&input, time_now); - unsigned int target_bitrate = remote_rate_.UpdateBandwidthEstimate(time_now); + const RateControlRegion region = remote_rate_.Update(&input, now_ms); + unsigned int target_bitrate = remote_rate_.UpdateBandwidthEstimate(now_ms); if (remote_rate_.ValidEstimate()) { std::vector ssrcs; GetSsrcs(&ssrcs); observer_->OnReceiveBitrateChanged(ssrcs, target_bitrate); } for (it = overuse_detectors_.begin(); it != overuse_detectors_.end(); ++it) { - it->second.SetRateControlRegion(region); + GetDetector(it).SetRateControlRegion(region); } } diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc index dc30d9334b..1b38a1ea30 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc @@ -226,7 +226,8 @@ void RemoteBitrateEstimatorTest::IncomingPacket(uint32_t ssrc, header.ssrc = ssrc; header.timestamp = rtp_timestamp; header.extension.absoluteSendTime = absolute_send_time; - bitrate_estimator_->IncomingPacket(arrival_time, payload_size, header); + bitrate_estimator_->IncomingPacket(arrival_time + kArrivalTimeClockOffsetMs, + payload_size, header); } // Generates a frame of packets belonging to a stream at a given bitrate and @@ -245,6 +246,10 @@ bool RemoteBitrateEstimatorTest::GenerateAndProcessFrame(unsigned int ssrc, while (!packets.empty()) { testing::RtpStream::RtpPacket* packet = packets.front(); bitrate_observer_->Reset(); + // The simulated clock should match the time of packet->arrival_time + // since both are used in IncomingPacket(). + clock_.AdvanceTimeMicroseconds(packet->arrival_time - + clock_.TimeInMicroseconds()); IncomingPacket(packet->ssrc, packet->size, (packet->arrival_time + 500) / 1000, @@ -256,8 +261,6 @@ bool RemoteBitrateEstimatorTest::GenerateAndProcessFrame(unsigned int ssrc, overuse = true; EXPECT_LE(bitrate_observer_->latest_bitrate(), bitrate_bps); } - clock_.AdvanceTimeMicroseconds(packet->arrival_time - - clock_.TimeInMicroseconds()); delete packet; packets.pop_front(); } diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.h b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.h index 14cfc31cea..1d748c57b9 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.h +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.h @@ -198,6 +198,7 @@ class RemoteBitrateEstimatorTest : public ::testing::Test { unsigned int expected_bitrate_drop_delta); static const unsigned int kDefaultSsrc; + static const int kArrivalTimeClockOffsetMs = 60000; SimulatedClock clock_; // Time at the receiver. scoped_ptr bitrate_observer_;