Only include overhead if using send side bandwidth estimation.
Bug: webrtc:11298
Change-Id: Ia2daf690461b55d394c1b964d6a7977a98be8be2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166820
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Reviewed-by: Sam Zackrisson <saza@webrtc.org>
Reviewed-by: Ali Tofigh <alito@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30382}
diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 5e3b9ff..79e08b7 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -342,6 +342,8 @@
config_.max_bitrate_bps != -1 &&
(allocate_audio_without_feedback_ || TransportSeqNumId(config_) != 0)) {
rtp_transport_->AccountForAudioPacketsInPacedSender(true);
+ if (send_side_bwe_with_overhead_)
+ rtp_transport_->IncludeOverheadInPacedSender();
rtp_rtcp_module_->SetAsPartOfAllocation(true);
rtc::Event thread_sync_event;
worker_queue_->PostTask([&] {
@@ -591,7 +593,8 @@
}
// Enable ANA if configured (currently only used by Opus).
- if (new_config.audio_network_adaptor_config) {
+ if (new_config.audio_network_adaptor_config &&
+ TransportSeqNumId(new_config) != 0) {
if (encoder->EnableAudioNetworkAdaptor(
*new_config.audio_network_adaptor_config, event_log_)) {
RTC_DLOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
@@ -690,7 +693,8 @@
config_.audio_network_adaptor_config) {
return;
}
- if (new_config.audio_network_adaptor_config) {
+ if (new_config.audio_network_adaptor_config &&
+ TransportSeqNumId(new_config) != 0) {
channel_send_->CallEncoder([&](AudioEncoder* encoder) {
if (encoder->EnableAudioNetworkAdaptor(
*new_config.audio_network_adaptor_config, event_log_)) {
@@ -765,6 +769,8 @@
if (!new_config.has_dscp && new_config.min_bitrate_bps != -1 &&
new_config.max_bitrate_bps != -1 && TransportSeqNumId(new_config) != 0) {
rtp_transport_->AccountForAudioPacketsInPacedSender(true);
+ if (send_side_bwe_with_overhead_)
+ rtp_transport_->IncludeOverheadInPacedSender();
rtc::Event thread_sync_event;
worker_queue_->PostTask([&] {
RTC_DCHECK_RUN_ON(worker_queue_);
diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc
index 0472366..3b9fbb7 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -490,6 +490,8 @@
const std::string kAnaConfigString = "abcde";
const std::string kAnaReconfigString = "12345";
+ helper.config().rtp.extensions.push_back(RtpExtension(
+ RtpExtension::kTransportSequenceNumberUri, kTransportSequenceNumberId));
helper.config().audio_network_adaptor_config = kAnaConfigString;
EXPECT_CALL(helper.mock_encoder_factory(), MakeAudioEncoderMock(_, _, _, _))
diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc
index 62b7008..c2946ad 100644
--- a/call/rtp_transport_controller_send.cc
+++ b/call/rtp_transport_controller_send.cc
@@ -434,6 +434,10 @@
pacer()->SetAccountForAudioPackets(account_for_audio);
}
+void RtpTransportControllerSend::IncludeOverheadInPacedSender() {
+ pacer()->SetIncludeOverhead();
+}
+
void RtpTransportControllerSend::OnReceivedEstimatedBitrate(uint32_t bitrate) {
RemoteBitrateReport msg;
msg.receive_time = Timestamp::ms(clock_->TimeInMilliseconds());
diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h
index f74c4e5..b07bea7 100644
--- a/call/rtp_transport_controller_send.h
+++ b/call/rtp_transport_controller_send.h
@@ -107,6 +107,7 @@
size_t transport_overhead_per_packet) override;
void AccountForAudioPacketsInPacedSender(bool account_for_audio) override;
+ void IncludeOverheadInPacedSender() override;
// Implements RtcpBandwidthObserver interface
void OnReceivedEstimatedBitrate(uint32_t bitrate) override;
diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h
index 1e881dc..b40aabd 100644
--- a/call/rtp_transport_controller_send_interface.h
+++ b/call/rtp_transport_controller_send_interface.h
@@ -150,6 +150,7 @@
size_t transport_overhead_per_packet) = 0;
virtual void AccountForAudioPacketsInPacedSender(bool account_for_audio) = 0;
+ virtual void IncludeOverheadInPacedSender() = 0;
};
} // namespace webrtc
diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc
index a926eb5..413171f 100644
--- a/call/rtp_video_sender.cc
+++ b/call/rtp_video_sender.cc
@@ -279,6 +279,11 @@
}
return PayloadStringToCodecType(config.payload_name);
}
+bool TransportSeqNumExtensionConfigured(const RtpConfig& config_config) {
+ return absl::c_any_of(config_config.extensions, [](const RtpExtension& ext) {
+ return ext.uri == RtpExtension::kTransportSequenceNumberUri;
+ });
+}
} // namespace
RtpVideoSender::RtpVideoSender(
@@ -301,6 +306,7 @@
"WebRTC-SubtractPacketizationOverhead")),
use_early_loss_detection_(
!webrtc::field_trial::IsDisabled("WebRTC-UseEarlyLossDetection")),
+ has_packet_feedback_(TransportSeqNumExtensionConfigured(rtp_config)),
active_(false),
module_process_thread_(nullptr),
suspended_ssrcs_(std::move(suspended_ssrcs)),
@@ -330,6 +336,8 @@
frame_counts_(rtp_config.ssrcs.size()),
frame_count_observer_(observers.frame_count_observer) {
RTC_DCHECK_EQ(rtp_config_.ssrcs.size(), rtp_streams_.size());
+ if (send_side_bwe_with_overhead_ && has_packet_feedback_)
+ transport_->IncludeOverheadInPacedSender();
module_process_thread_checker_.Detach();
// SSRCs are assumed to be sorted in the same order as |rtp_modules|.
for (uint32_t ssrc : rtp_config_.ssrcs) {
@@ -700,7 +708,7 @@
DataSize max_total_packet_size = DataSize::bytes(
rtp_config_.max_packet_size + transport_overhead_bytes_per_packet_);
uint32_t payload_bitrate_bps = update.target_bitrate.bps();
- if (send_side_bwe_with_overhead_) {
+ if (send_side_bwe_with_overhead_ && has_packet_feedback_) {
DataRate overhead_rate = CalculateOverheadRate(
update.target_bitrate, max_total_packet_size, packet_overhead);
// TODO(srte): We probably should not accept 0 payload bitrate here.
@@ -736,7 +744,7 @@
loss_mask_vector_.clear();
uint32_t encoder_overhead_rate_bps = 0;
- if (send_side_bwe_with_overhead_) {
+ if (send_side_bwe_with_overhead_ && has_packet_feedback_) {
// TODO(srte): The packet size should probably be the same as in the
// CalculateOverheadRate call above (just max_total_packet_size), it doesn't
// make sense to use different packet rates for different overhead
diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h
index fb01f1b..eb7e431 100644
--- a/call/rtp_video_sender.h
+++ b/call/rtp_video_sender.h
@@ -163,6 +163,7 @@
const bool send_side_bwe_with_overhead_;
const bool account_for_packetization_overhead_;
const bool use_early_loss_detection_;
+ const bool has_packet_feedback_;
// TODO(holmer): Remove crit_ once RtpVideoSender runs on the
// transport task queue.
diff --git a/call/test/mock_rtp_transport_controller_send.h b/call/test/mock_rtp_transport_controller_send.h
index 04dac29..fad27b0 100644
--- a/call/test/mock_rtp_transport_controller_send.h
+++ b/call/test/mock_rtp_transport_controller_send.h
@@ -67,6 +67,7 @@
MOCK_METHOD1(SetClientBitratePreferences, void(const BitrateSettings&));
MOCK_METHOD1(OnTransportOverheadChanged, void(size_t));
MOCK_METHOD1(AccountForAudioPacketsInPacedSender, void(bool));
+ MOCK_METHOD0(IncludeOverheadInPacedSender, void());
MOCK_METHOD1(OnReceivedPacket, void(const ReceivedPacket&));
};
} // namespace webrtc
diff --git a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
index 44cfe9e..168bcec 100644
--- a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
+++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
@@ -593,6 +593,11 @@
ApplyAudioNetworkAdaptor();
}
+void AudioEncoderOpusImpl::OnReceivedTargetAudioBitrate(
+ int target_audio_bitrate_bps) {
+ SetTargetBitrate(target_audio_bitrate_bps);
+}
+
void AudioEncoderOpusImpl::OnReceivedUplinkBandwidth(
int target_audio_bitrate_bps,
absl::optional<int64_t> bwe_period_ms,
diff --git a/modules/audio_coding/codecs/opus/audio_encoder_opus.h b/modules/audio_coding/codecs/opus/audio_encoder_opus.h
index 66c489f..40fd167 100644
--- a/modules/audio_coding/codecs/opus/audio_encoder_opus.h
+++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.h
@@ -104,6 +104,7 @@
void DisableAudioNetworkAdaptor() override;
void OnReceivedUplinkPacketLossFraction(
float uplink_packet_loss_fraction) override;
+ void OnReceivedTargetAudioBitrate(int target_audio_bitrate_bps) override;
void OnReceivedUplinkBandwidth(
int target_audio_bitrate_bps,
absl::optional<int64_t> bwe_period_ms) override;
diff --git a/modules/pacing/paced_sender.cc b/modules/pacing/paced_sender.cc
index f6c85d4..6dc47b6 100644
--- a/modules/pacing/paced_sender.cc
+++ b/modules/pacing/paced_sender.cc
@@ -126,6 +126,11 @@
pacing_controller_.SetAccountForAudioPackets(account_for_audio);
}
+void PacedSender::SetIncludeOverhead() {
+ rtc::CritScope cs(&critsect_);
+ pacing_controller_.SetIncludeOverhead();
+}
+
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 06a6c26..3691308 100644
--- a/modules/pacing/paced_sender.h
+++ b/modules/pacing/paced_sender.h
@@ -97,6 +97,8 @@
// at high priority.
void SetAccountForAudioPackets(bool account_for_audio) override;
+ void SetIncludeOverhead() 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 e6dd7ac..09b7630 100644
--- a/modules/pacing/pacing_controller.cc
+++ b/modules/pacing/pacing_controller.cc
@@ -99,8 +99,6 @@
pace_audio_(IsEnabled(*field_trials_, "WebRTC-Pacer-BlockAudio")),
small_first_probe_packet_(
IsEnabled(*field_trials_, "WebRTC-Pacer-SmallFirstProbePacket")),
- send_side_bwe_with_overhead_(
- IsEnabled(*field_trials_, "WebRTC-SendSideBwe-WithOverhead")),
min_packet_limit_(kDefaultMinPacketLimit),
last_timestamp_(clock_->CurrentTime()),
paused_(false),
@@ -120,7 +118,8 @@
congestion_window_size_(DataSize::PlusInfinity()),
outstanding_data_(DataSize::Zero()),
queue_time_limit(kMaxExpectedQueueLength),
- account_for_audio_(false) {
+ account_for_audio_(false),
+ include_overhead_(false) {
if (!drain_large_queues_) {
RTC_LOG(LS_WARNING) << "Pacer queues will not be drained,"
"pushback experiment must be enabled.";
@@ -226,6 +225,11 @@
account_for_audio_ = account_for_audio;
}
+void PacingController::SetIncludeOverhead() {
+ include_overhead_ = true;
+ packet_queue_.SetIncludeOverhead();
+}
+
TimeDelta PacingController::ExpectedQueueTime() const {
RTC_DCHECK_GT(pacing_bitrate_, DataRate::Zero());
return TimeDelta::ms(
@@ -517,10 +521,10 @@
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(
- send_side_bwe_with_overhead_
- ? rtp_packet->size()
- : rtp_packet->payload_size() + rtp_packet->padding_size());
+ const DataSize packet_size =
+ DataSize::bytes(include_overhead_ ? rtp_packet->size()
+ : rtp_packet->payload_size() +
+ rtp_packet->padding_size());
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 f39887d..12e3612 100644
--- a/modules/pacing/pacing_controller.h
+++ b/modules/pacing/pacing_controller.h
@@ -107,6 +107,7 @@
// the pacer budget calculation. The audio traffic still will be injected
// at high priority.
void SetAccountForAudioPackets(bool account_for_audio);
+ void SetIncludeOverhead();
// Returns the time since the oldest queued packet was enqueued.
TimeDelta OldestPacketWaitTime() const;
@@ -176,7 +177,6 @@
const bool send_padding_if_silent_;
const bool pace_audio_;
const bool small_first_probe_packet_;
- const bool send_side_bwe_with_overhead_;
TimeDelta min_packet_limit_;
@@ -219,6 +219,7 @@
TimeDelta queue_time_limit;
bool account_for_audio_;
+ bool include_overhead_;
};
} // namespace webrtc
diff --git a/modules/pacing/round_robin_packet_queue.cc b/modules/pacing/round_robin_packet_queue.cc
index 16542b3..b9cc35d 100644
--- a/modules/pacing/round_robin_packet_queue.cc
+++ b/modules/pacing/round_robin_packet_queue.cc
@@ -114,8 +114,7 @@
max_size_(kMaxLeadingSize),
queue_time_sum_(TimeDelta::Zero()),
pause_time_sum_(TimeDelta::Zero()),
- send_side_bwe_with_overhead_(
- IsEnabled(field_trials, "WebRTC-SendSideBwe-WithOverhead")) {}
+ include_overhead_(false) {}
RoundRobinPacketQueue::~RoundRobinPacketQueue() {
// Make sure to release any packets owned by raw pointer in QueuedPacket.
@@ -158,7 +157,7 @@
// 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(send_side_bwe_with_overhead_);
+ DataSize packet_size = queued_packet.Size(include_overhead_);
stream->size =
std::max(stream->size + packet_size, max_size_ - kMaxLeadingSize);
max_size_ = std::max(max_size_, stream->size);
@@ -238,6 +237,10 @@
paused_ = paused;
}
+void RoundRobinPacketQueue::SetIncludeOverhead() {
+ include_overhead_ = true;
+}
+
TimeDelta RoundRobinPacketQueue::AverageQueueTime() const {
if (Empty())
return TimeDelta::Zero();
@@ -279,7 +282,7 @@
packet.SubtractPauseTime(pause_time_sum_);
size_packets_ += 1;
- size_ += packet.Size(send_side_bwe_with_overhead_);
+ size_ += packet.Size(include_overhead_);
stream->packet_queue.push(packet);
}
diff --git a/modules/pacing/round_robin_packet_queue.h b/modules/pacing/round_robin_packet_queue.h
index 96b458f..858f169 100644
--- a/modules/pacing/round_robin_packet_queue.h
+++ b/modules/pacing/round_robin_packet_queue.h
@@ -52,6 +52,7 @@
TimeDelta AverageQueueTime() const;
void UpdateQueueTime(Timestamp now);
void SetPauseState(bool paused, Timestamp now);
+ void SetIncludeOverhead();
private:
struct QueuedPacket {
@@ -150,7 +151,7 @@
// the age of the oldest packet in the queue.
std::multiset<Timestamp> enqueue_times_;
- const bool send_side_bwe_with_overhead_;
+ bool include_overhead_;
};
} // namespace webrtc
diff --git a/modules/pacing/rtp_packet_pacer.h b/modules/pacing/rtp_packet_pacer.h
index 305be54..2f11c1f 100644
--- a/modules/pacing/rtp_packet_pacer.h
+++ b/modules/pacing/rtp_packet_pacer.h
@@ -64,6 +64,7 @@
// the pacer budget calculation. The audio traffic still will be injected
// at high priority.
virtual void SetAccountForAudioPackets(bool account_for_audio) = 0;
+ virtual void SetIncludeOverhead() = 0;
};
} // namespace webrtc
diff --git a/modules/pacing/task_queue_paced_sender.cc b/modules/pacing/task_queue_paced_sender.cc
index e1745db..54d2d84 100644
--- a/modules/pacing/task_queue_paced_sender.cc
+++ b/modules/pacing/task_queue_paced_sender.cc
@@ -136,6 +136,13 @@
});
}
+void TaskQueuePacedSender::SetIncludeOverhead() {
+ task_queue_.PostTask([this]() {
+ RTC_DCHECK_RUN_ON(&task_queue_);
+ pacing_controller_.SetIncludeOverhead();
+ });
+}
+
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 719886a..a50ffa2 100644
--- a/modules/pacing/task_queue_paced_sender.h
+++ b/modules/pacing/task_queue_paced_sender.h
@@ -79,6 +79,7 @@
// at high priority.
void SetAccountForAudioPackets(bool account_for_audio) override;
+ void SetIncludeOverhead() override;
// Returns the time since the oldest queued packet was enqueued.
TimeDelta OldestPacketWaitTime() const override;
