Revert "Remove Invoke from VideoChannel::FillBitrateInfo."

This reverts commit 1a17957. Reason for revert: Speculative revert (breaks downstream project). Original change's description: > Remove Invoke from VideoChannel::FillBitrateInfo. > > The method is relied upon by StatsCollector where it was called from the > signaling thread in a loop. Now there's at most one invoke (not N). > > Uncommenting thread checks and removing TODOs in SendStatisticsProxy, > VideoSendStream. Updating all related tests that fetched stats from > the wrong context. > > Bug: webrtc:12726 > Change-Id: Ia7db1afd7e103ec4f9816f5647203c4e2495586e > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/216688 > Commit-Queue: Tommi <[email protected]> > Reviewed-by: Niels Moller <[email protected]> > Reviewed-by: Ilya Nikolaevskiy <[email protected]> > Cr-Commit-Position: refs/heads/master@{#33894} [email protected],[email protected],[email protected],[email protected] Change-Id: I2520957cdb33492d187f04320c7416788fd0f820 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:12726 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217240 Reviewed-by: Mirko Bonadei <[email protected]> Commit-Queue: Mirko Bonadei <[email protected]> Cr-Commit-Position: refs/heads/master@{#33898}
facedawn · May 3, 2021 · 48a4d33 · 48a4d33
1 parent 2694672
commit 48a4d33
Show file tree

Hide file tree

Showing 13 changed files with 71 additions and 134 deletions.
diff --git a/call/call_perf_tests.cc b/call/call_perf_tests.cc
@@ -654,8 +654,7 @@ void CallPerfTest::TestMinTransmitBitrate(bool pad_to_min_bitrate) {
   static const int kAcceptableBitrateErrorMargin = 15;  // +- 7
   class BitrateObserver : public test::EndToEndTest {
    public:
-    explicit BitrateObserver(bool using_min_transmit_bitrate,
-                             TaskQueueBase* task_queue)
+    explicit BitrateObserver(bool using_min_transmit_bitrate)
         : EndToEndTest(kLongTimeoutMs),
           send_stream_(nullptr),
           converged_(false),
@@ -668,31 +667,27 @@ void CallPerfTest::TestMinTransmitBitrate(bool pad_to_min_bitrate) {
                                       ? kMaxAcceptableTransmitBitrate
                                       : (kMaxEncodeBitrateKbps +
                                          kAcceptableBitrateErrorMargin / 2)),
-          num_bitrate_observations_in_range_(0),
-          task_queue_(task_queue) {}
+          num_bitrate_observations_in_range_(0) {}
 
    private:
     // TODO(holmer): Run this with a timer instead of once per packet.
     Action OnSendRtp(const uint8_t* packet, size_t length) override {
-      task_queue_->PostTask(ToQueuedTask([this]() {
-        VideoSendStream::Stats stats = send_stream_->GetStats();
-
-        if (!stats.substreams.empty()) {
-          RTC_DCHECK_EQ(1, stats.substreams.size());
-          int bitrate_kbps =
-              stats.substreams.begin()->second.total_bitrate_bps / 1000;
-          if (bitrate_kbps > min_acceptable_bitrate_ &&
-              bitrate_kbps < max_acceptable_bitrate_) {
-            converged_ = true;
-            ++num_bitrate_observations_in_range_;
-            if (num_bitrate_observations_in_range_ ==
-                kNumBitrateObservationsInRange)
-              observation_complete_.Set();
-          }
-          if (converged_)
-            bitrate_kbps_list_.push_back(bitrate_kbps);
+      VideoSendStream::Stats stats = send_stream_->GetStats();
+      if (!stats.substreams.empty()) {
+        RTC_DCHECK_EQ(1, stats.substreams.size());
+        int bitrate_kbps =
+            stats.substreams.begin()->second.total_bitrate_bps / 1000;
+        if (bitrate_kbps > min_acceptable_bitrate_ &&
+            bitrate_kbps < max_acceptable_bitrate_) {
+          converged_ = true;
+          ++num_bitrate_observations_in_range_;
+          if (num_bitrate_observations_in_range_ ==
+              kNumBitrateObservationsInRange)
+            observation_complete_.Set();
         }
-      }));
+        if (converged_)
+          bitrate_kbps_list_.push_back(bitrate_kbps);
+      }
       return SEND_PACKET;
     }
 
@@ -729,8 +724,7 @@ void CallPerfTest::TestMinTransmitBitrate(bool pad_to_min_bitrate) {
     const int max_acceptable_bitrate_;
     int num_bitrate_observations_in_range_;
     std::vector<double> bitrate_kbps_list_;
-    TaskQueueBase* task_queue_;
-  } test(pad_to_min_bitrate, task_queue());
+  } test(pad_to_min_bitrate);
 
   fake_encoder_max_bitrate_ = kMaxEncodeBitrateKbps;
   RunBaseTest(&test);
@@ -781,7 +775,7 @@ TEST_F(CallPerfTest, MAYBE_KeepsHighBitrateWhenReconfiguringSender) {
 
   class BitrateObserver : public test::EndToEndTest, public test::FakeEncoder {
    public:
-    explicit BitrateObserver(TaskQueueBase* task_queue)
+    BitrateObserver()
         : EndToEndTest(kDefaultTimeoutMs),
           FakeEncoder(Clock::GetRealTimeClock()),
           encoder_inits_(0),
@@ -790,8 +784,7 @@ TEST_F(CallPerfTest, MAYBE_KeepsHighBitrateWhenReconfiguringSender) {
           frame_generator_(nullptr),
           encoder_factory_(this),
           bitrate_allocator_factory_(
-              CreateBuiltinVideoBitrateAllocatorFactory()),
-          task_queue_(task_queue) {}
+              CreateBuiltinVideoBitrateAllocatorFactory()) {}
 
     int32_t InitEncode(const VideoCodec* config,
                        const VideoEncoder::Settings& settings) override {
@@ -861,9 +854,7 @@ TEST_F(CallPerfTest, MAYBE_KeepsHighBitrateWhenReconfiguringSender) {
       ASSERT_TRUE(time_to_reconfigure_.Wait(kDefaultTimeoutMs))
           << "Timed out before receiving an initial high bitrate.";
       frame_generator_->ChangeResolution(kDefaultWidth * 2, kDefaultHeight * 2);
-      SendTask(RTC_FROM_HERE, task_queue_, [&]() {
-        send_stream_->ReconfigureVideoEncoder(encoder_config_.Copy());
-      });
+      send_stream_->ReconfigureVideoEncoder(encoder_config_.Copy());
       EXPECT_TRUE(Wait())
           << "Timed out while waiting for a couple of high bitrate estimates "
              "after reconfiguring the send stream.";
@@ -878,8 +869,7 @@ TEST_F(CallPerfTest, MAYBE_KeepsHighBitrateWhenReconfiguringSender) {
     test::VideoEncoderProxyFactory encoder_factory_;
     std::unique_ptr<VideoBitrateAllocatorFactory> bitrate_allocator_factory_;
     VideoEncoderConfig encoder_config_;
-    TaskQueueBase* task_queue_;
-  } test(task_queue());
+  } test;
 
   RunBaseTest(&test);
 }

diff --git a/call/rampup_tests.cc b/call/rampup_tests.cc
@@ -370,10 +370,7 @@ void RampUpTester::TriggerTestDone() {
   if (!send_stream_)
     return;
 
-  VideoSendStream::Stats send_stats;
-  SendTask(RTC_FROM_HERE, task_queue_,
-           [&] { send_stats = send_stream_->GetStats(); });
-
+  VideoSendStream::Stats send_stats = send_stream_->GetStats();
   send_stream_ = nullptr;  // To avoid dereferencing a bad pointer.
 
   size_t total_packets_sent = 0;

diff --git a/common_video/h264/h264_bitstream_parser.cc b/common_video/h264/h264_bitstream_parser.cc
@@ -275,14 +275,14 @@ void H264BitstreamParser::ParseSlice(const uint8_t* slice, size_t length) {
       sps_ = SpsParser::ParseSps(slice + H264::kNaluTypeSize,
                                  length - H264::kNaluTypeSize);
       if (!sps_)
-        RTC_DLOG(LS_WARNING) << "Unable to parse SPS from H264 bitstream.";
+        RTC_LOG(LS_WARNING) << "Unable to parse SPS from H264 bitstream.";
       break;
     }
     case H264::NaluType::kPps: {
       pps_ = PpsParser::ParsePps(slice + H264::kNaluTypeSize,
                                  length - H264::kNaluTypeSize);
       if (!pps_)
-        RTC_DLOG(LS_WARNING) << "Unable to parse PPS from H264 bitstream.";
+        RTC_LOG(LS_WARNING) << "Unable to parse PPS from H264 bitstream.";
       break;
     }
     case H264::NaluType::kAud:
@@ -291,7 +291,7 @@ void H264BitstreamParser::ParseSlice(const uint8_t* slice, size_t length) {
     default:
       Result res = ParseNonParameterSetNalu(slice, length, nalu_type);
       if (res != kOk)
-        RTC_DLOG(LS_INFO) << "Failed to parse bitstream. Error: " << res;
+        RTC_LOG(LS_INFO) << "Failed to parse bitstream. Error: " << res;
       break;
   }
 }

diff --git a/pc/channel.cc b/pc/channel.cc
@@ -1067,9 +1067,9 @@ void VideoChannel::UpdateMediaSendRecvState_w() {
 }
 
 void VideoChannel::FillBitrateInfo(BandwidthEstimationInfo* bwe_info) {
-  RTC_DCHECK_RUN_ON(worker_thread());
   VideoMediaChannel* mc = media_channel();
-  mc->FillBitrateInfo(bwe_info);
+  InvokeOnWorker<void>(RTC_FROM_HERE,
+                       [mc, bwe_info] { mc->FillBitrateInfo(bwe_info); });
 }
 
 bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,

diff --git a/pc/stats_collector.cc b/pc/stats_collector.cc
@@ -1026,25 +1026,16 @@ void StatsCollector::ExtractBweInfo() {
 
   // Fill in target encoder bitrate, actual encoder bitrate, rtx bitrate, etc.
   // TODO(holmer): Also fill this in for audio.
-  auto transceivers = pc_->GetTransceiversInternal();
-  std::vector<cricket::VideoChannel*> video_channels;
-  for (const auto& transceiver : transceivers) {
+  for (const auto& transceiver : pc_->GetTransceiversInternal()) {
     if (transceiver->media_type() != cricket::MEDIA_TYPE_VIDEO) {
       continue;
     }
     auto* video_channel =
         static_cast<cricket::VideoChannel*>(transceiver->internal()->channel());
-    if (video_channel) {
-      video_channels.push_back(video_channel);
+    if (!video_channel) {
+      continue;
     }
-  }
-
-  if (!video_channels.empty()) {
-    pc_->worker_thread()->Invoke<void>(RTC_FROM_HERE, [&] {
-      for (const auto& channel : video_channels) {
-        channel->FillBitrateInfo(&bwe_info);
-      }
-    });
+    video_channel->FillBitrateInfo(&bwe_info);
   }
 
   StatsReport::Id report_id(StatsReport::NewBandwidthEstimationId());

diff --git a/test/scenario/scenario_unittest.cc b/test/scenario/scenario_unittest.cc
@@ -182,11 +182,7 @@ TEST(ScenarioTest,
   s.RunFor(TimeDelta::Seconds(10));
   // Make sure retransmissions have happened.
   int retransmit_packets = 0;
-
-  VideoSendStream::Stats stats;
-  alice->SendTask([&]() { stats = video->send()->GetStats(); });
-
-  for (const auto& substream : stats.substreams) {
+  for (const auto& substream : video->send()->GetStats().substreams) {
     retransmit_packets += substream.second.rtp_stats.retransmitted.packets;
   }
   EXPECT_GT(retransmit_packets, 0);

diff --git a/test/scenario/stats_collection_unittest.cc b/test/scenario/stats_collection_unittest.cc
@@ -33,14 +33,8 @@ void CreateAnalyzedStream(Scenario* s,
   auto* audio = s->CreateAudioStream(route->forward(), AudioStreamConfig());
   s->Every(TimeDelta::Seconds(1), [=] {
     collectors->call.AddStats(caller->GetStats());
-
-    VideoSendStream::Stats send_stats;
-    caller->SendTask([&]() { send_stats = video->send()->GetStats(); });
-    collectors->video_send.AddStats(send_stats, s->Now());
-
-    AudioReceiveStream::Stats receive_stats;
-    caller->SendTask([&]() { receive_stats = audio->receive()->GetStats(); });
-    collectors->audio_receive.AddStats(receive_stats);
+    collectors->video_send.AddStats(video->send()->GetStats(), s->Now());
+    collectors->audio_receive.AddStats(audio->receive()->GetStats());
 
     // Querying the video stats from within the expected runtime environment
     // (i.e. the TQ that belongs to the CallClient, not the Scenario TQ that

diff --git a/test/scenario/video_stream_unittest.cc b/test/scenario/video_stream_unittest.cc
@@ -130,9 +130,7 @@ TEST(VideoStreamTest, SendsNacksOnLoss) {
   auto video = s.CreateVideoStream(route->forward(), VideoStreamConfig());
   s.RunFor(TimeDelta::Seconds(1));
   int retransmit_packets = 0;
-  VideoSendStream::Stats stats;
-  route->first()->SendTask([&]() { stats = video->send()->GetStats(); });
-  for (const auto& substream : stats.substreams) {
+  for (const auto& substream : video->send()->GetStats().substreams) {
     retransmit_packets += substream.second.rtp_stats.retransmitted.packets;
   }
   EXPECT_GT(retransmit_packets, 0);
@@ -154,8 +152,7 @@ TEST(VideoStreamTest, SendsFecWithUlpFec) {
     c->stream.use_ulpfec = true;
   });
   s.RunFor(TimeDelta::Seconds(5));
-  VideoSendStream::Stats video_stats;
-  route->first()->SendTask([&]() { video_stats = video->send()->GetStats(); });
+  VideoSendStream::Stats video_stats = video->send()->GetStats();
   EXPECT_GT(video_stats.substreams.begin()->second.rtp_stats.fec.packets, 0u);
 }
 TEST(VideoStreamTest, SendsFecWithFlexFec) {
@@ -172,8 +169,7 @@ TEST(VideoStreamTest, SendsFecWithFlexFec) {
     c->stream.use_flexfec = true;
   });
   s.RunFor(TimeDelta::Seconds(5));
-  VideoSendStream::Stats video_stats;
-  route->first()->SendTask([&]() { video_stats = video->send()->GetStats(); });
+  VideoSendStream::Stats video_stats = video->send()->GetStats();
   EXPECT_GT(video_stats.substreams.begin()->second.rtp_stats.fec.packets, 0u);
 }
 

diff --git a/video/end_to_end_tests/ssrc_tests.cc b/video/end_to_end_tests/ssrc_tests.cc
@@ -132,15 +132,13 @@ void SsrcEndToEndTest::TestSendsSetSsrcs(size_t num_ssrcs,
    public:
     SendsSetSsrcs(const uint32_t* ssrcs,
                   size_t num_ssrcs,
-                  bool send_single_ssrc_first,
-                  TaskQueueBase* task_queue)
+                  bool send_single_ssrc_first)
         : EndToEndTest(kDefaultTimeoutMs),
           num_ssrcs_(num_ssrcs),
           send_single_ssrc_first_(send_single_ssrc_first),
           ssrcs_to_observe_(num_ssrcs),
           expect_single_ssrc_(send_single_ssrc_first),
-          send_stream_(nullptr),
-          task_queue_(task_queue) {
+          send_stream_(nullptr) {
       for (size_t i = 0; i < num_ssrcs; ++i)
         valid_ssrcs_[ssrcs[i]] = true;
     }
@@ -202,10 +200,8 @@ void SsrcEndToEndTest::TestSendsSetSsrcs(size_t num_ssrcs,
 
       if (send_single_ssrc_first_) {
         // Set full simulcast and continue with the rest of the SSRCs.
-        SendTask(RTC_FROM_HERE, task_queue_, [&]() {
-          send_stream_->ReconfigureVideoEncoder(
-              std::move(video_encoder_config_all_streams_));
-        });
+        send_stream_->ReconfigureVideoEncoder(
+            std::move(video_encoder_config_all_streams_));
         EXPECT_TRUE(Wait()) << "Timed out while waiting on additional SSRCs.";
       }
     }
@@ -222,8 +218,7 @@ void SsrcEndToEndTest::TestSendsSetSsrcs(size_t num_ssrcs,
 
     VideoSendStream* send_stream_;
     VideoEncoderConfig video_encoder_config_all_streams_;
-    TaskQueueBase* task_queue_;
-  } test(kVideoSendSsrcs, num_ssrcs, send_single_ssrc_first, task_queue());
+  } test(kVideoSendSsrcs, num_ssrcs, send_single_ssrc_first);
 
   RunBaseTest(&test);
 }

diff --git a/video/end_to_end_tests/stats_tests.cc b/video/end_to_end_tests/stats_tests.cc
@@ -154,10 +154,7 @@ TEST_F(StatsEndToEndTest, GetStats) {
 
     bool CheckSendStats() {
       RTC_DCHECK(send_stream_);
-
-      VideoSendStream::Stats stats;
-      SendTask(RTC_FROM_HERE, task_queue_,
-               [&]() { stats = send_stream_->GetStats(); });
+      VideoSendStream::Stats stats = send_stream_->GetStats();
 
       size_t expected_num_streams =
           kNumSimulcastStreams + expected_send_ssrcs_.size();

diff --git a/video/send_statistics_proxy.cc b/video/send_statistics_proxy.cc
@@ -670,7 +670,6 @@ void SendStatisticsProxy::UmaSamplesContainer::UpdateHistograms(
 void SendStatisticsProxy::OnEncoderReconfigured(
     const VideoEncoderConfig& config,
     const std::vector<VideoStream>& streams) {
-  // Called on VideoStreamEncoder's encoder_queue_.
   MutexLock lock(&mutex_);
 
   if (content_type_ != config.content_type) {

diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc
@@ -169,7 +169,7 @@ void VideoSendStream::UpdateActiveSimulcastLayers(
 
 void VideoSendStream::Start() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
-  RTC_DLOG(LS_INFO) << "VideoSendStream::Start";
+  RTC_LOG(LS_INFO) << "VideoSendStream::Start";
   VideoSendStreamImpl* send_stream = send_stream_.get();
   worker_queue_->PostTask([this, send_stream] {
     send_stream->Start();
@@ -184,7 +184,7 @@ void VideoSendStream::Start() {
 
 void VideoSendStream::Stop() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
-  RTC_DLOG(LS_INFO) << "VideoSendStream::Stop";
+  RTC_LOG(LS_INFO) << "VideoSendStream::Stop";
   VideoSendStreamImpl* send_stream = send_stream_.get();
   worker_queue_->PostTask([send_stream] { send_stream->Stop(); });
 }
@@ -209,15 +209,19 @@ void VideoSendStream::SetSource(
 }
 
 void VideoSendStream::ReconfigureVideoEncoder(VideoEncoderConfig config) {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
-  RTC_DCHECK_EQ(content_type_, config.content_type);
+  // TODO(perkj): Some test cases in VideoSendStreamTest call
+  // ReconfigureVideoEncoder from the network thread.
+  // RTC_DCHECK_RUN_ON(&thread_checker_);
+  RTC_DCHECK(content_type_ == config.content_type);
   video_stream_encoder_->ConfigureEncoder(
       std::move(config),
       config_.rtp.max_packet_size - CalculateMaxHeaderSize(config_.rtp));
 }
 
 VideoSendStream::Stats VideoSendStream::GetStats() {
-  RTC_DCHECK_RUN_ON(&thread_checker_);
+  // TODO(perkj, solenberg): Some test cases in EndToEndTest call GetStats from
+  // a network thread. See comment in Call::GetStats().
+  // RTC_DCHECK_RUN_ON(&thread_checker_);
   return stats_proxy_.GetStats();
 }