Revert "Only include overhead if using send side bandwidth estimation."
This reverts commit 8c79c6e1af354c526497082c79ccbe12af03a33e.
Reason for revert: Introduced a Bug that can happen if the include overhead state changes between pushing and poping a packet from the pacer packet queue.
Original change's description:
> 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}
TBR=saza@webrtc.org,ossu@webrtc.org,sprang@webrtc.org,srte@webrtc.org,alito@webrtc.org
Change-Id: I0cacbc26408b7bec5bc3855a628e62407c081117
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:11298
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167523
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30383}
diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 79e08b7..5e3b9ff 100644
--- a/audio/audio_send_stream.cc
+++ b/audio/audio_send_stream.cc
@@ -342,8 +342,6 @@
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([&] {
@@ -593,8 +591,7 @@
}
// Enable ANA if configured (currently only used by Opus).
- if (new_config.audio_network_adaptor_config &&
- TransportSeqNumId(new_config) != 0) {
+ if (new_config.audio_network_adaptor_config) {
if (encoder->EnableAudioNetworkAdaptor(
*new_config.audio_network_adaptor_config, event_log_)) {
RTC_DLOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
@@ -693,8 +690,7 @@
config_.audio_network_adaptor_config) {
return;
}
- if (new_config.audio_network_adaptor_config &&
- TransportSeqNumId(new_config) != 0) {
+ if (new_config.audio_network_adaptor_config) {
channel_send_->CallEncoder([&](AudioEncoder* encoder) {
if (encoder->EnableAudioNetworkAdaptor(
*new_config.audio_network_adaptor_config, event_log_)) {
@@ -769,8 +765,6 @@
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 3b9fbb7..0472366 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -490,8 +490,6 @@
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 c2946ad..62b7008 100644
--- a/call/rtp_transport_controller_send.cc
+++ b/call/rtp_transport_controller_send.cc
@@ -434,10 +434,6 @@
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 b07bea7..f74c4e5 100644
--- a/call/rtp_transport_controller_send.h
+++ b/call/rtp_transport_controller_send.h
@@ -107,7 +107,6 @@
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 b40aabd..1e881dc 100644
--- a/call/rtp_transport_controller_send_interface.h
+++ b/call/rtp_transport_controller_send_interface.h
@@ -150,7 +150,6 @@
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 413171f..a926eb5 100644
--- a/call/rtp_video_sender.cc
+++ b/call/rtp_video_sender.cc
@@ -279,11 +279,6 @@
}
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(
@@ -306,7 +301,6 @@
"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)),
@@ -336,8 +330,6 @@
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) {
@@ -708,7 +700,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_ && has_packet_feedback_) {
+ if (send_side_bwe_with_overhead_) {
DataRate overhead_rate = CalculateOverheadRate(
update.target_bitrate, max_total_packet_size, packet_overhead);
// TODO(srte): We probably should not accept 0 payload bitrate here.
@@ -744,7 +736,7 @@
loss_mask_vector_.clear();
uint32_t encoder_overhead_rate_bps = 0;
- if (send_side_bwe_with_overhead_ && has_packet_feedback_) {
+ if (send_side_bwe_with_overhead_) {
// 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 eb7e431..fb01f1b 100644
--- a/call/rtp_video_sender.h
+++ b/call/rtp_video_sender.h
@@ -163,7 +163,6 @@
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 fad27b0..04dac29 100644
--- a/call/test/mock_rtp_transport_controller_send.h
+++ b/call/test/mock_rtp_transport_controller_send.h
@@ -67,7 +67,6 @@
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 168bcec..44cfe9e 100644
--- a/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
+++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.cc
@@ -593,11 +593,6 @@
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 40fd167..66c489f 100644
--- a/modules/audio_coding/codecs/opus/audio_encoder_opus.h
+++ b/modules/audio_coding/codecs/opus/audio_encoder_opus.h
@@ -104,7 +104,6 @@
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 6dc47b6..f6c85d4 100644
--- a/modules/pacing/paced_sender.cc
+++ b/modules/pacing/paced_sender.cc
@@ -126,11 +126,6 @@
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 3691308..06a6c26 100644
--- a/modules/pacing/paced_sender.h
+++ b/modules/pacing/paced_sender.h
@@ -97,8 +97,6 @@
// 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 09b7630..e6dd7ac 100644
--- a/modules/pacing/pacing_controller.cc
+++ b/modules/pacing/pacing_controller.cc
@@ -99,6 +99,8 @@
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),
@@ -118,8 +120,7 @@
congestion_window_size_(DataSize::PlusInfinity()),
outstanding_data_(DataSize::Zero()),
queue_time_limit(kMaxExpectedQueueLength),
- account_for_audio_(false),
- include_overhead_(false) {
+ account_for_audio_(false) {
if (!drain_large_queues_) {
RTC_LOG(LS_WARNING) << "Pacer queues will not be drained,"
"pushback experiment must be enabled.";
@@ -225,11 +226,6 @@
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(
@@ -521,10 +517,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(include_overhead_ ? rtp_packet->size()
- : rtp_packet->payload_size() +
- rtp_packet->padding_size());
+ const DataSize packet_size = DataSize::bytes(
+ send_side_bwe_with_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 12e3612..f39887d 100644
--- a/modules/pacing/pacing_controller.h
+++ b/modules/pacing/pacing_controller.h
@@ -107,7 +107,6 @@
// 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;
@@ -177,6 +176,7 @@
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,7 +219,6 @@
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 b9cc35d..16542b3 100644
--- a/modules/pacing/round_robin_packet_queue.cc
+++ b/modules/pacing/round_robin_packet_queue.cc
@@ -114,7 +114,8 @@
max_size_(kMaxLeadingSize),
queue_time_sum_(TimeDelta::Zero()),
pause_time_sum_(TimeDelta::Zero()),
- include_overhead_(false) {}
+ send_side_bwe_with_overhead_(
+ IsEnabled(field_trials, "WebRTC-SendSideBwe-WithOverhead")) {}
RoundRobinPacketQueue::~RoundRobinPacketQueue() {
// Make sure to release any packets owned by raw pointer in QueuedPacket.
@@ -157,7 +158,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(include_overhead_);
+ DataSize packet_size = queued_packet.Size(send_side_bwe_with_overhead_);
stream->size =
std::max(stream->size + packet_size, max_size_ - kMaxLeadingSize);
max_size_ = std::max(max_size_, stream->size);
@@ -237,10 +238,6 @@
paused_ = paused;
}
-void RoundRobinPacketQueue::SetIncludeOverhead() {
- include_overhead_ = true;
-}
-
TimeDelta RoundRobinPacketQueue::AverageQueueTime() const {
if (Empty())
return TimeDelta::Zero();
@@ -282,7 +279,7 @@
packet.SubtractPauseTime(pause_time_sum_);
size_packets_ += 1;
- size_ += packet.Size(include_overhead_);
+ size_ += packet.Size(send_side_bwe_with_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 858f169..96b458f 100644
--- a/modules/pacing/round_robin_packet_queue.h
+++ b/modules/pacing/round_robin_packet_queue.h
@@ -52,7 +52,6 @@
TimeDelta AverageQueueTime() const;
void UpdateQueueTime(Timestamp now);
void SetPauseState(bool paused, Timestamp now);
- void SetIncludeOverhead();
private:
struct QueuedPacket {
@@ -151,7 +150,7 @@
// the age of the oldest packet in the queue.
std::multiset<Timestamp> enqueue_times_;
- bool include_overhead_;
+ const bool send_side_bwe_with_overhead_;
};
} // namespace webrtc
diff --git a/modules/pacing/rtp_packet_pacer.h b/modules/pacing/rtp_packet_pacer.h
index 2f11c1f..305be54 100644
--- a/modules/pacing/rtp_packet_pacer.h
+++ b/modules/pacing/rtp_packet_pacer.h
@@ -64,7 +64,6 @@
// 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 54d2d84..e1745db 100644
--- a/modules/pacing/task_queue_paced_sender.cc
+++ b/modules/pacing/task_queue_paced_sender.cc
@@ -136,13 +136,6 @@
});
}
-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 a50ffa2..719886a 100644
--- a/modules/pacing/task_queue_paced_sender.h
+++ b/modules/pacing/task_queue_paced_sender.h
@@ -79,7 +79,6 @@
// 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;
