From 7fbab87b6044afcb24a849bd225dc042f3cf7843 Mon Sep 17 00:00:00 2001 From: Tommi Date: Wed, 10 Aug 2022 07:45:43 +0200 Subject: [PATCH] Report histograms in dtor of UlpfecReceiver. The data that's used to report the histograms is owned by UlpfecReceiver and moving the reporting there, simplifies things as configuration changes happen in RtpVideoStreamReceiver2 (which currently require all receive streams to be deleted+reconstructed). Additional updates: * Consistently using `Clock` for timestamps. Before there was a mix of Clock and rtc::TimeMillis. * Update code to use Timestamp and TimeDelta. Bug: none Change-Id: I89ca28ec7067a49d6b357315ae733b04e7c5a2e3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/271027 Reviewed-by: Rasmus Brandt Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#37729} --- modules/rtp_rtcp/source/ulpfec_receiver.cc | 42 ++++++++++++++++--- modules/rtp_rtcp/source/ulpfec_receiver.h | 15 +++++-- .../source/ulpfec_receiver_unittest.cc | 31 ++++++++------ test/fuzzers/ulpfec_receiver_fuzzer.cc | 5 ++- video/rtp_video_stream_receiver2.cc | 36 +++------------- video/rtp_video_stream_receiver2.h | 1 - 6 files changed, 73 insertions(+), 57 deletions(-) diff --git a/modules/rtp_rtcp/source/ulpfec_receiver.cc b/modules/rtp_rtcp/source/ulpfec_receiver.cc index 0558a2b680..f226a66786 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver.cc @@ -17,19 +17,50 @@ #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/logging.h" #include "rtc_base/time_utils.h" +#include "system_wrappers/include/metrics.h" namespace webrtc { UlpfecReceiver::UlpfecReceiver(uint32_t ssrc, + int ulpfec_payload_type, RecoveredPacketReceiver* callback, - rtc::ArrayView extensions) + rtc::ArrayView extensions, + Clock* clock) : ssrc_(ssrc), + ulpfec_payload_type_(ulpfec_payload_type), + clock_(clock), extensions_(extensions), recovered_packet_callback_(callback), fec_(ForwardErrorCorrection::CreateUlpfec(ssrc_)) {} UlpfecReceiver::~UlpfecReceiver() { RTC_DCHECK_RUN_ON(&sequence_checker_); + + if (packet_counter_.first_packet_time != Timestamp::MinusInfinity()) { + const Timestamp now = clock_->CurrentTime(); + TimeDelta elapsed = (now - packet_counter_.first_packet_time); + if (elapsed.seconds() >= metrics::kMinRunTimeInSeconds) { + if (packet_counter_.num_packets > 0) { + RTC_HISTOGRAM_PERCENTAGE( + "WebRTC.Video.ReceivedFecPacketsInPercent", + static_cast(packet_counter_.num_fec_packets * 100 / + packet_counter_.num_packets)); + } + if (packet_counter_.num_fec_packets > 0) { + RTC_HISTOGRAM_PERCENTAGE( + "WebRTC.Video.RecoveredMediaPacketsInPercentOfFec", + static_cast(packet_counter_.num_recovered_packets * 100 / + packet_counter_.num_fec_packets)); + } + if (ulpfec_payload_type_ != -1) { + RTC_HISTOGRAM_COUNTS_10000( + "WebRTC.Video.FecBitrateReceivedInKbps", + static_cast(packet_counter_.num_bytes * 8 / elapsed.seconds() / + 1000)); + } + } + } + received_packets_.clear(); fec_->ResetState(&recovered_packets_); } @@ -73,8 +104,7 @@ void UlpfecReceiver::SetRtpExtensions( // block length: 10 bits Length in bytes of the corresponding data // block excluding header. -bool UlpfecReceiver::AddReceivedRedPacket(const RtpPacketReceived& rtp_packet, - uint8_t ulpfec_payload_type) { +bool UlpfecReceiver::AddReceivedRedPacket(const RtpPacketReceived& rtp_packet) { RTC_DCHECK_RUN_ON(&sequence_checker_); // TODO(bugs.webrtc.org/11993): We get here via Call::DeliverRtp, so should be // moved to the network thread. @@ -104,7 +134,7 @@ bool UlpfecReceiver::AddReceivedRedPacket(const RtpPacketReceived& rtp_packet, // Get payload type from RED header and sequence number from RTP header. uint8_t payload_type = rtp_packet.payload()[0] & 0x7f; - received_packet->is_fec = payload_type == ulpfec_payload_type; + received_packet->is_fec = payload_type == ulpfec_payload_type_; received_packet->is_recovered = rtp_packet.recovered(); received_packet->ssrc = rtp_packet.Ssrc(); received_packet->seq_num = rtp_packet.SequenceNumber(); @@ -118,8 +148,8 @@ bool UlpfecReceiver::AddReceivedRedPacket(const RtpPacketReceived& rtp_packet, ++packet_counter_.num_packets; packet_counter_.num_bytes += rtp_packet.size(); - if (packet_counter_.first_packet_time_ms == -1) { - packet_counter_.first_packet_time_ms = rtc::TimeMillis(); + if (packet_counter_.first_packet_time == Timestamp::MinusInfinity()) { + packet_counter_.first_packet_time = clock_->CurrentTime(); } if (received_packet->is_fec) { diff --git a/modules/rtp_rtcp/source/ulpfec_receiver.h b/modules/rtp_rtcp/source/ulpfec_receiver.h index 727dba6439..b8ac8d8c30 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver.h +++ b/modules/rtp_rtcp/source/ulpfec_receiver.h @@ -23,6 +23,7 @@ #include "modules/rtp_rtcp/source/forward_error_correction.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/system/no_unique_address.h" +#include "system_wrappers/include/clock.h" namespace webrtc { @@ -33,18 +34,22 @@ struct FecPacketCounter { 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. + // Time when first packet is received. + Timestamp first_packet_time = Timestamp::MinusInfinity(); }; class UlpfecReceiver { public: UlpfecReceiver(uint32_t ssrc, + int ulpfec_payload_type, RecoveredPacketReceiver* callback, - rtc::ArrayView extensions); + rtc::ArrayView extensions, + Clock* clock); ~UlpfecReceiver(); - bool AddReceivedRedPacket(const RtpPacketReceived& rtp_packet, - uint8_t ulpfec_payload_type); + int ulpfec_payload_type() const { return ulpfec_payload_type_; } + + bool AddReceivedRedPacket(const RtpPacketReceived& rtp_packet); void ProcessReceivedFec(); @@ -54,6 +59,8 @@ class UlpfecReceiver { private: const uint32_t ssrc_; + const int ulpfec_payload_type_; + Clock* const clock_; RtpHeaderExtensionMap extensions_ RTC_GUARDED_BY(&sequence_checker_); RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_; diff --git a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc index a0f71e31d8..1b0b0daf56 100644 --- a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc +++ b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc @@ -50,7 +50,11 @@ class UlpfecReceiverTest : public ::testing::Test { protected: UlpfecReceiverTest() : fec_(ForwardErrorCorrection::CreateUlpfec(kMediaSsrc)), - receiver_fec_(kMediaSsrc, &recovered_packet_receiver_, {}), + receiver_fec_(kMediaSsrc, + kFecPayloadType, + &recovered_packet_receiver_, + {}, + Clock::GetRealTimeClock()), packet_generator_(kMediaSsrc) {} // Generates `num_fec_packets` FEC packets, given `media_packets`. @@ -125,13 +129,13 @@ void UlpfecReceiverTest::BuildAndAddRedMediaPacket(AugmentedPacket* packet, bool is_recovered) { RtpPacketReceived red_packet = packet_generator_.BuildMediaRedPacket(*packet, is_recovered); - EXPECT_TRUE(receiver_fec_.AddReceivedRedPacket(red_packet, kFecPayloadType)); + EXPECT_TRUE(receiver_fec_.AddReceivedRedPacket(red_packet)); } void UlpfecReceiverTest::BuildAndAddRedFecPacket(Packet* packet) { RtpPacketReceived red_packet = packet_generator_.BuildUlpfecRedPacket(*packet); - EXPECT_TRUE(receiver_fec_.AddReceivedRedPacket(red_packet, kFecPayloadType)); + EXPECT_TRUE(receiver_fec_.AddReceivedRedPacket(red_packet)); } void UlpfecReceiverTest::VerifyReconstructedMediaPacket( @@ -175,11 +179,12 @@ void UlpfecReceiverTest::SurvivesMaliciousPacket(const uint8_t* data, size_t length, uint8_t ulpfec_payload_type) { NullRecoveredPacketReceiver null_callback; - UlpfecReceiver receiver_fec(kMediaSsrc, &null_callback, {}); + UlpfecReceiver receiver_fec(kMediaSsrc, ulpfec_payload_type, &null_callback, + {}, Clock::GetRealTimeClock()); RtpPacketReceived rtp_packet; ASSERT_TRUE(rtp_packet.Parse(data, length)); - receiver_fec.AddReceivedRedPacket(rtp_packet, ulpfec_payload_type); + receiver_fec.AddReceivedRedPacket(rtp_packet); } TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { @@ -192,7 +197,7 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { FecPacketCounter counter = receiver_fec_.GetPacketCounter(); EXPECT_EQ(0u, counter.num_packets); - EXPECT_EQ(-1, counter.first_packet_time_ms); + EXPECT_EQ(Timestamp::MinusInfinity(), counter.first_packet_time); // Recovery auto it = augmented_media_packets.begin(); @@ -203,8 +208,8 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { EXPECT_EQ(1u, counter.num_packets); EXPECT_EQ(0u, counter.num_fec_packets); EXPECT_EQ(0u, counter.num_recovered_packets); - const int64_t first_packet_time_ms = counter.first_packet_time_ms; - EXPECT_NE(-1, first_packet_time_ms); + const Timestamp first_packet_time = counter.first_packet_time; + EXPECT_NE(Timestamp::MinusInfinity(), first_packet_time); // Drop one media packet. auto fec_it = fec_packets.begin(); @@ -217,7 +222,7 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { EXPECT_EQ(2u, counter.num_packets); EXPECT_EQ(1u, counter.num_fec_packets); EXPECT_EQ(1u, counter.num_recovered_packets); - EXPECT_EQ(first_packet_time_ms, counter.first_packet_time_ms); + EXPECT_EQ(first_packet_time, counter.first_packet_time); } TEST_F(UlpfecReceiverTest, TwoMediaOneFecNotUsesRecoveredPackets) { @@ -230,7 +235,7 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFecNotUsesRecoveredPackets) { FecPacketCounter counter = receiver_fec_.GetPacketCounter(); EXPECT_EQ(0u, counter.num_packets); - EXPECT_EQ(-1, counter.first_packet_time_ms); + EXPECT_EQ(Timestamp::MinusInfinity(), counter.first_packet_time); // Recovery auto it = augmented_media_packets.begin(); @@ -241,8 +246,8 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFecNotUsesRecoveredPackets) { EXPECT_EQ(1u, counter.num_packets); EXPECT_EQ(0u, counter.num_fec_packets); EXPECT_EQ(0u, counter.num_recovered_packets); - const int64_t first_packet_time_ms = counter.first_packet_time_ms; - EXPECT_NE(-1, first_packet_time_ms); + const Timestamp first_packet_time = counter.first_packet_time; + EXPECT_NE(Timestamp::MinusInfinity(), first_packet_time); // Drop one media packet. auto fec_it = fec_packets.begin(); @@ -254,7 +259,7 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFecNotUsesRecoveredPackets) { EXPECT_EQ(2u, counter.num_packets); EXPECT_EQ(1u, counter.num_fec_packets); EXPECT_EQ(0u, counter.num_recovered_packets); - EXPECT_EQ(first_packet_time_ms, counter.first_packet_time_ms); + EXPECT_EQ(first_packet_time, counter.first_packet_time); } TEST_F(UlpfecReceiverTest, InjectGarbageFecHeaderLengthRecovery) { diff --git a/test/fuzzers/ulpfec_receiver_fuzzer.cc b/test/fuzzers/ulpfec_receiver_fuzzer.cc index c76744ec13..d88bee2c1b 100644 --- a/test/fuzzers/ulpfec_receiver_fuzzer.cc +++ b/test/fuzzers/ulpfec_receiver_fuzzer.cc @@ -36,7 +36,8 @@ void FuzzOneInput(const uint8_t* data, size_t size) { uint16_t media_seq_num = ByteReader::ReadLittleEndian(data + 10); DummyCallback callback; - UlpfecReceiver receiver(ulpfec_ssrc, &callback, {}); + UlpfecReceiver receiver(ulpfec_ssrc, 0, &callback, {}, + Clock::GetRealTimeClock()); test::FuzzDataHelper fuzz_data(rtc::MakeArrayView(data, size)); while (fuzz_data.CanReadBytes(kMinDataNeeded)) { @@ -62,7 +63,7 @@ void FuzzOneInput(const uint8_t* data, size_t size) { parsed_packet.SetSsrc(media_ssrc); } - receiver.AddReceivedRedPacket(parsed_packet, 0); + receiver.AddReceivedRedPacket(parsed_packet); } receiver.ProcessReceivedFec(); diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc index 756b6e4510..284a50185e 100644 --- a/video/rtp_video_stream_receiver2.cc +++ b/video/rtp_video_stream_receiver2.cc @@ -236,8 +236,10 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2( rtp_receive_statistics_(rtp_receive_statistics), ulpfec_receiver_( std::make_unique(config->rtp.remote_ssrc, + config_.rtp.ulpfec_payload_type, this, - config->rtp.extensions)), + config->rtp.extensions, + clock)), packet_sink_(config->rtp.packet_sink_), receiving_(false), last_packet_log_ms_(-1), @@ -319,7 +321,7 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2( RtpVideoStreamReceiver2::~RtpVideoStreamReceiver2() { if (packet_router_) packet_router_->RemoveReceiveRtpModule(rtp_rtcp_.get()); - UpdateHistograms(); + ulpfec_receiver_.reset(); if (frame_transformer_delegate_) frame_transformer_delegate_->Reset(); } @@ -1027,8 +1029,7 @@ void RtpVideoStreamReceiver2::ParseAndHandleEncapsulatingHeader( // packets. NotifyReceiverOfEmptyPacket(packet.SequenceNumber()); } - if (!ulpfec_receiver_->AddReceivedRedPacket( - packet, config_.rtp.ulpfec_payload_type)) { + if (!ulpfec_receiver_->AddReceivedRedPacket(packet)) { return; } ulpfec_receiver_->ProcessReceivedFec(); @@ -1148,33 +1149,6 @@ void RtpVideoStreamReceiver2::StopReceive() { receiving_ = false; } -void RtpVideoStreamReceiver2::UpdateHistograms() { - FecPacketCounter counter = ulpfec_receiver_->GetPacketCounter(); - if (counter.first_packet_time_ms == -1) - return; - - int64_t elapsed_sec = - (clock_->TimeInMilliseconds() - counter.first_packet_time_ms) / 1000; - if (elapsed_sec < metrics::kMinRunTimeInSeconds) - return; - - if (counter.num_packets > 0) { - RTC_HISTOGRAM_PERCENTAGE( - "WebRTC.Video.ReceivedFecPacketsInPercent", - static_cast(counter.num_fec_packets * 100 / counter.num_packets)); - } - if (counter.num_fec_packets > 0) { - RTC_HISTOGRAM_PERCENTAGE("WebRTC.Video.RecoveredMediaPacketsInPercentOfFec", - 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 RtpVideoStreamReceiver2::InsertSpsPpsIntoTracker(uint8_t payload_type) { RTC_DCHECK_RUN_ON(&packet_sequence_checker_); RTC_DCHECK_RUN_ON(&worker_task_checker_); diff --git a/video/rtp_video_stream_receiver2.h b/video/rtp_video_stream_receiver2.h index 39d34cb524..73ed80e0ed 100644 --- a/video/rtp_video_stream_receiver2.h +++ b/video/rtp_video_stream_receiver2.h @@ -288,7 +288,6 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender, RTC_RUN_ON(packet_sequence_checker_); void NotifyReceiverOfEmptyPacket(uint16_t seq_num) RTC_RUN_ON(packet_sequence_checker_); - void UpdateHistograms(); bool IsRedEnabled() const; void InsertSpsPpsIntoTracker(uint8_t payload_type) RTC_RUN_ON(packet_sequence_checker_);