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 <brandtr@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37729}
diff --git a/modules/rtp_rtcp/source/ulpfec_receiver.cc b/modules/rtp_rtcp/source/ulpfec_receiver.cc
index 0558a2b..f226a66 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<const RtpExtension> extensions)
+ rtc::ArrayView<const RtpExtension> 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<int>(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<int>(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<int>(packet_counter_.num_bytes * 8 / elapsed.seconds() /
+ 1000));
+ }
+ }
+ }
+
received_packets_.clear();
fec_->ResetState(&recovered_packets_);
}
@@ -73,8 +104,7 @@
// 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 @@
// 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 @@
++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 727dba6..b8ac8d8 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 @@
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<const RtpExtension> extensions);
+ rtc::ArrayView<const RtpExtension> 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 @@
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 a0f71e3..1b0b0da 100644
--- a/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc
+++ b/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc
@@ -50,7 +50,11 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 c76744e..d88bee2 100644
--- a/test/fuzzers/ulpfec_receiver_fuzzer.cc
+++ b/test/fuzzers/ulpfec_receiver_fuzzer.cc
@@ -36,7 +36,8 @@
uint16_t media_seq_num = ByteReader<uint16_t>::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 @@
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 756b6e4..284a501 100644
--- a/video/rtp_video_stream_receiver2.cc
+++ b/video/rtp_video_stream_receiver2.cc
@@ -236,8 +236,10 @@
rtp_receive_statistics_(rtp_receive_statistics),
ulpfec_receiver_(
std::make_unique<UlpfecReceiver>(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() {
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 @@
// 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 @@
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<int>(counter.num_fec_packets * 100 / counter.num_packets));
- }
- if (counter.num_fec_packets > 0) {
- RTC_HISTOGRAM_PERCENTAGE("WebRTC.Video.RecoveredMediaPacketsInPercentOfFec",
- static_cast<int>(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<int>(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 39d34cb..73ed80e 100644
--- a/video/rtp_video_stream_receiver2.h
+++ b/video/rtp_video_stream_receiver2.h
@@ -288,7 +288,6 @@
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_);
