From 378fb2862162823eaffceb83b56a5fa408ce11b5 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 11 Sep 2023 11:34:57 +0200 Subject: [PATCH] Propagate OnSendPacket even if transport sequence number is not registered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#40731} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 2 +- .../source/rtp_rtcp_impl2_unittest.cc | 6 +++-- modules/rtp_rtcp/source/rtp_sender_egress.cc | 10 +++++---- modules/rtp_rtcp/source/rtp_sender_egress.h | 4 +++- .../source/rtp_sender_egress_unittest.cc | 22 ++++++++++++++----- video/video_send_stream.h | 6 +++-- 6 files changed, 35 insertions(+), 15 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 698f284fa5..6804ac3e2e 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -422,7 +422,7 @@ class SendSideDelayObserver { class SendPacketObserver { public: virtual ~SendPacketObserver() = default; - virtual void OnSendPacket(uint16_t packet_id, + virtual void OnSendPacket(absl::optional 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 0821f6deb0..8df4c3ecbd 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 @@ class RtpRtcpModule : public RtcpPacketTypeCounterObserver, counter_map_[ssrc] = packet_counter; } - void OnSendPacket(uint16_t packet_id, + void OnSendPacket(absl::optional 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 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 7265f409c6..64feec9b8e 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -252,7 +252,9 @@ void RtpSenderEgress::CompleteSendPacket(const Packet& compound_packet, // Downstream code actually uses this flag to distinguish between media and // everything else. options.is_retransmit = !is_media; - if (auto packet_id = packet->GetExtension()) { + absl::optional packet_id = + packet->GetExtension(); + if (packet_id.has_value()) { options.packet_id = *packet_id; options.included_in_feedback = true; options.included_in_allocation = true; @@ -265,7 +267,7 @@ void RtpSenderEgress::CompleteSendPacket(const Packet& compound_packet, 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::RecomputeMaxSendDelay() { } } -void RtpSenderEgress::UpdateOnSendPacket(int packet_id, +void RtpSenderEgress::UpdateOnSendPacket(absl::optional 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 3e5b2b21c3..7d32c8576f 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.h +++ b/modules/rtp_rtcp/source/rtp_sender_egress.h @@ -117,7 +117,9 @@ class RtpSenderEgress { Timestamp now, uint32_t ssrc); void RecomputeMaxSendDelay(); - void UpdateOnSendPacket(int packet_id, Timestamp capture_time, uint32_t ssrc); + void UpdateOnSendPacket(absl::optional 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 6c798e4595..a57293c43b 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 @@ namespace { using ::testing::_; using ::testing::AllOf; +using ::testing::Eq; using ::testing::Field; using ::testing::InSequence; using ::testing::NiceMock; @@ -57,7 +58,10 @@ enum : int { class MockSendPacketObserver : public SendPacketObserver { public: - MOCK_METHOD(void, OnSendPacket, (uint16_t, Timestamp, uint32_t), (override)); + MOCK_METHOD(void, + OnSendPacket, + (absl::optional, Timestamp, uint32_t), + (override)); }; class MockTransportFeedbackObserver : public TransportFeedbackObserver { @@ -421,12 +425,20 @@ TEST_F(RtpSenderEgressTest, OnSendPacketUpdated) { const uint16_t kTransportSequenceNumber = 1; EXPECT_CALL( send_packet_observer_, - OnSendPacket(kTransportSequenceNumber, clock_->CurrentTime(), kSsrc)); + OnSendPacket(Eq(kTransportSequenceNumber), clock_->CurrentTime(), kSsrc)); std::unique_ptr packet = BuildRtpPacket(); packet->SetExtension(kTransportSequenceNumber); sender->SendPacket(std::move(packet), PacedPacketInfo()); } +TEST_F(RtpSenderEgressTest, OnSendPacketUpdatedWithoutTransportSequenceNumber) { + std::unique_ptr 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 sender = CreateRtpSenderEgress(); header_extensions_.RegisterByUri(kTransportSequenceNumberExtensionId, @@ -882,16 +894,16 @@ TEST_F(RtpSenderEgressTest, SendPacketUpdatesStats) { 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 8ae70be79c..05970d619e 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -105,11 +105,13 @@ class VideoSendStream : public webrtc::VideoSendStream { SendDelayStats* send_delay_stats) : stats_proxy_(*stats_proxy), send_delay_stats_(*send_delay_stats) {} - void OnSendPacket(uint16_t packet_id, + void OnSendPacket(absl::optional 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: