From 7058da6e297af167552703e73551bc28c70bcae2 Mon Sep 17 00:00:00 2001 From: Jakob Ivarsson Date: Mon, 28 Oct 2024 12:33:53 +0000 Subject: [PATCH] Rename PacketOptions.is_retransmit to is_media. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is used to distinguish between audio/video packets and everything else (retransmit/padding/fec), so naming it is_media makes more sense. This is a follow up to https://webrtc-review.googlesource.com/366644 Bug: b/375148360 Change-Id: Ia53f4d707ceb85f059688d86bc5dcc2d57908d88 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/366424 Reviewed-by: Danil Chapovalov Commit-Queue: Jakob Ivarsson‎ Reviewed-by: Per Kjellander Cr-Commit-Position: refs/heads/main@{#43319} --- api/call/transport.h | 4 ++-- media/base/media_channel_impl.cc | 4 ++-- .../source/deprecated/deprecated_rtp_sender_egress.cc | 9 +++------ modules/rtp_rtcp/source/rtp_sender_egress.cc | 10 +++------- modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc | 10 +++++----- 5 files changed, 15 insertions(+), 22 deletions(-) diff --git a/api/call/transport.h b/api/call/transport.h index 7240a8b8b1..eb2059f34d 100644 --- a/api/call/transport.h +++ b/api/call/transport.h @@ -27,8 +27,8 @@ struct PacketOptions { // Negative ids are invalid and should be interpreted // as packet_id not being set. int64_t packet_id = -1; - // Whether this is a retransmission of an earlier packet. - bool is_retransmit = false; + // Whether this is an audio or video packet, excluding retransmissions. + bool is_media = true; bool included_in_feedback = false; bool included_in_allocation = false; // Whether this packet can be part of a packet batch at lower levels. diff --git a/media/base/media_channel_impl.cc b/media/base/media_channel_impl.cc index e3ae4a2878..e354db54a0 100644 --- a/media/base/media_channel_impl.cc +++ b/media/base/media_channel_impl.cc @@ -207,7 +207,7 @@ bool MediaChannelUtil::TransportForMediaChannels::SendRtp( included_in_allocation = options.included_in_allocation, batchable = options.batchable, last_packet_in_batch = options.last_packet_in_batch, - is_retransmit = options.is_retransmit, + is_media = options.is_media, packet = rtc::CopyOnWriteBuffer(packet, kMaxRtpPacketLen)]() mutable { rtc::PacketOptions rtc_options; rtc_options.packet_id = packet_id; @@ -218,7 +218,7 @@ bool MediaChannelUtil::TransportForMediaChannels::SendRtp( included_in_feedback; rtc_options.info_signaled_after_sent.included_in_allocation = included_in_allocation; - rtc_options.info_signaled_after_sent.is_media = !is_retransmit; + rtc_options.info_signaled_after_sent.is_media = is_media; rtc_options.batchable = batchable; rtc_options.last_packet_in_batch = last_packet_in_batch; DoSendPacket(&packet, false, rtc_options); diff --git a/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc b/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc index 63f2e8d269..fceb541b7b 100644 --- a/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/deprecated/deprecated_rtp_sender_egress.cc @@ -153,12 +153,9 @@ void DEPRECATED_RtpSenderEgress::SendPacket( } } - const bool is_media = packet->packet_type() == RtpPacketMediaType::kAudio || - packet->packet_type() == RtpPacketMediaType::kVideo; + options.is_media = packet->packet_type() == RtpPacketMediaType::kAudio || + packet->packet_type() == RtpPacketMediaType::kVideo; - // Downstream code actually uses this flag to distinguish between media and - // everything else. - options.is_retransmit = !is_media; if (auto packet_id = packet->GetExtension()) { options.packet_id = *packet_id; options.included_in_feedback = true; @@ -176,7 +173,7 @@ void DEPRECATED_RtpSenderEgress::SendPacket( // Put packet in retransmission history or update pending status even if // actual sending fails. - if (is_media && packet->allow_retransmission()) { + if (options.is_media && packet->allow_retransmission()) { packet_history_->PutRtpPacket(std::make_unique(*packet), now); } else if (packet->retransmitted_sequence_number()) { diff --git a/modules/rtp_rtcp/source/rtp_sender_egress.cc b/modules/rtp_rtcp/source/rtp_sender_egress.cc index e93d1b8603..2d3ecf26ac 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress.cc @@ -274,15 +274,11 @@ void RtpSenderEgress::CompleteSendPacket(const Packet& compound_packet, RTC_DCHECK_RUN_ON(worker_queue_); auto& [packet, pacing_info, now] = compound_packet; RTC_CHECK(packet); - const bool is_media = packet->packet_type() == RtpPacketMediaType::kAudio || - packet->packet_type() == RtpPacketMediaType::kVideo; PacketOptions options; options.included_in_allocation = force_part_of_allocation_; - - // Downstream code actually uses this flag to distinguish between media and - // everything else. - options.is_retransmit = !is_media; + options.is_media = packet->packet_type() == RtpPacketMediaType::kAudio || + packet->packet_type() == RtpPacketMediaType::kVideo; // Set Packet id from transport sequence number header extension if it is // used. The source of the header extension is @@ -312,7 +308,7 @@ void RtpSenderEgress::CompleteSendPacket(const Packet& compound_packet, // Put packet in retransmission history or update pending status even if // actual sending fails. - if (is_media && packet->allow_retransmission()) { + if (options.is_media && packet->allow_retransmission()) { packet_history_->PutRtpPacket(std::make_unique(*packet), now); } else if (packet->retransmitted_sequence_number()) { diff --git a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc index c1e4772b85..28e146d960 100644 --- a/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_egress_unittest.cc @@ -215,20 +215,20 @@ TEST_F(RtpSenderEgressTest, CollectsPacketsWhenBatchingWithVideo) { sender.OnBatchComplete(); } -TEST_F(RtpSenderEgressTest, PacketOptionsIsRetransmitSetByPacketType) { +TEST_F(RtpSenderEgressTest, PacketOptionsIsMediaSetByPacketType) { std::unique_ptr sender = CreateRtpSenderEgress(); std::unique_ptr media_packet = BuildRtpPacket(); auto sequence_number = media_packet->SequenceNumber(); media_packet->set_packet_type(RtpPacketMediaType::kVideo); sender->SendPacket(std::move(media_packet), PacedPacketInfo()); - EXPECT_FALSE(transport_.last_packet()->options.is_retransmit); + EXPECT_TRUE(transport_.last_packet()->options.is_media); std::unique_ptr retransmission = BuildRtpPacket(); retransmission->set_packet_type(RtpPacketMediaType::kRetransmission); retransmission->set_retransmitted_sequence_number(sequence_number); sender->SendPacket(std::move(retransmission), PacedPacketInfo()); - EXPECT_TRUE(transport_.last_packet()->options.is_retransmit); + EXPECT_FALSE(transport_.last_packet()->options.is_media); } TEST_F(RtpSenderEgressTest, DoesnSetIncludedInAllocationByDefault) { @@ -785,7 +785,7 @@ TEST_F(RtpSenderEgressTest, SendPacketSetsPacketOptions) { EXPECT_EQ(packet_options.packet_id, kPacketId); EXPECT_TRUE(packet_options.included_in_allocation); EXPECT_TRUE(packet_options.included_in_feedback); - EXPECT_FALSE(packet_options.is_retransmit); + EXPECT_TRUE(packet_options.is_media); // Send another packet as retransmission, verify options are populated. std::unique_ptr retransmission = BuildRtpPacket(); @@ -794,7 +794,7 @@ TEST_F(RtpSenderEgressTest, SendPacketSetsPacketOptions) { retransmission->set_retransmitted_sequence_number(kSequenceNumber); retransmission->set_original_ssrc(ssrc); sender->SendPacket(std::move(retransmission), PacedPacketInfo()); - EXPECT_TRUE(transport_.last_packet()->options.is_retransmit); + EXPECT_FALSE(transport_.last_packet()->options.is_media); } TEST_F(RtpSenderEgressTest, SendPacketSetsPacketOptionsIdFromExtension) {