From 17f914ce50ece85310aa30b5ae4ec3623ec0ce3a Mon Sep 17 00:00:00 2001 From: Jeremy Leconte Date: Thu, 18 Feb 2021 08:53:51 +0000 Subject: [PATCH] Revert "Batch assign RTP seq# for all packets of a frame." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 5cc99570620890edc3989b2cae1d1ee0669a021c. Reason for revert: Seems this CL breaks the below test when being imported in google3 https://webrtc-review.googlesource.com/c/src/+/207867 Original change's description: > Batch assign RTP seq# for all packets of a frame. > > This avoids a potential race where other call sites could assign > sequence numbers while the video frame is mid packetization - resulting > in a non-contiguous video sequence. > > Avoiding the tight lock-unlock within the loop also couldn't hurt from > a performance standpoint. > > Bug: webrtc:12448 > Change-Id: I6cc31c7743d2ca75caeaeffb98651a480dbe08e2 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/207867 > Commit-Queue: Erik Språng > Reviewed-by: Danil Chapovalov > Cr-Commit-Position: refs/heads/master@{#33291} Bug: webrtc:12448 Change-Id: I2547f946a5ba75aa09cdbfd902157011425d1c30 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/208220 Reviewed-by: Jeremy Leconte Commit-Queue: Jeremy Leconte Cr-Commit-Position: refs/heads/master@{#33294} --- modules/rtp_rtcp/source/rtp_sender.cc | 39 ++++----------------- modules/rtp_rtcp/source/rtp_sender.h | 8 ----- modules/rtp_rtcp/source/rtp_sender_video.cc | 9 ++--- 3 files changed, 9 insertions(+), 47 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 45d5a6a65d..d9796ceb74 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -47,9 +47,6 @@ constexpr uint32_t kTimestampTicksPerMs = 90; // Min size needed to get payload padding from packet history. constexpr int kMinPayloadPaddingBytes = 50; -// RED header if first byte of payload. -constexpr size_t kRedForFecHeaderLength = 1; - template constexpr RtpExtensionSize CreateExtensionSize() { return {Extension::kId, Extension::kValueSizeBytes}; @@ -614,40 +611,16 @@ bool RTPSender::AssignSequenceNumber(RtpPacketToSend* packet) { RTC_DCHECK(packet->Ssrc() == ssrc_); packet->SetSequenceNumber(sequence_number_++); - UpdateLastPacketState(*packet); - return true; -} - -bool RTPSender::AssignSequenceNumbersAndStoreLastPacketState( - rtc::ArrayView> packets) { - RTC_DCHECK(!packets.empty()); - MutexLock lock(&send_mutex_); - if (!sending_media_) - return false; - for (auto& packet : packets) { - RTC_DCHECK_EQ(packet->Ssrc(), ssrc_); - packet->SetSequenceNumber(sequence_number_++); - } - UpdateLastPacketState(**packets.rbegin()); - return true; -} - -void RTPSender::UpdateLastPacketState(const RtpPacketToSend& packet) { // Remember marker bit to determine if padding can be inserted with // sequence number following |packet|. - last_packet_marker_bit_ = packet.Marker(); - // Remember media payload type to use in the padding packet if rtx is - // disabled. - if (packet.is_red()) { - RTC_DCHECK_GE(packet.payload_size(), kRedForFecHeaderLength); - last_payload_type_ = packet.PayloadBuffer()[0]; - } else { - last_payload_type_ = packet.PayloadType(); - } + last_packet_marker_bit_ = packet->Marker(); + // Remember payload type to use in the padding packet if rtx is disabled. + last_payload_type_ = packet->PayloadType(); // Save timestamps to generate timestamp field and extensions for the padding. - last_rtp_timestamp_ = packet.Timestamp(); + last_rtp_timestamp_ = packet->Timestamp(); last_timestamp_time_ms_ = clock_->TimeInMilliseconds(); - capture_time_ms_ = packet.capture_time_ms(); + capture_time_ms_ = packet->capture_time_ms(); + return true; } void RTPSender::SetSendingMediaStatus(bool enabled) { diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 642c647955..f2b493d0c2 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -137,11 +137,6 @@ class RTPSender { // Return false if sending was turned off. bool AssignSequenceNumber(RtpPacketToSend* packet) RTC_LOCKS_EXCLUDED(send_mutex_); - // Same as AssignSequenceNumber(), but applies sequence numbers atomically to - // a batch of packets. - bool AssignSequenceNumbersAndStoreLastPacketState( - rtc::ArrayView> packets) - RTC_LOCKS_EXCLUDED(send_mutex_); // Maximum header overhead per fec/padding packet. size_t FecOrPaddingPacketMaxRtpHeaderLength() const RTC_LOCKS_EXCLUDED(send_mutex_); @@ -184,9 +179,6 @@ class RTPSender { void UpdateHeaderSizes() RTC_EXCLUSIVE_LOCKS_REQUIRED(send_mutex_); - void UpdateLastPacketState(const RtpPacketToSend& packet) - RTC_EXCLUSIVE_LOCKS_REQUIRED(send_mutex_); - Clock* const clock_; Random random_ RTC_GUARDED_BY(send_mutex_); diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 602cf9d82a..934be824a4 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -648,6 +648,8 @@ bool RTPSenderVideo::SendVideo( if (!packetizer->NextPacket(packet.get())) return false; RTC_DCHECK_LE(packet->payload_size(), expected_payload_capacity); + if (!rtp_sender_->AssignSequenceNumber(packet.get())) + return false; packet->set_allow_retransmission(allow_retransmission); packet->set_is_key_frame(video_header.frame_type == @@ -668,7 +670,7 @@ bool RTPSenderVideo::SendVideo( red_packet->SetPayloadType(*red_payload_type_); red_packet->set_is_red(true); - // Append |red_packet| instead of |packet| to output. + // Send |red_packet| instead of |packet| for allocated sequence number. red_packet->set_packet_type(RtpPacketMediaType::kVideo); red_packet->set_allow_retransmission(packet->allow_retransmission()); rtp_packets.emplace_back(std::move(red_packet)); @@ -689,11 +691,6 @@ bool RTPSenderVideo::SendVideo( } } - if (!rtp_sender_->AssignSequenceNumbersAndStoreLastPacketState(rtp_packets)) { - // Media not being sent. - return false; - } - LogAndSendToNetwork(std::move(rtp_packets), payload.size()); // Update details about the last sent frame.