Reland "Reland "Only include overhead if using send side bandwidth estimation.""
This is a reland of 086055d0fd9b9b9efe8bcf85884324a019e9bd33
ANA was accitendly disabled even when transport sequence numbers were
negotiated due to a bug in how the audio send stream is configured. To
solve this we simply continue to always allow enabling ANA and leave it
up to the application to ensure that it's not used together with receive
side estimation.
Original change's description:
> Reland "Only include overhead if using send side bandwidth estimation."
>
> This is a reland of 8c79c6e1af354c526497082c79ccbe12af03a33e
>
> 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}
>
> Bug: webrtc:11298
> Change-Id: I33205e869a8ae27c15ffe991f6d985973ed6d15a
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167524
> Reviewed-by: Ali Tofigh <alito@webrtc.org>
> Reviewed-by: Sam Zackrisson <saza@webrtc.org>
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
> Commit-Queue: Sebastian Jansson <srte@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#30390}
Bug: webrtc:11298
Change-Id: If2ad91e17ebfc85dc51edcd9607996e18c5d1f13
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167883
Commit-Queue: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30413}
diff --git a/audio/audio_send_stream.cc b/audio/audio_send_stream.cc
index 5e3b9ff..ba13fcb 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([&] {
@@ -765,6 +767,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 75c0aa3..fb4d9d3 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..754ff58 100644
--- a/modules/pacing/round_robin_packet_queue.cc
+++ b/modules/pacing/round_robin_packet_queue.cc
@@ -93,6 +93,16 @@
enqueue_time_ -= pause_time_sum;
}
+RoundRobinPacketQueue::PriorityPacketQueue::const_iterator
+RoundRobinPacketQueue::PriorityPacketQueue::begin() const {
+ return c.begin();
+}
+
+RoundRobinPacketQueue::PriorityPacketQueue::const_iterator
+RoundRobinPacketQueue::PriorityPacketQueue::end() const {
+ return c.end();
+}
+
RoundRobinPacketQueue::Stream::Stream() : size(DataSize::Zero()), ssrc(0) {}
RoundRobinPacketQueue::Stream::Stream(const Stream& stream) = default;
RoundRobinPacketQueue::Stream::~Stream() = default;
@@ -114,8 +124,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 +167,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 +247,17 @@
paused_ = paused;
}
+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_);
+ }
+ }
+}
+
TimeDelta RoundRobinPacketQueue::AverageQueueTime() const {
if (Empty())
return TimeDelta::Zero();
@@ -279,7 +299,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..d0a2f7c 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 {
@@ -89,6 +90,13 @@
RtpPacketToSend* owned_packet_;
};
+ class PriorityPacketQueue : public std::priority_queue<QueuedPacket> {
+ public:
+ using const_iterator = container_type::const_iterator;
+ const_iterator begin() const;
+ const_iterator end() const;
+ };
+
struct StreamPrioKey {
StreamPrioKey(int priority, DataSize size)
: priority(priority), size(size) {}
@@ -111,7 +119,8 @@
DataSize size;
uint32_t ssrc;
- std::priority_queue<QueuedPacket> packet_queue;
+
+ PriorityPacketQueue packet_queue;
// Whenever a packet is inserted for this stream we check if |priority_it|
// points to an element in |stream_priorities_|, and if it does it means
@@ -150,7 +159,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;
