From caef51e25a4d88b9bc2b791009b5cc39a678a567 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Tue, 27 Aug 2019 09:19:49 +0200 Subject: [PATCH] Consolidate FEC book-keeping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Number of received FEC bytes is used for the WebRTC.Video.FecBitrateReceivedInKbps UMA histogram. Before this cl, that value is based on a FEC packet counter updated by ReceiveStatistics::FecPacketReceived. This cl deletes that method, and instead adds a byte count to the FecPacketCounter struct, which is maintained by the UlpFecReceiver and used for other FEC-related stats. Bug: webrtc:10917 Change-Id: I24bd494b6909a2fe109d28e2b71ca8f413d05911 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150533 Reviewed-by: Åsa Persson Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#28976} --- modules/rtp_rtcp/include/receive_statistics.h | 3 -- modules/rtp_rtcp/include/ulpfec_receiver.h | 17 +++---- .../source/receive_statistics_impl.cc | 14 ------ .../rtp_rtcp/source/receive_statistics_impl.h | 2 - .../source/receive_statistics_unittest.cc | 46 ------------------- .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 1 + video/receive_statistics_proxy.cc | 6 --- video/rtp_video_stream_receiver.cc | 6 ++- 8 files changed, 13 insertions(+), 82 deletions(-) diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h index 7f185a8360..cb4ad59a4d 100644 --- a/modules/rtp_rtcp/include/receive_statistics.h +++ b/modules/rtp_rtcp/include/receive_statistics.h @@ -64,9 +64,6 @@ class ReceiveStatistics : public ReceiveStatisticsProvider, static std::unique_ptr Create(Clock* clock); - // Increment counter for number of FEC packets received. - virtual void FecPacketReceived(const RtpPacketReceived& packet) = 0; - // Returns a pointer to the statistician of an ssrc. virtual StreamStatistician* GetStatistician(uint32_t ssrc) const = 0; diff --git a/modules/rtp_rtcp/include/ulpfec_receiver.h b/modules/rtp_rtcp/include/ulpfec_receiver.h index 30fac726fa..5e0a156273 100644 --- a/modules/rtp_rtcp/include/ulpfec_receiver.h +++ b/modules/rtp_rtcp/include/ulpfec_receiver.h @@ -20,16 +20,13 @@ namespace webrtc { struct FecPacketCounter { - FecPacketCounter() - : num_packets(0), - num_fec_packets(0), - num_recovered_packets(0), - first_packet_time_ms(-1) {} - - size_t num_packets; // Number of received packets. - size_t num_fec_packets; // Number of received FEC packets. - size_t num_recovered_packets; // Number of recovered media packets using FEC. - int64_t first_packet_time_ms; // Time when first packet is received. + FecPacketCounter() = default; + size_t num_packets = 0; // Number of received packets. + size_t num_bytes = 0; + size_t num_fec_packets = 0; // Number of received FEC packets. + size_t num_recovered_packets = + 0; // Number of recovered media packets using FEC. + int64_t first_packet_time_ms = -1; // Time when first packet is received. }; class UlpfecReceiver { diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index f4ea2a096c..ca74908271 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -149,12 +149,6 @@ void StreamStatisticianImpl::UpdateJitter(const RtpPacketReceived& packet, } } -void StreamStatisticianImpl::FecPacketReceived( - const RtpPacketReceived& packet) { - rtc::CritScope cs(&stream_lock_); - receive_counters_.fec.AddPacket(packet); -} - void StreamStatisticianImpl::SetMaxReorderingThreshold( int max_reordering_threshold) { rtc::CritScope cs(&stream_lock_); @@ -358,14 +352,6 @@ void ReceiveStatisticsImpl::OnRtpPacket(const RtpPacketReceived& packet) { GetOrCreateStatistician(packet.Ssrc())->OnRtpPacket(packet); } -void ReceiveStatisticsImpl::FecPacketReceived(const RtpPacketReceived& packet) { - StreamStatisticianImpl* impl = GetStatistician(packet.Ssrc()); - // Ignore FEC if it is the first packet. - if (impl) { - impl->FecPacketReceived(packet); - } -} - StreamStatisticianImpl* ReceiveStatisticsImpl::GetStatistician( uint32_t ssrc) const { rtc::CritScope cs(&receive_statistics_lock_); diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index d76e431826..b7b9be3fdf 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -44,7 +44,6 @@ class StreamStatisticianImpl : public StreamStatistician, // Implements RtpPacketSinkInterface void OnRtpPacket(const RtpPacketReceived& packet) override; - void FecPacketReceived(const RtpPacketReceived& packet); void SetMaxReorderingThreshold(int max_reordering_threshold); void EnableRetransmitDetection(bool enable); @@ -114,7 +113,6 @@ class ReceiveStatisticsImpl : public ReceiveStatistics { void OnRtpPacket(const RtpPacketReceived& packet) override; // Implements ReceiveStatistics. - void FecPacketReceived(const RtpPacketReceived& packet) override; // Note: More specific return type for use in the implementation. StreamStatisticianImpl* GetStatistician(uint32_t ssrc) const override; void SetMaxReorderingThreshold(int max_reordering_threshold) override; diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index 81e66b6418..053460e2ba 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -552,20 +552,6 @@ TEST_F(ReceiveStatisticsTest, StreamDataCounters) { EXPECT_EQ(counters.retransmitted.header_bytes, kHeaderLength); EXPECT_EQ(counters.retransmitted.padding_bytes, kPaddingLength); EXPECT_EQ(counters.retransmitted.packets, 1u); - - // One FEC packet. - packet1.SetSequenceNumber(packet2.SequenceNumber() + 1); - clock_.AdvanceTimeMilliseconds(5); - receive_statistics_->OnRtpPacket(packet1); - receive_statistics_->FecPacketReceived(packet1); - counters = receive_statistics_->GetStatistician(kSsrc1) - ->GetReceiveStreamDataCounters(); - EXPECT_EQ(counters.transmitted.payload_bytes, kPacketSize1 * 4); - EXPECT_EQ(counters.transmitted.header_bytes, kHeaderLength * 4); - EXPECT_EQ(counters.transmitted.packets, 4u); - EXPECT_EQ(counters.fec.payload_bytes, kPacketSize1); - EXPECT_EQ(counters.fec.header_bytes, kHeaderLength); - EXPECT_EQ(counters.fec.packets, 1u); } TEST_F(ReceiveStatisticsTest, LastPacketReceivedTimestamp) { @@ -585,37 +571,5 @@ TEST_F(ReceiveStatisticsTest, LastPacketReceivedTimestamp) { EXPECT_EQ(45, counters.last_packet_received_timestamp_ms); } -TEST_F(ReceiveStatisticsTest, FecFirst) { - receive_statistics_ = ReceiveStatistics::Create(&clock_); - - const uint32_t kHeaderLength = 20; - RtpPacketReceived packet = - CreateRtpPacket(kSsrc1, kHeaderLength, kPacketSize1, 0); - // If first packet is FEC, ignore it. - receive_statistics_->FecPacketReceived(packet); - - EXPECT_EQ(receive_statistics_->GetStatistician(kSsrc1), nullptr); - - receive_statistics_->OnRtpPacket(packet); - StreamDataCounters counters = receive_statistics_->GetStatistician(kSsrc1) - ->GetReceiveStreamDataCounters(); - EXPECT_EQ(counters.transmitted.payload_bytes, kPacketSize1); - EXPECT_EQ(counters.transmitted.header_bytes, kHeaderLength); - EXPECT_EQ(counters.transmitted.padding_bytes, 0u); - EXPECT_EQ(counters.transmitted.packets, 1u); - EXPECT_EQ(counters.fec.packets, 0u); - - receive_statistics_->FecPacketReceived(packet); - counters = receive_statistics_->GetStatistician(kSsrc1) - ->GetReceiveStreamDataCounters(); - EXPECT_EQ(counters.transmitted.payload_bytes, kPacketSize1); - EXPECT_EQ(counters.transmitted.header_bytes, kHeaderLength); - EXPECT_EQ(counters.transmitted.padding_bytes, 0u); - EXPECT_EQ(counters.transmitted.packets, 1u); - EXPECT_EQ(counters.fec.payload_bytes, kPacketSize1); - EXPECT_EQ(counters.fec.header_bytes, kHeaderLength); - EXPECT_EQ(counters.fec.packets, 1u); -} - } // namespace } // namespace webrtc diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index a5d6368df0..42d7af0109 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -122,6 +122,7 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( } ++packet_counter_.num_packets; + packet_counter_.num_bytes += packet_length; if (packet_counter_.first_packet_time_ms == -1) { packet_counter_.first_packet_time_ms = rtc::TimeMillis(); } diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc index 17cec1aba7..4f3b6b9db8 100644 --- a/video/receive_statistics_proxy.cc +++ b/video/receive_statistics_proxy.cc @@ -432,12 +432,6 @@ void ReceiveStatisticsProxy::UpdateHistograms( static_cast(rtx_stats->transmitted.TotalBytes() * 8 / elapsed_sec / 1000)); } - if (config_.rtp.ulpfec_payload_type != -1) { - RTC_HISTOGRAM_COUNTS_10000( - "WebRTC.Video.FecBitrateReceivedInKbps", - static_cast(rtp_rtx_stats.fec.TotalBytes() * 8 / elapsed_sec / - 1000)); - } const RtcpPacketTypeCounter& counters = stats_.rtcp_packet_type_counts; RTC_HISTOGRAM_COUNTS_10000("WebRTC.Video.NackPacketsSentPerMinute", counters.nack_packets * 60 / elapsed_sec); diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 6f478f8c22..cef5602780 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -725,7 +725,6 @@ void RtpVideoStreamReceiver::ParseAndHandleEncapsulatingHeader( if (packet.PayloadType() == config_.rtp.red_payload_type && packet.payload_size() > 0) { if (packet.payload()[0] == config_.rtp.ulpfec_payload_type) { - rtp_receive_statistics_->FecPacketReceived(packet); // Notify video_receiver about received FEC packets to avoid NACKing these // packets. NotifyReceiverOfEmptyPacket(packet.SequenceNumber()); @@ -866,6 +865,11 @@ void RtpVideoStreamReceiver::UpdateHistograms() { static_cast(counter.num_recovered_packets * 100 / counter.num_fec_packets)); } + if (config_.rtp.ulpfec_payload_type != -1) { + RTC_HISTOGRAM_COUNTS_10000( + "WebRTC.Video.FecBitrateReceivedInKbps", + static_cast(counter.num_bytes * 8 / elapsed_sec / 1000)); + } } void RtpVideoStreamReceiver::InsertSpsPpsIntoTracker(uint8_t payload_type) {