Revert "Preparation for ReceiveStatisticsProxy lock reduction."

This reverts commit 24eed27. Reason for revert: Speculative revert: breaks downstream project Original change's description: > Preparation for ReceiveStatisticsProxy lock reduction. > > Update tests to call VideoReceiveStream::GetStats() in the same or at > least similar way it gets called in production (construction thread, > same TQ/thread). > > Mapped out threads and context for ReceiveStatisticsProxy, > VideoQualityObserver and VideoReceiveStream. Added > follow-up TODOs for webrtc:11489. > > One functional change in ReceiveStatisticsProxy is that when sender > side RtcpPacketTypesCounterUpdated calls are made, the counter is > updated asynchronously since the sender calls the method on a different > thread than the receiver. > > Make CallClient::SendTask public to allow tests to run tasks in the > right context. CallClient already does this internally for GetStats. > > Remove 10 sec sleep in StopSendingKeyframeRequestsForInactiveStream. > > Bug: webrtc:11489 > Change-Id: Ib45bfc59d8472e9c5ea556e6ecf38298b8f14921 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/172847 > Commit-Queue: Tommi <[email protected]> > Reviewed-by: Mirko Bonadei <[email protected]> > Reviewed-by: Magnus Flodman <[email protected]> > Cr-Commit-Position: refs/heads/master@{#31008} [email protected],[email protected],[email protected],[email protected],[email protected],[email protected] # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:11489 Change-Id: I48b8359cdb791bf22b1a2c2c43d46263b01e0d65 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/173082 Reviewed-by: Artem Titov <[email protected]> Reviewed-by: Mirko Bonadei <[email protected]> Cr-Commit-Position: refs/heads/master@{#31023}
cdx08222028 · Apr 7, 2020 · 16cc9ef · 16cc9ef
1 parent 7e60483
commit 16cc9ef
Show file tree

Hide file tree

Showing 19 changed files with 100 additions and 655 deletions.
diff --git a/BUILD.gn b/BUILD.gn
@@ -547,7 +547,6 @@ if (rtc_include_tests) {
       "rtc_base:weak_ptr_unittests",
       "rtc_base/experiments:experiments_unittests",
       "rtc_base/synchronization:sequence_checker_unittests",
-      "rtc_base/task_utils:pending_task_safety_flag_unittests",
       "rtc_base/task_utils:to_queued_task_unittests",
       "sdk:sdk_tests",
       "test:rtp_test_utils",

diff --git a/call/call_perf_tests.cc b/call/call_perf_tests.cc
@@ -96,24 +96,21 @@ class VideoRtcpAndSyncObserver : public test::RtpRtcpObserver,
   static const int kMinRunTimeMs = 30000;
 
  public:
-  explicit VideoRtcpAndSyncObserver(TaskQueueBase* task_queue,
-                                    Clock* clock,
-                                    const std::string& test_label)
+  explicit VideoRtcpAndSyncObserver(Clock* clock, const std::string& test_label)
       : test::RtpRtcpObserver(CallPerfTest::kLongTimeoutMs),
         clock_(clock),
         test_label_(test_label),
         creation_time_ms_(clock_->TimeInMilliseconds()),
-        task_queue_(task_queue) {}
+        first_time_in_sync_(-1),
+        receive_stream_(nullptr) {}
 
   void OnFrame(const VideoFrame& video_frame) override {
-    task_queue_->PostTask(ToQueuedTask([this]() { CheckStats(); }));
-  }
-
-  void CheckStats() {
-    if (!receive_stream_)
-      return;
-
-    VideoReceiveStream::Stats stats = receive_stream_->GetStats();
+    VideoReceiveStream::Stats stats;
+    {
+      rtc::CritScope lock(&crit_);
+      if (receive_stream_)
+        stats = receive_stream_->GetStats();
+    }
     if (stats.sync_offset_ms == std::numeric_limits<int>::max())
       return;
 
@@ -138,8 +135,7 @@ class VideoRtcpAndSyncObserver : public test::RtpRtcpObserver,
   }
 
   void set_receive_stream(VideoReceiveStream* receive_stream) {
-    RTC_DCHECK_EQ(task_queue_, TaskQueueBase::Current());
-    // Note that receive_stream may be nullptr.
+    rtc::CritScope lock(&crit_);
     receive_stream_ = receive_stream;
   }
 
@@ -152,10 +148,10 @@ class VideoRtcpAndSyncObserver : public test::RtpRtcpObserver,
   Clock* const clock_;
   std::string test_label_;
   const int64_t creation_time_ms_;
-  int64_t first_time_in_sync_ = -1;
-  VideoReceiveStream* receive_stream_ = nullptr;
+  int64_t first_time_in_sync_;
+  rtc::CriticalSection crit_;
+  VideoReceiveStream* receive_stream_ RTC_GUARDED_BY(crit_);
   std::vector<double> sync_offset_ms_list_;
-  TaskQueueBase* const task_queue_;
 };
 
 void CallPerfTest::TestAudioVideoSync(FecMode fec,
@@ -172,8 +168,7 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
   audio_net_config.queue_delay_ms = 500;
   audio_net_config.loss_percent = 5;
 
-  auto observer = std::make_unique<VideoRtcpAndSyncObserver>(
-      task_queue(), Clock::GetRealTimeClock(), test_label);
+  VideoRtcpAndSyncObserver observer(Clock::GetRealTimeClock(), test_label);
 
   std::map<uint8_t, MediaType> audio_pt_map;
   std::map<uint8_t, MediaType> video_pt_map;
@@ -223,23 +218,23 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
                  });
 
     audio_send_transport = std::make_unique<test::PacketTransport>(
-        task_queue(), sender_call_.get(), observer.get(),
+        task_queue(), sender_call_.get(), &observer,
         test::PacketTransport::kSender, audio_pt_map,
         std::make_unique<FakeNetworkPipe>(
             Clock::GetRealTimeClock(),
             std::make_unique<SimulatedNetwork>(audio_net_config)));
     audio_send_transport->SetReceiver(receiver_call_->Receiver());
 
     video_send_transport = std::make_unique<test::PacketTransport>(
-        task_queue(), sender_call_.get(), observer.get(),
+        task_queue(), sender_call_.get(), &observer,
         test::PacketTransport::kSender, video_pt_map,
         std::make_unique<FakeNetworkPipe>(Clock::GetRealTimeClock(),
                                           std::make_unique<SimulatedNetwork>(
                                               BuiltInNetworkBehaviorConfig())));
     video_send_transport->SetReceiver(receiver_call_->Receiver());
 
     receive_transport = std::make_unique<test::PacketTransport>(
-        task_queue(), receiver_call_.get(), observer.get(),
+        task_queue(), receiver_call_.get(), &observer,
         test::PacketTransport::kReceiver, payload_type_map_,
         std::make_unique<FakeNetworkPipe>(Clock::GetRealTimeClock(),
                                           std::make_unique<SimulatedNetwork>(
@@ -264,7 +259,7 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
       video_receive_configs_[0].rtp.ulpfec_payload_type = kUlpfecPayloadType;
     }
     video_receive_configs_[0].rtp.nack.rtp_history_ms = 1000;
-    video_receive_configs_[0].renderer = observer.get();
+    video_receive_configs_[0].renderer = &observer;
     video_receive_configs_[0].sync_group = kSyncGroup;
 
     AudioReceiveStream::Config audio_recv_config;
@@ -286,7 +281,7 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
           receiver_call_->CreateAudioReceiveStream(audio_recv_config);
     }
     EXPECT_EQ(1u, video_receive_streams_.size());
-    observer->set_receive_stream(video_receive_streams_[0]);
+    observer.set_receive_stream(video_receive_streams_[0]);
     drifting_clock = std::make_unique<DriftingClock>(clock_, video_ntp_speed);
     CreateFrameGeneratorCapturerWithDrift(drifting_clock.get(), video_rtp_speed,
                                           kDefaultFramerate, kDefaultWidth,
@@ -298,13 +293,10 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
     audio_receive_stream->Start();
   });
 
-  EXPECT_TRUE(observer->Wait())
+  EXPECT_TRUE(observer.Wait())
       << "Timed out while waiting for audio and video to be synchronized.";
 
   SendTask(RTC_FROM_HERE, task_queue(), [&]() {
-    // Clear the pointer to the receive stream since it will now be deleted.
-    observer->set_receive_stream(nullptr);
-
     audio_send_stream->Stop();
     audio_receive_stream->Stop();
 
@@ -322,7 +314,7 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
     DestroyCalls();
   });
 
-  observer->PrintResults();
+  observer.PrintResults();
 
   // In quick test synchronization may not be achieved in time.
   if (!field_trial::IsEnabled("WebRTC-QuickPerfTest")) {
@@ -331,9 +323,6 @@ void CallPerfTest::TestAudioVideoSync(FecMode fec,
     EXPECT_METRIC_EQ(1, metrics::NumSamples("WebRTC.Video.AVSyncOffsetInMs"));
 #endif
   }
-
-  task_queue()->PostTask(
-      ToQueuedTask([to_delete = observer.release()]() { delete to_delete; }));
 }
 
 TEST_F(CallPerfTest, PlaysOutAudioAndVideoInSyncWithoutClockDrift) {

diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc
@@ -510,9 +510,9 @@ TEST(RtpVideoSenderTest, RetransmitsOnTransportWideLossInfo) {
   test::NetworkSimulationConfig net_conf;
   net_conf.bandwidth = DataRate::KilobitsPerSec(300);
   auto send_node = s.CreateSimulationNode(net_conf);
-  auto* callee = s.CreateClient("return", call_conf);
   auto* route = s.CreateRoutes(s.CreateClient("send", call_conf), {send_node},
-                               callee, {s.CreateSimulationNode(net_conf)});
+                               s.CreateClient("return", call_conf),
+                               {s.CreateSimulationNode(net_conf)});
 
   test::VideoStreamConfig lossy_config;
   lossy_config.source.framerate = 5;
@@ -540,20 +540,14 @@ TEST(RtpVideoSenderTest, RetransmitsOnTransportWideLossInfo) {
   // from initial probing.
   s.RunFor(TimeDelta::Seconds(1));
   rtx_packets = 0;
-  int decoded_baseline = 0;
-  callee->SendTask([&decoded_baseline, &lossy]() {
-    decoded_baseline = lossy->receive()->GetStats().frames_decoded;
-  });
+  int decoded_baseline = lossy->receive()->GetStats().frames_decoded;
   s.RunFor(TimeDelta::Seconds(1));
   // We expect both that RTX packets were sent and that an appropriate number of
   // frames were received. This is somewhat redundant but reduces the risk of
   // false positives in future regressions (e.g. RTX is send due to probing).
   EXPECT_GE(rtx_packets, 1);
-  int frames_decoded = 0;
-  callee->SendTask([&decoded_baseline, &frames_decoded, &lossy]() {
-    frames_decoded =
-        lossy->receive()->GetStats().frames_decoded - decoded_baseline;
-  });
+  int frames_decoded =
+      lossy->receive()->GetStats().frames_decoded - decoded_baseline;
   EXPECT_EQ(frames_decoded, 5);
 }
 

diff --git a/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc b/modules/congestion_controller/goog_cc/goog_cc_network_control_unittest.cc
@@ -537,8 +537,8 @@ DataRate AverageBitrateAfterCrossInducedLoss(std::string name) {
   auto ret_net = {s.CreateSimulationNode(net_conf)};
 
   auto* client = s.CreateClient("send", CallClientConfig());
-  auto* callee = s.CreateClient("return", CallClientConfig());
-  auto* route = s.CreateRoutes(client, send_net, callee, ret_net);
+  auto* route = s.CreateRoutes(
+      client, send_net, s.CreateClient("return", CallClientConfig()), ret_net);
   // TODO(srte): Make this work with RTX enabled or remove it.
   auto* video = s.CreateVideoStream(route->forward(), [](VideoStreamConfig* c) {
     c->stream.use_rtx = false;
@@ -553,17 +553,9 @@ DataRate AverageBitrateAfterCrossInducedLoss(std::string name) {
     s.net()->StopCrossTraffic(tcp_traffic);
     s.RunFor(TimeDelta::Seconds(20));
   }
-
-  // Querying the video stats from within the expected runtime environment
-  // (i.e. the TQ that belongs to the CallClient, not the Scenario TQ that
-  // we're currently on).
-  VideoReceiveStream::Stats video_receive_stats;
-  auto* video_stream = video->receive();
-  callee->SendTask([&video_stream, &video_receive_stats]() {
-    video_receive_stats = video_stream->GetStats();
-  });
-  return DataSize::Bytes(
-             video_receive_stats.rtp_stats.packet_counter.TotalBytes()) /
+  return DataSize::Bytes(video->receive()
+                             ->GetStats()
+                             .rtp_stats.packet_counter.TotalBytes()) /
          s.TimeSinceStart();
 }
 

diff --git a/rtc_base/task_utils/BUILD.gn b/rtc_base/task_utils/BUILD.gn
@@ -26,39 +26,12 @@ rtc_library("repeating_task") {
   ]
 }
 
-rtc_library("pending_task_safety_flag") {
-  sources = [
-    "pending_task_safety_flag.cc",
-    "pending_task_safety_flag.h",
-  ]
-  deps = [
-    "..:checks",
-    "..:refcount",
-    "..:thread_checker",
-    "../../api:scoped_refptr",
-    "../synchronization:sequence_checker",
-  ]
-}
-
 rtc_source_set("to_queued_task") {
   sources = [ "to_queued_task.h" ]
   deps = [ "../../api/task_queue" ]
 }
 
 if (rtc_include_tests) {
-  rtc_library("pending_task_safety_flag_unittests") {
-    testonly = true
-    sources = [ "pending_task_safety_flag_unittest.cc" ]
-    deps = [
-      ":pending_task_safety_flag",
-      ":to_queued_task",
-      "..:rtc_base_approved",
-      "..:rtc_task_queue",
-      "..:task_queue_for_test",
-      "../../test:test_support",
-    ]
-  }
-
   rtc_library("repeating_task_unittests") {
     testonly = true
     sources = [ "repeating_task_unittest.cc" ]

diff --git a/rtc_base/task_utils/pending_task_safety_flag.cc b/rtc_base/task_utils/pending_task_safety_flag.cc
diff --git a/rtc_base/task_utils/pending_task_safety_flag.h b/rtc_base/task_utils/pending_task_safety_flag.h