diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h index a0dcd85efe..9ed9cf8ed8 100644 --- a/modules/rtp_rtcp/include/receive_statistics.h +++ b/modules/rtp_rtcp/include/receive_statistics.h @@ -61,9 +61,14 @@ class ReceiveStatistics : public ReceiveStatisticsProvider, ~ReceiveStatistics() override = default; static ReceiveStatistics* Create(Clock* clock) { - return Create(clock, nullptr, nullptr).release(); + return Create(clock, nullptr).release(); } + static std::unique_ptr Create( + Clock* clock, + StreamDataCountersCallback* rtp_callback); + + RTC_DEPRECATED static std::unique_ptr Create( Clock* clock, RtcpStatisticsCallback* rtcp_callback, diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index 45be4d1d39..1302dac969 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -34,7 +34,6 @@ StreamStatisticianImpl::StreamStatisticianImpl( uint32_t ssrc, Clock* clock, int max_reordering_threshold, - RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback) : ssrc_(ssrc), clock_(clock), @@ -51,7 +50,6 @@ StreamStatisticianImpl::StreamStatisticianImpl( last_report_inorder_packets_(0), last_report_old_packets_(0), last_report_seq_max_(-1), - rtcp_callback_(rtcp_callback), rtp_callback_(rtp_callback) {} StreamStatisticianImpl::~StreamStatisticianImpl() = default; @@ -181,48 +179,40 @@ void StreamStatisticianImpl::EnableRetransmitDetection(bool enable) { bool StreamStatisticianImpl::GetStatistics(RtcpStatistics* statistics, bool reset) { - { - rtc::CritScope cs(&stream_lock_); - if (!ReceivedRtpPacket()) { - return false; - } - - if (!reset) { - if (last_report_inorder_packets_ == 0) { - // No report. - return false; - } - // Just get last report. - *statistics = last_reported_statistics_; - return true; - } - - *statistics = CalculateRtcpStatistics(); + rtc::CritScope cs(&stream_lock_); + if (!ReceivedRtpPacket()) { + return false; } - if (rtcp_callback_) - rtcp_callback_->StatisticsUpdated(*statistics, ssrc_); + if (!reset) { + if (last_report_inorder_packets_ == 0) { + // No report. + return false; + } + // Just get last report. + *statistics = last_reported_statistics_; + return true; + } + + *statistics = CalculateRtcpStatistics(); + return true; } bool StreamStatisticianImpl::GetActiveStatisticsAndReset( RtcpStatistics* statistics) { - { - rtc::CritScope cs(&stream_lock_); - if (clock_->TimeInMilliseconds() - last_receive_time_ms_ >= - kStatisticsTimeoutMs) { - // Not active. - return false; - } - if (!ReceivedRtpPacket()) { - return false; - } - - *statistics = CalculateRtcpStatistics(); + rtc::CritScope cs(&stream_lock_); + if (clock_->TimeInMilliseconds() - last_receive_time_ms_ >= + kStatisticsTimeoutMs) { + // Not active. + return false; + } + if (!ReceivedRtpPacket()) { + return false; } - if (rtcp_callback_) - rtcp_callback_->StatisticsUpdated(*statistics, ssrc_); + *statistics = CalculateRtcpStatistics(); + return true; } @@ -355,22 +345,26 @@ bool StreamStatisticianImpl::IsRetransmitOfOldPacket( return time_diff_ms > rtp_time_stamp_diff_ms + max_delay_ms; } +std::unique_ptr ReceiveStatistics::Create( + Clock* clock, + StreamDataCountersCallback* rtp_callback) { + return absl::make_unique(clock, rtp_callback); +} + std::unique_ptr ReceiveStatistics::Create( Clock* clock, RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback) { - return absl::make_unique(clock, rtcp_callback, - rtp_callback); + RTC_CHECK(rtcp_callback == nullptr); + return Create(clock, rtp_callback); } ReceiveStatisticsImpl::ReceiveStatisticsImpl( Clock* clock, - RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback) : clock_(clock), last_returned_ssrc_(0), max_reordering_threshold_(kDefaultMaxReorderingThreshold), - rtcp_stats_callback_(rtcp_callback), rtp_stats_callback_(rtp_callback) {} ReceiveStatisticsImpl::~ReceiveStatisticsImpl() { @@ -410,9 +404,8 @@ StreamStatisticianImpl* ReceiveStatisticsImpl::GetOrCreateStatistician( rtc::CritScope cs(&receive_statistics_lock_); StreamStatisticianImpl*& impl = statisticians_[ssrc]; if (impl == nullptr) { // new element - impl = - new StreamStatisticianImpl(ssrc, clock_, max_reordering_threshold_, - rtcp_stats_callback_, rtp_stats_callback_); + impl = new StreamStatisticianImpl(ssrc, clock_, max_reordering_threshold_, + rtp_stats_callback_); } return impl; } diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index 74150a9601..3935d876de 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -30,7 +30,6 @@ class StreamStatisticianImpl : public StreamStatistician, StreamStatisticianImpl(uint32_t ssrc, Clock* clock, int max_reordering_threshold, - RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback); ~StreamStatisticianImpl() override; @@ -104,14 +103,12 @@ class StreamStatisticianImpl : public StreamStatistician, RtcpStatistics last_reported_statistics_ RTC_GUARDED_BY(&stream_lock_); // stream_lock_ shouldn't be held when calling callbacks. - RtcpStatisticsCallback* const rtcp_callback_; StreamDataCountersCallback* const rtp_callback_; }; class ReceiveStatisticsImpl : public ReceiveStatistics { public: ReceiveStatisticsImpl(Clock* clock, - RtcpStatisticsCallback* rtcp_callback, StreamDataCountersCallback* rtp_callback); ~ReceiveStatisticsImpl() override; @@ -141,7 +138,6 @@ class ReceiveStatisticsImpl : public ReceiveStatistics { std::map statisticians_ RTC_GUARDED_BY(receive_statistics_lock_); - RtcpStatisticsCallback* const rtcp_stats_callback_; StreamDataCountersCallback* const rtp_stats_callback_; }; } // namespace webrtc diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index a06754e5a6..28c14036f0 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -65,16 +65,11 @@ void IncrementSequenceNumber(RtpPacketReceived* packet) { IncrementSequenceNumber(packet, 1); } -void IncrementTimestamp(RtpPacketReceived* packet, uint32_t incr) { - packet->SetTimestamp(packet->Timestamp() + incr); -} - class ReceiveStatisticsTest : public ::testing::Test { public: ReceiveStatisticsTest() : clock_(0), - receive_statistics_( - ReceiveStatistics::Create(&clock_, nullptr, nullptr)) { + receive_statistics_(ReceiveStatistics::Create(&clock_, nullptr)) { packet1_ = CreateRtpPacket(kSsrc1, kPacketSize1); packet2_ = CreateRtpPacket(kSsrc2, kPacketSize2); } @@ -233,67 +228,6 @@ TEST_F(ReceiveStatisticsTest, GetReceiveStreamDataCounters) { EXPECT_EQ(2u, counters.transmitted.packets); } -TEST_F(ReceiveStatisticsTest, RtcpCallbacks) { - class TestCallback : public RtcpStatisticsCallback { - public: - TestCallback() - : RtcpStatisticsCallback(), num_calls_(0), ssrc_(0), stats_() {} - ~TestCallback() override {} - - void StatisticsUpdated(const RtcpStatistics& statistics, - uint32_t ssrc) override { - ssrc_ = ssrc; - stats_ = statistics; - ++num_calls_; - } - - uint32_t num_calls_; - uint32_t ssrc_; - RtcpStatistics stats_; - } callback; - - receive_statistics_ = ReceiveStatistics::Create(&clock_, &callback, nullptr); - receive_statistics_->EnableRetransmitDetection(kSsrc1, true); - - // Add some arbitrary data, with loss and jitter. - packet1_.SetSequenceNumber(1); - clock_.AdvanceTimeMilliseconds(7); - IncrementTimestamp(&packet1_, 3); - receive_statistics_->OnRtpPacket(packet1_); - IncrementSequenceNumber(&packet1_, 2); - clock_.AdvanceTimeMilliseconds(9); - IncrementTimestamp(&packet1_, 9); - receive_statistics_->OnRtpPacket(packet1_); - IncrementSequenceNumber(&packet1_, -1); - clock_.AdvanceTimeMilliseconds(13); - IncrementTimestamp(&packet1_, 47); - receive_statistics_->OnRtpPacket(packet1_); - IncrementSequenceNumber(&packet1_, 3); - clock_.AdvanceTimeMilliseconds(11); - IncrementTimestamp(&packet1_, 17); - receive_statistics_->OnRtpPacket(packet1_); - IncrementSequenceNumber(&packet1_); - - EXPECT_EQ(0u, callback.num_calls_); - - // Call GetStatistics, simulating a timed rtcp sender thread. - RtcpStatistics statistics; - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); - - EXPECT_EQ(1u, callback.num_calls_); - EXPECT_EQ(callback.ssrc_, kSsrc1); - EXPECT_EQ(statistics.packets_lost, callback.stats_.packets_lost); - EXPECT_EQ(statistics.extended_highest_sequence_number, - callback.stats_.extended_highest_sequence_number); - EXPECT_EQ(statistics.fraction_lost, callback.stats_.fraction_lost); - EXPECT_EQ(statistics.jitter, callback.stats_.jitter); - EXPECT_EQ(51, statistics.fraction_lost); - EXPECT_EQ(1, statistics.packets_lost); - EXPECT_EQ(5u, statistics.extended_highest_sequence_number); - EXPECT_EQ(177u, statistics.jitter); -} - TEST_F(ReceiveStatisticsTest, SimpleLossComputation) { packet1_.SetSequenceNumber(1); receive_statistics_->OnRtpPacket(packet1_); @@ -558,7 +492,7 @@ class RtpTestCallback : public StreamDataCountersCallback { TEST_F(ReceiveStatisticsTest, RtpCallbacks) { RtpTestCallback callback; - receive_statistics_ = ReceiveStatistics::Create(&clock_, nullptr, &callback); + receive_statistics_ = ReceiveStatistics::Create(&clock_, &callback); receive_statistics_->EnableRetransmitDetection(kSsrc1, true); const size_t kHeaderLength = 20; @@ -621,7 +555,7 @@ TEST_F(ReceiveStatisticsTest, RtpCallbacks) { TEST_F(ReceiveStatisticsTest, LastPacketReceivedTimestamp) { RtpTestCallback callback; - receive_statistics_ = ReceiveStatistics::Create(&clock_, nullptr, &callback); + receive_statistics_ = ReceiveStatistics::Create(&clock_, &callback); clock_.AdvanceTimeMilliseconds(42); receive_statistics_->OnRtpPacket(packet1_); @@ -634,7 +568,7 @@ TEST_F(ReceiveStatisticsTest, LastPacketReceivedTimestamp) { TEST_F(ReceiveStatisticsTest, RtpCallbacksFecFirst) { RtpTestCallback callback; - receive_statistics_ = ReceiveStatistics::Create(&clock_, nullptr, &callback); + receive_statistics_ = ReceiveStatistics::Create(&clock_, &callback); const uint32_t kHeaderLength = 20; RtpPacketReceived packet = diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc index ea888460ba..139ba69991 100644 --- a/video/receive_statistics_proxy.cc +++ b/video/receive_statistics_proxy.cc @@ -661,15 +661,6 @@ void ReceiveStatisticsProxy::RtcpPacketTypesCounterUpdated( stats_.rtcp_packet_type_counts = packet_counter; } -void ReceiveStatisticsProxy::StatisticsUpdated( - const webrtc::RtcpStatistics& statistics, - uint32_t ssrc) { - rtc::CritScope lock(&crit_); - if (stats_.ssrc != ssrc) - return; - stats_.rtcp_stats = statistics; -} - void ReceiveStatisticsProxy::OnCname(uint32_t ssrc, absl::string_view cname) { rtc::CritScope lock(&crit_); // TODO(pbos): Handle both local and remote ssrcs here and RTC_DCHECK that we diff --git a/video/receive_statistics_proxy.h b/video/receive_statistics_proxy.h index adfab02132..715f9b4673 100644 --- a/video/receive_statistics_proxy.h +++ b/video/receive_statistics_proxy.h @@ -37,7 +37,6 @@ class Clock; struct CodecSpecificInfo; class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, - public RtcpStatisticsCallback, public RtcpCnameCallback, public RtcpPacketTypeCounterObserver, public StreamDataCountersCallback, @@ -78,9 +77,6 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, void OnTimingFrameInfoUpdated(const TimingFrameInfo& info) override; - // Overrides RtcpStatisticsCallback. - void StatisticsUpdated(const webrtc::RtcpStatistics& statistics, - uint32_t ssrc) override; // Overrides RtcpCnameCallback. void OnCname(uint32_t ssrc, absl::string_view cname) override; diff --git a/video/receive_statistics_proxy_unittest.cc b/video/receive_statistics_proxy_unittest.cc index ce622eb879..5a6a0ef903 100644 --- a/video/receive_statistics_proxy_unittest.cc +++ b/video/receive_statistics_proxy_unittest.cc @@ -463,26 +463,6 @@ TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsFrameCounts) { EXPECT_EQ(kDeltaFrames, stats.frame_counts.delta_frames); } -TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsRtcpStats) { - const uint8_t kFracLost = 0; - const int32_t kCumLost = 1; - const uint32_t kExtSeqNum = 10; - const uint32_t kJitter = 4; - - RtcpStatistics rtcp_stats; - rtcp_stats.fraction_lost = kFracLost; - rtcp_stats.packets_lost = kCumLost; - rtcp_stats.extended_highest_sequence_number = kExtSeqNum; - rtcp_stats.jitter = kJitter; - statistics_proxy_->StatisticsUpdated(rtcp_stats, kRemoteSsrc); - - VideoReceiveStream::Stats stats = statistics_proxy_->GetStats(); - EXPECT_EQ(kFracLost, stats.rtcp_stats.fraction_lost); - EXPECT_EQ(kCumLost, stats.rtcp_stats.packets_lost); - EXPECT_EQ(kExtSeqNum, stats.rtcp_stats.extended_highest_sequence_number); - EXPECT_EQ(kJitter, stats.rtcp_stats.jitter); -} - TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsCName) { const char* kName = "cName"; statistics_proxy_->OnCname(kRemoteSsrc, kName); diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index ec9de913ac..f56906d10e 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -193,8 +193,7 @@ VideoReceiveStream::VideoReceiveStream( call_stats_(call_stats), source_tracker_(clock_), stats_proxy_(&config_, clock_), - rtp_receive_statistics_( - ReceiveStatistics::Create(clock_, &stats_proxy_, &stats_proxy_)), + rtp_receive_statistics_(ReceiveStatistics::Create(clock_, &stats_proxy_)), timing_(timing), video_receiver_(clock_, timing_.get()), rtp_video_stream_receiver_(clock_, @@ -459,7 +458,12 @@ void VideoReceiveStream::Stop() { } VideoReceiveStream::Stats VideoReceiveStream::GetStats() const { - return stats_proxy_.GetStats(); + VideoReceiveStream::Stats stats = stats_proxy_.GetStats(); + StreamStatistician* statistician = + rtp_receive_statistics_->GetStatistician(stats.ssrc); + if (statistician) + statistician->GetStatistics(&stats.rtcp_stats, /*reset=*/false); + return stats; } void VideoReceiveStream::UpdateHistograms() {