Reland "[Battery]: Delay start of TaskQueuePacedSender." Take 2

This is a reland of 89cb65ed663a9000b9f7c90a78039bd85731e9ae
... and f28aade91dcc2cb8f590dc1379ac7ab5c1981909

Reason for revert: crashes due to uninitialized pacing_bitrate_
crbug.com/1190547
Apparently pacer() is sometimes being used before EnsureStarted()
Fix: Instead of delaying first call to SetPacingRates(),
this CL no-ops MaybeProcessPackets() until EnsureStarted()
is called for the first time.

Original change's description:
> [Battery]: Delay start of TaskQueuePacedSender.
>
> To avoid unnecessary repeating tasks, TaskQueuePacedSender is started
> only upon RtpTransportControllerSend::EnsureStarted().
>
> More specifically, the repeating task happens in
> TaskQueuePacedSender::MaybeProcessPackets() every 500ms, using a self
> task_queue_.PostDelayedTask().
>
> Bug: chromium:1152887
> Change-Id: I72c96d2c4b491d5edb45a30b210b3797165cbf48
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/208560
> Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Reviewed-by: Erik Språng <sprang@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#33421}

Bug: chromium:1152887
Change-Id: I9aba4882a64bbee7d97ace9059dea8a24c144f93
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/212880
Reviewed-by: Erik Språng <sprang@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#33554}

call/rtp_transport_controller_send.cc

@@ -87,7 +87,7 @@ RtpTransportControllerSend::RtpTransportControllerSend(
     : clock_(clock),
       event_log_(event_log),
       bitrate_configurator_(bitrate_config),
-      process_thread_started_(false),
+      pacer_started_(false),
       process_thread_(std::move(process_thread)),
       use_task_queue_pacer_(IsEnabled(trials, "WebRTC-TaskQueuePacer")),
       process_thread_pacer_(use_task_queue_pacer_
@@ -496,9 +496,13 @@ void RtpTransportControllerSend::IncludeOverheadInPacedSender() {
 }
 
 void RtpTransportControllerSend::EnsureStarted() {
-  if (!use_task_queue_pacer_ && !process_thread_started_) {
-    process_thread_started_ = true;
-    process_thread_->Start();
+  if (!pacer_started_) {
+    pacer_started_ = true;
+    if (use_task_queue_pacer_) {
+      task_queue_pacer_->EnsureStarted();
+    } else {
+      process_thread_->Start();
+    }
   }
 }
 

call/rtp_transport_controller_send.h

@@ -152,7 +152,7 @@ class RtpTransportControllerSend final
   std::vector<std::unique_ptr<RtpVideoSenderInterface>> video_rtp_senders_;
   RtpBitrateConfigurator bitrate_configurator_;
   std::map<std::string, rtc::NetworkRoute> network_routes_;
-  bool process_thread_started_;
+  bool pacer_started_;
   const std::unique_ptr<ProcessThread> process_thread_;
   const bool use_task_queue_pacer_;
   std::unique_ptr<PacedSender> process_thread_pacer_;

modules/pacing/task_queue_paced_sender.cc

@@ -62,6 +62,14 @@ TaskQueuePacedSender::~TaskQueuePacedSender() {
   });
 }
 
+void TaskQueuePacedSender::EnsureStarted() {
+  task_queue_.PostTask([this]() {
+    RTC_DCHECK_RUN_ON(&task_queue_);
+    is_started_ = true;
+    MaybeProcessPackets(Timestamp::MinusInfinity());
+  });
+}
+
 void TaskQueuePacedSender::CreateProbeCluster(DataRate bitrate,
                                               int cluster_id) {
   task_queue_.PostTask([this, bitrate, cluster_id]() {
@@ -197,7 +205,7 @@ void TaskQueuePacedSender::MaybeProcessPackets(
     Timestamp scheduled_process_time) {
   RTC_DCHECK_RUN_ON(&task_queue_);
 
-  if (is_shutdown_) {
+  if (is_shutdown_ || !is_started_) {
     return;
   }
 

modules/pacing/task_queue_paced_sender.h

@@ -55,6 +55,9 @@ class TaskQueuePacedSender : public RtpPacketPacer, public RtpPacketSender {
 
   ~TaskQueuePacedSender() override;
 
+  // Ensure that necessary delayed tasks are scheduled.
+  void EnsureStarted();
+
   // Methods implementing RtpPacketSender.
 
   // Adds the packet to the queue and calls PacketRouter::SendPacket() when
@@ -150,6 +153,10 @@ class TaskQueuePacedSender : public RtpPacketPacer, public RtpPacketSender {
   // Last time stats were updated.
   Timestamp last_stats_time_ RTC_GUARDED_BY(task_queue_);
 
+  // Indicates if this task queue is started. If not, don't allow
+  // posting delayed tasks yet.
+  bool is_started_ RTC_GUARDED_BY(task_queue_) = false;
+
   // Indicates if this task queue is shutting down. If so, don't allow
   // posting any more delayed tasks as that can cause the task queue to
   // never drain.

modules/pacing/task_queue_paced_sender_unittest.cc

@@ -157,6 +157,7 @@ namespace test {
     pacer.SetPacingRates(
         DataRate::BitsPerSec(kDefaultPacketSize * 8 * kPacketsToSend),
         DataRate::Zero());
+    pacer.EnsureStarted();
     pacer.EnqueuePackets(
         GeneratePackets(RtpPacketMediaType::kVideo, kPacketsToSend));
 
@@ -196,6 +197,7 @@ namespace test {
     const DataRate kPacingRate =
         DataRate::BitsPerSec(kDefaultPacketSize * 8 * kPacketsPerSecond);
     pacer.SetPacingRates(kPacingRate, DataRate::Zero());
+    pacer.EnsureStarted();
 
     // Send some initial packets to be rid of any probes.
     EXPECT_CALL(packet_router, SendPacket).Times(kPacketsPerSecond);
@@ -247,6 +249,7 @@ namespace test {
     const TimeDelta kPacketPacingTime = kPacketSize / kPacingDataRate;
 
     pacer.SetPacingRates(kPacingDataRate, DataRate::Zero());
+    pacer.EnsureStarted();
 
     // Add some initial video packets, only one should be sent.
     EXPECT_CALL(packet_router, SendPacket);
@@ -280,6 +283,7 @@ namespace test {
     const DataRate kPacingDataRate = kPacketSize / kPacketPacingTime;
 
     pacer.SetPacingRates(kPacingDataRate, DataRate::Zero());
+    pacer.EnsureStarted();
 
     // Add 10 packets. The first should be sent immediately since the buffers
     // are clear.
@@ -316,6 +320,7 @@ namespace test {
     const DataRate kPacingDataRate = kPacketSize / kPacketPacingTime;
 
     pacer.SetPacingRates(kPacingDataRate, DataRate::Zero());
+    pacer.EnsureStarted();
 
     // Add 10 packets. The first should be sent immediately since the buffers
     // are clear. This will also trigger the probe to start.
@@ -342,6 +347,7 @@ namespace test {
         kCoalescingWindow);
     const DataRate kPacingDataRate = DataRate::KilobitsPerSec(300);
     pacer.SetPacingRates(kPacingDataRate, DataRate::Zero());
+    pacer.EnsureStarted();
 
     const TimeDelta kMinTimeBetweenStatsUpdates = TimeDelta::Millis(1);
 
@@ -388,6 +394,7 @@ namespace test {
     size_t num_expected_stats_updates = 0;
     EXPECT_EQ(pacer.num_stats_updates_, num_expected_stats_updates);
     pacer.SetPacingRates(kPacingDataRate, DataRate::Zero());
+    pacer.EnsureStarted();
     time_controller.AdvanceTime(kMinTimeBetweenStatsUpdates);
     // Updating pacing rates refreshes stats.
     EXPECT_EQ(pacer.num_stats_updates_, ++num_expected_stats_updates);
@@ -443,6 +450,7 @@ namespace test {
     const TimeDelta kPacketPacingTime = TimeDelta::Millis(4);
     const DataRate kPacingDataRate = kPacketSize / kPacketPacingTime;
     pacer.SetPacingRates(kPacingDataRate, /*padding_rate=*/DataRate::Zero());
+    pacer.EnsureStarted();
     EXPECT_CALL(packet_router, FetchFec).WillRepeatedly([]() {
       return std::vector<std::unique_ptr<RtpPacketToSend>>();
     });
@@ -514,6 +522,7 @@ namespace test {
     const TimeDelta kPacketPacingTime = TimeDelta::Millis(4);
     const DataRate kPacingDataRate = kPacketSize / kPacketPacingTime;
     pacer.SetPacingRates(kPacingDataRate, /*padding_rate=*/DataRate::Zero());
+    pacer.EnsureStarted();
     EXPECT_CALL(packet_router, FetchFec).WillRepeatedly([]() {
       return std::vector<std::unique_ptr<RtpPacketToSend>>();
     });
@@ -552,5 +561,33 @@ namespace test {
     EXPECT_EQ(data_sent,
               kProbingRate * TimeDelta::Millis(1) + DataSize::Bytes(1));
   }
+
+  TEST(TaskQueuePacedSenderTest, NoStatsUpdatesBeforeStart) {
+    const TimeDelta kCoalescingWindow = TimeDelta::Millis(5);
+    GlobalSimulatedTimeController time_controller(Timestamp::Millis(1234));
+    MockPacketRouter packet_router;
+    TaskQueuePacedSenderForTest pacer(
+        time_controller.GetClock(), &packet_router,
+        /*event_log=*/nullptr,
+        /*field_trials=*/nullptr, time_controller.GetTaskQueueFactory(),
+        kCoalescingWindow);
+    const DataRate kPacingDataRate = DataRate::KilobitsPerSec(300);
+    pacer.SetPacingRates(kPacingDataRate, DataRate::Zero());
+
+    const TimeDelta kMinTimeBetweenStatsUpdates = TimeDelta::Millis(1);
+
+    // Nothing inserted, no stats updates yet.
+    EXPECT_EQ(pacer.num_stats_updates_, 0u);
+
+    // Insert one packet, stats should not be updated.
+    pacer.EnqueuePackets(GeneratePackets(RtpPacketMediaType::kVideo, 1));
+    time_controller.AdvanceTime(TimeDelta::Zero());
+    EXPECT_EQ(pacer.num_stats_updates_, 0u);
+
+    // Advance time of the min stats update interval, and trigger a
+    // refresh - stats should not be updated still.
+    time_controller.AdvanceTime(kMinTimeBetweenStatsUpdates);
+    EXPECT_EQ(pacer.num_stats_updates_, 0u);
+  }
 }  // namespace test
 }  // namespace webrtc