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}

audio/audio_send_stream.cc

@@ -342,8 +342,6 @@ void AudioSendStream::Start() {
       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 @@ bool AudioSendStream::SetupSendCodec(const Config& new_config) {
   }
 
   // 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 "
@@ -693,8 +690,7 @@ void AudioSendStream::ReconfigureANA(const Config& new_config) {
       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_)) {
@@ -769,8 +765,6 @@ void AudioSendStream::ReconfigureBitrateObserver(
   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_);

audio/audio_send_stream_unittest.cc

@@ -490,8 +490,6 @@ TEST(AudioSendStreamTest, SendCodecAppliesAudioNetworkAdaptor) {
   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(_, _, _, _))

call/rtp_transport_controller_send.cc

@@ -434,10 +434,6 @@ void RtpTransportControllerSend::AccountForAudioPacketsInPacedSender(
   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());

call/rtp_transport_controller_send.h

@@ -107,7 +107,6 @@ class RtpTransportControllerSend final
       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;

call/rtp_transport_controller_send_interface.h

@@ -150,7 +150,6 @@ class RtpTransportControllerSendInterface {
       size_t transport_overhead_per_packet) = 0;
 
   virtual void AccountForAudioPacketsInPacedSender(bool account_for_audio) = 0;
-  virtual void IncludeOverheadInPacedSender() = 0;
 };
 
 }  // namespace webrtc

call/rtp_video_sender.cc

@@ -279,11 +279,6 @@ absl::optional<VideoCodecType> GetVideoCodecType(const RtpConfig& config) {
   }
   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 @@ RtpVideoSender::RtpVideoSender(
           "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 @@ RtpVideoSender::RtpVideoSender(
       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 @@ void RtpVideoSender::OnBitrateUpdated(BitrateAllocationUpdate update,
   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 @@ void RtpVideoSender::OnBitrateUpdated(BitrateAllocationUpdate update,
   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

call/rtp_video_sender.h

@@ -163,7 +163,6 @@ class RtpVideoSender : public RtpVideoSenderInterface,
   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.

call/test/mock_rtp_transport_controller_send.h

@@ -67,7 +67,6 @@ class MockRtpTransportControllerSend
   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

modules/audio_coding/codecs/opus/audio_encoder_opus.cc

@@ -593,11 +593,6 @@ void AudioEncoderOpusImpl::OnReceivedUplinkPacketLossFraction(
   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,

modules/audio_coding/codecs/opus/audio_encoder_opus.h

@@ -104,7 +104,6 @@ class AudioEncoderOpusImpl final : public AudioEncoder {
   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;

modules/pacing/paced_sender.cc

@@ -126,11 +126,6 @@ void PacedSender::SetAccountForAudioPackets(bool account_for_audio) {
   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();

modules/pacing/paced_sender.h

@@ -97,8 +97,6 @@ class PacedSender : public Module,
   // 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;
 

modules/pacing/pacing_controller.cc

@@ -99,6 +99,8 @@ PacingController::PacingController(Clock* clock,
       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 @@ PacingController::PacingController(Clock* clock,
       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.";
@@ -225,11 +226,6 @@ void PacingController::SetAccountForAudioPackets(bool account_for_audio) {
   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 @@ void PacingController::ProcessPackets() {
     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 =
+    const DataSize packet_size = DataSize::bytes(
-        DataSize::bytes(include_overhead_ ? rtp_packet->size()
+        send_side_bwe_with_overhead_
-                                          : rtp_packet->payload_size() +
+            ? rtp_packet->size()
-                                                rtp_packet->padding_size());
+            : rtp_packet->payload_size() + rtp_packet->padding_size());
     packet_sender_->SendRtpPacket(std::move(rtp_packet), pacing_info);
 
     data_sent += packet_size;

modules/pacing/pacing_controller.h

@@ -107,7 +107,6 @@ class PacingController {
   // 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 @@ class PacingController {
   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 @@ class PacingController {
 
   TimeDelta queue_time_limit;
   bool account_for_audio_;
-  bool include_overhead_;
 };
 }  // namespace webrtc
 

modules/pacing/round_robin_packet_queue.cc

@@ -114,7 +114,8 @@ RoundRobinPacketQueue::RoundRobinPacketQueue(
       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 @@ std::unique_ptr<RtpPacketToSend> RoundRobinPacketQueue::Pop() {
   // 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 @@ void RoundRobinPacketQueue::SetPauseState(bool paused, Timestamp now) {
   paused_ = paused;
 }
 
-void RoundRobinPacketQueue::SetIncludeOverhead() {
-  include_overhead_ = true;
-}
-
 TimeDelta RoundRobinPacketQueue::AverageQueueTime() const {
   if (Empty())
     return TimeDelta::Zero();
@@ -282,7 +279,7 @@ void RoundRobinPacketQueue::Push(QueuedPacket packet) {
   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);
 }

modules/pacing/round_robin_packet_queue.h

@@ -52,7 +52,6 @@ class RoundRobinPacketQueue {
   TimeDelta AverageQueueTime() const;
   void UpdateQueueTime(Timestamp now);
   void SetPauseState(bool paused, Timestamp now);
-  void SetIncludeOverhead();
 
  private:
   struct QueuedPacket {
@@ -151,7 +150,7 @@ class RoundRobinPacketQueue {
   // 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
 

modules/pacing/rtp_packet_pacer.h

@@ -64,7 +64,6 @@ class RtpPacketPacer {
   // 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

modules/pacing/task_queue_paced_sender.cc

@@ -136,13 +136,6 @@ void TaskQueuePacedSender::SetAccountForAudioPackets(bool account_for_audio) {
   });
 }
 
-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_);

modules/pacing/task_queue_paced_sender.h

@@ -79,7 +79,6 @@ class TaskQueuePacedSender : public RtpPacketPacer,
   // 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;