Reland "Adds trial to use correct overhead calculation in pacer." This reverts commit 7affd9bcbb7a778408942d8afa4fe3ce29a8fc0b. Reason for revert: The perf issue has been addressed in the reland (https://webrtc-review.googlesource.com/c/src/+/167883). Original change's description: > Revert "Adds trial to use correct overhead calculation in pacer." > > This reverts commit 71a77c4b3b314a5e3b4e6b2f12d4886cff1b60d7. > > Reason for revert: https://webrtc-review.googlesource.com/c/src/+/167524 needs to be reverted and this CL causes a merge conflict. > > Original change's description: > > Adds trial to use correct overhead calculation in pacer. > > > > Bug: webrtc:9883 > > Change-Id: I1f25a235468678bf823ee1399ba31d94acf33be9 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166534 > > Reviewed-by: Erik Språng <sprang@webrtc.org> > > Commit-Queue: Sebastian Jansson <srte@webrtc.org> > > Cr-Commit-Position: refs/heads/master@{#30399} > > TBR=sprang@webrtc.org,srte@webrtc.org > > Change-Id: I7d3efa29f70aa0363311766980acae6d88bbcaaa > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:9883 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167880 > Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org> > Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org> > Cr-Commit-Position: refs/heads/master@{#30409} TBR=mbonadei@webrtc.org,sprang@webrtc.org,srte@webrtc.org Change-Id: Iafdef81d08078000dc368e001f67bee660e2f5bc No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9883 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167861 Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org> Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org> Cr-Commit-Position: refs/heads/master@{#30414}
diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index c2946ad..20f3a99 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc
@@ -421,6 +421,9 @@ return; } + pacer()->SetTransportOverhead( + DataSize::bytes(transport_overhead_bytes_per_packet)); + // TODO(holmer): Call AudioRtpSenders when they have been moved to // RtpTransportControllerSend. for (auto& rtp_video_sender : video_rtp_senders_) {
diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc index 6dc47b6..3646952 100644 --- a/modules/pacing/paced_sender.cc +++ b/modules/pacing/paced_sender.cc
@@ -131,6 +131,11 @@ pacing_controller_.SetIncludeOverhead(); } +void PacedSender::SetTransportOverhead(DataSize overhead_per_packet) { + rtc::CritScope cs(&critsect_); + pacing_controller_.SetTransportOverhead(overhead_per_packet); +} + TimeDelta PacedSender::ExpectedQueueTime() const { rtc::CritScope cs(&critsect_); return pacing_controller_.ExpectedQueueTime();
diff --git a/modules/pacing/paced_sender.h b/modules/pacing/paced_sender.h index 3691308..16137df 100644 --- a/modules/pacing/paced_sender.h +++ b/modules/pacing/paced_sender.h
@@ -98,6 +98,7 @@ void SetAccountForAudioPackets(bool account_for_audio) override; void SetIncludeOverhead() override; + void SetTransportOverhead(DataSize overhead_per_packet) override; // Returns the time since the oldest queued packet was enqueued. TimeDelta OldestPacketWaitTime() const override;
diff --git a/modules/pacing/pacing_controller.cc b/modules/pacing/pacing_controller.cc index 09b7630..f2b2149 100644 --- a/modules/pacing/pacing_controller.cc +++ b/modules/pacing/pacing_controller.cc
@@ -99,7 +99,10 @@ pace_audio_(IsEnabled(*field_trials_, "WebRTC-Pacer-BlockAudio")), small_first_probe_packet_( IsEnabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")), + ignore_transport_overhead_( + !IsDisabled(*field_trials_, "WebRTC-Pacer-IgnoreTransportOverhead")), min_packet_limit_(kDefaultMinPacketLimit), + transport_overhead_per_packet_(DataSize::Zero()), last_timestamp_(clock_->CurrentTime()), paused_(false), media_budget_(0), @@ -230,6 +233,13 @@ packet_queue_.SetIncludeOverhead(); } +void PacingController::SetTransportOverhead(DataSize overhead_per_packet) { + if (ignore_transport_overhead_) + return; + transport_overhead_per_packet_ = overhead_per_packet; + packet_queue_.SetTransportOverhead(overhead_per_packet); +} + TimeDelta PacingController::ExpectedQueueTime() const { RTC_DCHECK_GT(pacing_bitrate_, DataRate::Zero()); return TimeDelta::ms( @@ -521,10 +531,13 @@ RTC_DCHECK(rtp_packet); RTC_DCHECK(rtp_packet->packet_type().has_value()); const RtpPacketToSend::Type packet_type = *rtp_packet->packet_type(); - const DataSize packet_size = - DataSize::bytes(include_overhead_ ? rtp_packet->size() - : rtp_packet->payload_size() + - rtp_packet->padding_size()); + DataSize packet_size = DataSize::bytes(rtp_packet->payload_size() + + rtp_packet->padding_size()); + + if (include_overhead_) { + packet_size += DataSize::bytes(rtp_packet->headers_size()) + + transport_overhead_per_packet_; + } packet_sender_->SendRtpPacket(std::move(rtp_packet), pacing_info); data_sent += packet_size;
diff --git a/modules/pacing/pacing_controller.h b/modules/pacing/pacing_controller.h index fb4d9d3..c1b3942 100644 --- a/modules/pacing/pacing_controller.h +++ b/modules/pacing/pacing_controller.h
@@ -109,6 +109,8 @@ void SetAccountForAudioPackets(bool account_for_audio); void SetIncludeOverhead(); + void SetTransportOverhead(DataSize overhead_per_packet); + // Returns the time since the oldest queued packet was enqueued. TimeDelta OldestPacketWaitTime() const; @@ -177,9 +179,12 @@ const bool send_padding_if_silent_; const bool pace_audio_; const bool small_first_probe_packet_; + const bool ignore_transport_overhead_; TimeDelta min_packet_limit_; + DataSize transport_overhead_per_packet_; + // TODO(webrtc:9716): Remove this when we are certain clocks are monotonic. // The last millisecond timestamp returned by |clock_|. mutable Timestamp last_timestamp_;
diff --git a/modules/pacing/round_robin_packet_queue.cc b/modules/pacing/round_robin_packet_queue.cc index 754ff58..32f288c 100644 --- a/modules/pacing/round_robin_packet_queue.cc +++ b/modules/pacing/round_robin_packet_queue.cc
@@ -73,12 +73,6 @@ return enqueue_order_; } -DataSize RoundRobinPacketQueue::QueuedPacket::Size(bool count_overhead) const { - return DataSize::bytes(count_overhead ? owned_packet_->size() - : owned_packet_->payload_size() + - owned_packet_->padding_size()); -} - RtpPacketToSend* RoundRobinPacketQueue::QueuedPacket::RtpPacket() const { return owned_packet_; } @@ -117,7 +111,8 @@ RoundRobinPacketQueue::RoundRobinPacketQueue( Timestamp start_time, const WebRtcKeyValueConfig* field_trials) - : time_last_updated_(start_time), + : transport_overhead_per_packet_(DataSize::Zero()), + time_last_updated_(start_time), paused_(false), size_packets_(0), size_(DataSize::Zero()), @@ -167,7 +162,13 @@ // case a "budget" will be built up for the stream sending at the lower // rate. To avoid building a too large budget we limit |bytes| to be within // kMaxLeading bytes of the stream that has sent the most amount of bytes. - DataSize packet_size = queued_packet.Size(include_overhead_); + DataSize packet_size = + DataSize::bytes(queued_packet.RtpPacket()->payload_size() + + queued_packet.RtpPacket()->padding_size()); + if (include_overhead_) { + packet_size += DataSize::bytes(queued_packet.RtpPacket()->headers_size()) + + transport_overhead_per_packet_; + } stream->size = std::max(stream->size + packet_size, max_size_ - kMaxLeadingSize); max_size_ = std::max(max_size_, stream->size); @@ -250,14 +251,18 @@ void RoundRobinPacketQueue::SetIncludeOverhead() { include_overhead_ = true; // We need to update the size to reflect overhead for existing packets. - size_ = DataSize::Zero(); for (const auto& stream : streams_) { for (const QueuedPacket& packet : stream.second.packet_queue) { - size_ += packet.Size(include_overhead_); + size_ += DataSize::bytes(packet.RtpPacket()->headers_size()) + + transport_overhead_per_packet_; } } } +void RoundRobinPacketQueue::SetTransportOverhead(DataSize overhead_per_packet) { + transport_overhead_per_packet_ = overhead_per_packet; +} + TimeDelta RoundRobinPacketQueue::AverageQueueTime() const { if (Empty()) return TimeDelta::Zero(); @@ -299,7 +304,12 @@ packet.SubtractPauseTime(pause_time_sum_); size_packets_ += 1; - size_ += packet.Size(include_overhead_); + size_ += DataSize::bytes(packet.RtpPacket()->payload_size() + + packet.RtpPacket()->padding_size()); + if (include_overhead_) { + size_ += DataSize::bytes(packet.RtpPacket()->headers_size()) + + transport_overhead_per_packet_; + } stream->packet_queue.push(packet); }
diff --git a/modules/pacing/round_robin_packet_queue.h b/modules/pacing/round_robin_packet_queue.h index d0a2f7c..225e137 100644 --- a/modules/pacing/round_robin_packet_queue.h +++ b/modules/pacing/round_robin_packet_queue.h
@@ -53,6 +53,7 @@ void UpdateQueueTime(Timestamp now); void SetPauseState(bool paused, Timestamp now); void SetIncludeOverhead(); + void SetTransportOverhead(DataSize overhead_per_packet); private: struct QueuedPacket { @@ -73,7 +74,6 @@ Timestamp EnqueueTime() const; bool IsRetransmission() const; uint64_t EnqueueOrder() const; - DataSize Size(bool count_overhead) const; RtpPacketToSend* RtpPacket() const; std::multiset<Timestamp>::iterator EnqueueTimeIterator() const; @@ -137,6 +137,8 @@ // Just used to verify correctness. bool IsSsrcScheduled(uint32_t ssrc) const; + DataSize transport_overhead_per_packet_; + Timestamp time_last_updated_; bool paused_;
diff --git a/modules/pacing/rtp_packet_pacer.h b/modules/pacing/rtp_packet_pacer.h index 2f11c1f..d826edd 100644 --- a/modules/pacing/rtp_packet_pacer.h +++ b/modules/pacing/rtp_packet_pacer.h
@@ -65,6 +65,7 @@ // at high priority. virtual void SetAccountForAudioPackets(bool account_for_audio) = 0; virtual void SetIncludeOverhead() = 0; + virtual void SetTransportOverhead(DataSize overhead_per_packet) = 0; }; } // namespace webrtc
diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc index 54d2d84..646af4e 100644 --- a/modules/pacing/task_queue_paced_sender.cc +++ b/modules/pacing/task_queue_paced_sender.cc
@@ -143,6 +143,13 @@ }); } +void TaskQueuePacedSender::SetTransportOverhead(DataSize overhead_per_packet) { + task_queue_.PostTask([this, overhead_per_packet]() { + RTC_DCHECK_RUN_ON(&task_queue_); + pacing_controller_.SetTransportOverhead(overhead_per_packet); + }); +} + void TaskQueuePacedSender::SetQueueTimeLimit(TimeDelta limit) { task_queue_.PostTask([this, limit]() { RTC_DCHECK_RUN_ON(&task_queue_);
diff --git a/modules/pacing/task_queue_paced_sender.h b/modules/pacing/task_queue_paced_sender.h index a50ffa2..8b47f5e 100644 --- a/modules/pacing/task_queue_paced_sender.h +++ b/modules/pacing/task_queue_paced_sender.h
@@ -80,6 +80,8 @@ void SetAccountForAudioPackets(bool account_for_audio) override; void SetIncludeOverhead() override; + void SetTransportOverhead(DataSize overhead_per_packet) override; + // Returns the time since the oldest queued packet was enqueued. TimeDelta OldestPacketWaitTime() const override;