Propagate OnSendPacket even if transport sequence number is not registered
To allow to calculate send delay with that callback
Bug: None
Change-Id: I0fe1ffd42b62c99bd01670e583584511c34277db
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/319563
Reviewed-by: Åsa Persson <asapersson@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40731}
diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h
index 698f284..6804ac3 100644
--- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h
+++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h
@@ -422,7 +422,7 @@
class SendPacketObserver {
public:
virtual ~SendPacketObserver() = default;
- virtual void OnSendPacket(uint16_t packet_id,
+ virtual void OnSendPacket(absl::optional<uint16_t> packet_id,
Timestamp capture_time,
uint32_t ssrc) = 0;
};
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc
index 0821f6d..8df4c3e 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2_unittest.cc
@@ -196,10 +196,12 @@
counter_map_[ssrc] = packet_counter;
}
- void OnSendPacket(uint16_t packet_id,
+ void OnSendPacket(absl::optional<uint16_t> packet_id,
Timestamp capture_time,
uint32_t ssrc) override {
- last_sent_packet_.emplace(packet_id, capture_time, ssrc);
+ if (packet_id.has_value()) {
+ last_sent_packet_.emplace(*packet_id, capture_time, ssrc);
+ }
}
absl::optional<SentPacket> last_sent_packet() const {
diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc
index 7265f40..64feec9 100644
--- a/modules/rtp_rtcp/source/rtp_sender_egress.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc
@@ -252,7 +252,9 @@
// Downstream code actually uses this flag to distinguish between media and
// everything else.
options.is_retransmit = !is_media;
- if (auto packet_id = packet->GetExtension<TransportSequenceNumber>()) {
+ absl::optional<uint16_t> packet_id =
+ packet->GetExtension<TransportSequenceNumber>();
+ if (packet_id.has_value()) {
options.packet_id = *packet_id;
options.included_in_feedback = true;
options.included_in_allocation = true;
@@ -265,7 +267,7 @@
if (packet->packet_type() != RtpPacketMediaType::kPadding &&
packet->packet_type() != RtpPacketMediaType::kRetransmission) {
UpdateDelayStatistics(packet->capture_time(), now, packet_ssrc);
- UpdateOnSendPacket(options.packet_id, packet->capture_time(), packet_ssrc);
+ UpdateOnSendPacket(packet_id, packet->capture_time(), packet_ssrc);
}
options.batchable = enable_send_packet_batching_ && !is_audio_;
options.last_packet_in_batch = last_in_batch;
@@ -506,10 +508,10 @@
}
}
-void RtpSenderEgress::UpdateOnSendPacket(int packet_id,
+void RtpSenderEgress::UpdateOnSendPacket(absl::optional<uint16_t> packet_id,
Timestamp capture_time,
uint32_t ssrc) {
- if (!send_packet_observer_ || capture_time.IsInfinite() || packet_id == -1) {
+ if (!send_packet_observer_ || capture_time.IsInfinite()) {
return;
}
diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.h b/modules/rtp_rtcp/source/rtp_sender_egress.h
index 3e5b2b2..7d32c85 100644
--- a/modules/rtp_rtcp/source/rtp_sender_egress.h
+++ b/modules/rtp_rtcp/source/rtp_sender_egress.h
@@ -117,7 +117,9 @@
Timestamp now,
uint32_t ssrc);
void RecomputeMaxSendDelay();
- void UpdateOnSendPacket(int packet_id, Timestamp capture_time, uint32_t ssrc);
+ void UpdateOnSendPacket(absl::optional<uint16_t> packet_id,
+ Timestamp capture_time,
+ uint32_t ssrc);
// Sends packet on to `transport_`, leaving the RTP module.
bool SendPacketToNetwork(const RtpPacketToSend& packet,
const PacketOptions& options,
diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc
index 6c798e4..a57293c 100644
--- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc
@@ -36,6 +36,7 @@
using ::testing::_;
using ::testing::AllOf;
+using ::testing::Eq;
using ::testing::Field;
using ::testing::InSequence;
using ::testing::NiceMock;
@@ -57,7 +58,10 @@
class MockSendPacketObserver : public SendPacketObserver {
public:
- MOCK_METHOD(void, OnSendPacket, (uint16_t, Timestamp, uint32_t), (override));
+ MOCK_METHOD(void,
+ OnSendPacket,
+ (absl::optional<uint16_t>, Timestamp, uint32_t),
+ (override));
};
class MockTransportFeedbackObserver : public TransportFeedbackObserver {
@@ -421,12 +425,20 @@
const uint16_t kTransportSequenceNumber = 1;
EXPECT_CALL(
send_packet_observer_,
- OnSendPacket(kTransportSequenceNumber, clock_->CurrentTime(), kSsrc));
+ OnSendPacket(Eq(kTransportSequenceNumber), clock_->CurrentTime(), kSsrc));
std::unique_ptr<RtpPacketToSend> packet = BuildRtpPacket();
packet->SetExtension<TransportSequenceNumber>(kTransportSequenceNumber);
sender->SendPacket(std::move(packet), PacedPacketInfo());
}
+TEST_F(RtpSenderEgressTest, OnSendPacketUpdatedWithoutTransportSequenceNumber) {
+ std::unique_ptr<RtpSenderEgress> sender = CreateRtpSenderEgress();
+
+ EXPECT_CALL(send_packet_observer_,
+ OnSendPacket(Eq(absl::nullopt), clock_->CurrentTime(), kSsrc));
+ sender->SendPacket(BuildRtpPacket(), PacedPacketInfo());
+}
+
TEST_F(RtpSenderEgressTest, OnSendPacketNotUpdatedForRetransmits) {
std::unique_ptr<RtpSenderEgress> sender = CreateRtpSenderEgress();
header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId,
@@ -882,16 +894,16 @@
EXPECT_CALL(send_side_delay_observer,
SendSideDelayUpdated(kDiffMs, kDiffMs, kFlexFecSsrc));
- EXPECT_CALL(send_packet_observer_, OnSendPacket(1, capture_time, kSsrc));
+ EXPECT_CALL(send_packet_observer_, OnSendPacket(Eq(1), capture_time, kSsrc));
sender->SendPacket(std::move(video_packet), PacedPacketInfo());
// Send packet observer not called for padding/retransmissions.
- EXPECT_CALL(send_packet_observer_, OnSendPacket(2, _, _)).Times(0);
+ EXPECT_CALL(send_packet_observer_, OnSendPacket(Eq(2), _, _)).Times(0);
sender->SendPacket(std::move(rtx_packet), PacedPacketInfo());
EXPECT_CALL(send_packet_observer_,
- OnSendPacket(3, capture_time, kFlexFecSsrc));
+ OnSendPacket(Eq(3), capture_time, kFlexFecSsrc));
sender->SendPacket(std::move(fec_packet), PacedPacketInfo());
time_controller_.AdvanceTime(TimeDelta::Zero());
diff --git a/video/video_send_stream.h b/video/video_send_stream.h
index 8ae70be..05970d6 100644
--- a/video/video_send_stream.h
+++ b/video/video_send_stream.h
@@ -105,11 +105,13 @@
SendDelayStats* send_delay_stats)
: stats_proxy_(*stats_proxy), send_delay_stats_(*send_delay_stats) {}
- void OnSendPacket(uint16_t packet_id,
+ void OnSendPacket(absl::optional<uint16_t> packet_id,
Timestamp capture_time,
uint32_t ssrc) override {
stats_proxy_.OnSendPacket(ssrc, capture_time);
- send_delay_stats_.OnSendPacket(packet_id, capture_time, ssrc);
+ if (packet_id.has_value()) {
+ send_delay_stats_.OnSendPacket(*packet_id, capture_time, ssrc);
+ }
}
private:
