From 8ca8a71de2ab16eaafd9c0e5aac87d28aab490ea Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Tue, 23 Apr 2013 16:48:32 +0000 Subject: [PATCH] Revert "Add a default RTT to CallStats and use different values for buffered/real-time mode." This reverts commit aae26db1da5803482b094357c546b8454ab1c26d. BUG=1613 TBR=mflodman@webrtc.org Review URL: https://webrtc-codereview.appspot.com/1327008 git-svn-id: http://webrtc.googlecode.com/svn/trunk@3890 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../modules/rtp_rtcp/source/rtcp_receiver.cc | 20 ++++++++++++- .../modules/rtp_rtcp/source/rtcp_receiver.h | 5 ++++ .../modules/rtp_rtcp/source/rtp_receiver.cc | 3 +- webrtc/modules/rtp_rtcp/source/rtp_receiver.h | 5 ---- .../modules/rtp_rtcp/source/rtp_rtcp_config.h | 1 - .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 13 +++++--- webrtc/video_engine/call_stats.cc | 18 +++++------ webrtc/video_engine/call_stats.h | 3 -- webrtc/video_engine/call_stats_unittest.cc | 30 ------------------- webrtc/video_engine/vie_defines.h | 2 -- webrtc/video_engine/vie_rtp_rtcp_impl.cc | 3 +- 11 files changed, 43 insertions(+), 60 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc index b39043af6d..3b9659a599 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -54,7 +54,8 @@ RTCPReceiver::RTCPReceiver(const int32_t id, Clock* clock, _receivedInfoMap(), _packetTimeOutMS(0), _lastReceivedRrMs(0), - _lastIncreasedSequenceNumberMs(0) { + _lastIncreasedSequenceNumberMs(0), + _rtt(0) { memset(&_remoteSenderInfo, 0, sizeof(_remoteSenderInfo)); WEBRTC_TRACE(kTraceMemory, kTraceRtpRtcp, id, "%s created", __FUNCTION__); } @@ -210,6 +211,23 @@ int32_t RTCPReceiver::RTT(const uint32_t remoteSSRC, return 0; } +uint16_t RTCPReceiver::RTT() const { + CriticalSectionScoped lock(_criticalSectionRTCPReceiver); + if (!_receivedReportBlockMap.empty()) { + return 0; + } + return _rtt; +} + +int RTCPReceiver::SetRTT(uint16_t rtt) { + CriticalSectionScoped lock(_criticalSectionRTCPReceiver); + if (!_receivedReportBlockMap.empty()) { + return -1; + } + _rtt = rtt; + return 0; +} + int32_t RTCPReceiver::NTP(uint32_t *ReceivedNTPsecs, uint32_t *ReceivedNTPfrac, diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h index d92318925a..0833b4a573 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h @@ -237,6 +237,11 @@ protected: // The time we last received an RTCP RR telling we have ssuccessfully // delivered RTP packet to the remote side. int64_t _lastIncreasedSequenceNumberMs; + + // Externally set RTT. This value can only be used if there are no valid + // RTT estimates. + uint16_t _rtt; + }; } // namespace webrtc #endif // WEBRTC_MODULES_RTP_RTCP_SOURCE_RTCP_RECEIVER_H_ diff --git a/webrtc/modules/rtp_rtcp/source/rtp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtp_receiver.cc index 2855a147e9..7a811659a5 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_receiver.cc @@ -95,8 +95,7 @@ RTPReceiver::RTPReceiver(const int32_t id, max_reordering_threshold_(kDefaultMaxReorderingThreshold), rtx_(false), ssrc_rtx_(0), - payload_type_rtx_(-1), - rtt_ms_(kInitialReceiveSideRtt) { + payload_type_rtx_(-1) { assert(incoming_audio_messages_callback && incoming_messages_callback && incoming_payload_callback); diff --git a/webrtc/modules/rtp_rtcp/source/rtp_receiver.h b/webrtc/modules/rtp_rtcp/source/rtp_receiver.h index 998a62d33d..eaaab79c68 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_receiver.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_receiver.h @@ -158,10 +158,6 @@ class RTPReceiver : public Bitrate { const uint16_t bytes, const bool old_packet); - void set_rtt_ms(uint32_t rtt_ms) { rtt_ms_ = rtt_ms; } - - uint32_t rtt_ms() const { return rtt_ms_; } - private: // Returns whether RED is configured with payload_type. bool REDPayloadType(const int8_t payload_type) const; @@ -241,7 +237,6 @@ class RTPReceiver : public Bitrate { bool rtx_; uint32_t ssrc_rtx_; int payload_type_rtx_; - uint32_t rtt_ms_; }; } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h index c42f1de057..408354056a 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h @@ -23,7 +23,6 @@ enum { NACK_BYTECOUNT_SIZE = 60}; // size of our NACK history enum { kSendSideNackListSizeSanity = 20000 }; enum { kDefaultMaxReorderingThreshold = 50 }; // In sequence numbers. enum { kRtcpMaxNackFields = 253 }; -enum { kInitialReceiveSideRtt = 100 }; enum { RTCP_INTERVAL_VIDEO_MS = 1000 }; enum { RTCP_INTERVAL_AUDIO_MS = 5000 }; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 96d186e13e..bda7f12186 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -1224,9 +1224,9 @@ int32_t ModuleRtpRtcpImpl::ResetRTT(const uint32_t remote_ssrc) { return rtcp_receiver_.ResetRTT(remote_ssrc); } -void ModuleRtpRtcpImpl::SetRtt(uint32_t rtt) { +void ModuleRtpRtcpImpl:: SetRtt(uint32_t rtt) { WEBRTC_TRACE(kTraceModuleCall, kTraceRtpRtcp, id_, "SetRtt(rtt: %u)", rtt); - rtp_receiver_->set_rtt_ms(rtt); + rtcp_receiver_.SetRTT(static_cast(rtt)); } // Reset RTP statistics. @@ -1545,8 +1545,13 @@ int32_t ModuleRtpRtcpImpl::SendNACK(const uint16_t* nack_list, id_, "SendNACK(size:%u)", size); - // 5 + RTT * 1.5. - int64_t wait_time = 5 + ((rtp_receiver_->rtt_ms() * 3) / 2); + uint16_t avg_rtt = 0; + rtcp_receiver_.RTT(rtp_receiver_->SSRC(), NULL, &avg_rtt, NULL, NULL); + + int64_t wait_time = 5 + ((avg_rtt * 3) >> 1); // 5 + RTT * 1.5. + if (wait_time == 5) { + wait_time = 100; // During startup we don't have an RTT. + } const int64_t now = clock_->TimeInMilliseconds(); const int64_t time_limit = now - wait_time; uint16_t nackLength = size; diff --git a/webrtc/video_engine/call_stats.cc b/webrtc/video_engine/call_stats.cc index 823c33602b..233d536ab5 100644 --- a/webrtc/video_engine/call_stats.cc +++ b/webrtc/video_engine/call_stats.cc @@ -23,7 +23,6 @@ namespace webrtc { const int kRttTimeoutMs = 1500; // Time interval for updating the observers. const int kUpdateIntervalMs = 1000; -const uint32_t kInitialRttMs = 200; class RtcpObserver : public RtcpRttObserver { public: @@ -43,9 +42,7 @@ class RtcpObserver : public RtcpRttObserver { CallStats::CallStats() : crit_(CriticalSectionWrapper::CreateCriticalSection()), rtcp_rtt_observer_(new RtcpObserver(this)), - last_process_time_(TickTime::MillisecondTimestamp()), - last_reported_rtt_(kInitialRttMs), - rtt_report_received_(false) { + last_process_time_(TickTime::MillisecondTimestamp()) { } CallStats::~CallStats() { @@ -76,13 +73,13 @@ int32_t CallStats::Process() { if (it->rtt > max_rtt) max_rtt = it->rtt; } - if (max_rtt > 0) { - last_reported_rtt_ = max_rtt; - } + // If there is a valid rtt, update all observers. - for (std::list::iterator it = observers_.begin(); - it != observers_.end(); ++it) { - (*it)->OnRttUpdate(last_reported_rtt_); + if (max_rtt > 0) { + for (std::list::iterator it = observers_.begin(); + it != observers_.end(); ++it) { + (*it)->OnRttUpdate(max_rtt); + } } last_process_time_ = time_now; return 0; @@ -117,7 +114,6 @@ void CallStats::OnRttUpdate(uint32_t rtt) { CriticalSectionScoped cs(crit_.get()); int64_t time_now = TickTime::MillisecondTimestamp(); reports_.push_back(RttTime(rtt, time_now)); - rtt_report_received_ = true; } } // namespace webrtc diff --git a/webrtc/video_engine/call_stats.h b/webrtc/video_engine/call_stats.h index 378e1ba15a..5fd93a7624 100644 --- a/webrtc/video_engine/call_stats.h +++ b/webrtc/video_engine/call_stats.h @@ -68,9 +68,6 @@ class CallStats : public Module { // Observers getting stats reports. std::list observers_; - uint32_t last_reported_rtt_; - bool rtt_report_received_; - DISALLOW_COPY_AND_ASSIGN(CallStats); }; diff --git a/webrtc/video_engine/call_stats_unittest.cc b/webrtc/video_engine/call_stats_unittest.cc index 83d1c82903..4c14699738 100644 --- a/webrtc/video_engine/call_stats_unittest.cc +++ b/webrtc/video_engine/call_stats_unittest.cc @@ -22,8 +22,6 @@ using ::testing::Return; namespace webrtc { -enum { kDefaultRttMs = 200 }; - class MockStatsObserver : public CallStatsObserver { public: MockStatsObserver() {} @@ -178,32 +176,4 @@ TEST_F(CallStatsTest, ChangeRtt) { call_stats_->DeregisterStatsObserver(&stats_observer); } -TEST_F(CallStatsTest, NoRttUpdates) { - MockStatsObserver stats_observer; - call_stats_->RegisterStatsObserver(&stats_observer); - - // Advance clock to be ready for an update. - TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer, OnRttUpdate(kDefaultRttMs)) - .Times(1); - call_stats_->Process(); - - RtcpRttObserver* rtcp_observer = call_stats_->rtcp_rtt_observer(); - const int kNewRtt = 50; - // Report an RTT and verify that it replaces the default. - rtcp_observer->OnRttUpdate(kNewRtt); - TickTime::AdvanceFakeClock(1000); - EXPECT_CALL(stats_observer, OnRttUpdate(kNewRtt)) - .Times(1); - call_stats_->Process(); - - TickTime::AdvanceFakeClock(1500); - // The last reported RTT should still be reported when all reports have - // timed out. - EXPECT_CALL(stats_observer, OnRttUpdate(kNewRtt)) - .Times(1); - call_stats_->Process(); - - call_stats_->DeregisterStatsObserver(&stats_observer); -} } // namespace webrtc diff --git a/webrtc/video_engine/vie_defines.h b/webrtc/video_engine/vie_defines.h index 1a3095dea5..189a0e96d1 100644 --- a/webrtc/video_engine/vie_defines.h +++ b/webrtc/video_engine/vie_defines.h @@ -76,8 +76,6 @@ enum { kViEDefaultRenderDelayMs = 10 }; // ViERTP_RTCP enum { kSendSidePacketHistorySize = 600 }; -enum { kDefaultBufferingRtt = 20 }; -enum { kDefaultRealtimeRtt = 200 }; // NACK enum { kMaxPacketAgeToNack = 450 }; // In sequence numbers. diff --git a/webrtc/video_engine/vie_rtp_rtcp_impl.cc b/webrtc/video_engine/vie_rtp_rtcp_impl.cc index 32dcf0c225..4233894dff 100644 --- a/webrtc/video_engine/vie_rtp_rtcp_impl.cc +++ b/webrtc/video_engine/vie_rtp_rtcp_impl.cc @@ -631,7 +631,7 @@ int ViERTP_RTCPImpl::SetSenderBufferingMode(int video_channel, } int ViERTP_RTCPImpl::SetReceiverBufferingMode(int video_channel, - int target_delay_ms) { + int target_delay_ms) { WEBRTC_TRACE(kTraceApiCall, kTraceVideo, ViEId(shared_data_->instance_id(), video_channel), "%s(channel: %d, receiver target_delay: %d)", @@ -645,6 +645,7 @@ int ViERTP_RTCPImpl::SetReceiverBufferingMode(int video_channel, shared_data_->SetLastError(kViERtpRtcpInvalidChannelId); return -1; } + // Update the channel with buffering mode settings. if (vie_channel->SetReceiverBufferingMode(target_delay_ms) != 0) { WEBRTC_TRACE(kTraceError, kTraceVideo,