Keep transport_queue_safety_ alive until stopped permanently.

After a send stream is stopped, it can still be re-used and implicitly
restarted by activating layers. This change removes marking the flag
we use for async operations as 'not alive' inside Stop() and only doing
so when the send stream is stopped permanently.

The effect this has is that an implicit start via
UpdateActiveSimulcastLayers() will run and correctly update the states.
Before, if a stream had been stopped, the safety flag would prevent
the async operation from running.

Bug: chromium:1241213
Change-Id: Iebdfabba3e1955aafa364760eebd4f66281bcc60
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229304
Commit-Queue: Tommi <[email protected]>
Reviewed-by: Mirko Bonadei <[email protected]>
Cr-Commit-Position: refs/heads/master@{#34809}

video/video_send_stream.cc

@@ -244,7 +244,10 @@ void VideoSendStream::Stop() {
   RTC_DLOG(LS_INFO) << "VideoSendStream::Stop";
   running_ = false;
   rtp_transport_queue_->PostTask(ToQueuedTask(transport_queue_safety_, [this] {
-    transport_queue_safety_->SetNotAlive();
+    // As the stream can get re-used and implicitly restarted via changing
+    // the state of the active layers, we do not mark the
+    // `transport_queue_safety_` flag with `SetNotAlive()` here. That's only
+    // done when we stop permanently via `StopPermanentlyAndGetRtpStates()`.
     send_stream_.Stop();
   }));
 }

video/video_send_stream_tests.cc

@@ -95,6 +95,9 @@ enum : int {  // The first valid value is 1.
   kVideoTimingExtensionId,
 };
 
+// Readability convenience enum for `WaitBitrateChanged()`.
+enum class WaitUntil : bool { kZero = false, kNonZero = true };
+
 constexpr int64_t kRtcpIntervalMs = 1000;
 
 enum VideoFormat {
@@ -2162,7 +2165,7 @@ class StartStopBitrateObserver : public test::FakeEncoder {
     return encoder_init_.Wait(VideoSendStreamTest::kDefaultTimeoutMs);
   }
 
-  bool WaitBitrateChanged(bool non_zero) {
+  bool WaitBitrateChanged(WaitUntil until) {
     do {
       absl::optional<int> bitrate_kbps;
       {
@@ -2172,8 +2175,8 @@ class StartStopBitrateObserver : public test::FakeEncoder {
       if (!bitrate_kbps)
         continue;
 
-      if ((non_zero && *bitrate_kbps > 0) ||
-          (!non_zero && *bitrate_kbps == 0)) {
+      if ((until == WaitUntil::kNonZero && *bitrate_kbps > 0) ||
+          (until == WaitUntil::kZero && *bitrate_kbps == 0)) {
         return true;
       }
     } while (bitrate_changed_.Wait(VideoSendStreamTest::kDefaultTimeoutMs));
@@ -2220,15 +2223,15 @@ TEST_F(VideoSendStreamTest, VideoSendStreamStopSetEncoderRateToZero) {
 
   SendTask(RTC_FROM_HERE, task_queue(),
            [this]() { GetVideoSendStream()->Start(); });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(true));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kNonZero));
 
   SendTask(RTC_FROM_HERE, task_queue(),
            [this]() { GetVideoSendStream()->Stop(); });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(false));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kZero));
 
   SendTask(RTC_FROM_HERE, task_queue(),
            [this]() { GetVideoSendStream()->Start(); });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(true));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kNonZero));
 
   SendTask(RTC_FROM_HERE, task_queue(), [this]() {
     DestroyStreams();
@@ -2277,15 +2280,15 @@ TEST_F(VideoSendStreamTest, VideoSendStreamUpdateActiveSimulcastLayers) {
     GetVideoSendStream()->UpdateActiveSimulcastLayers({true, true});
     EXPECT_TRUE(GetVideoSendStream()->started());
   });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(true));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kNonZero));
 
   GetVideoEncoderConfig()->simulcast_layers[0].active = true;
   GetVideoEncoderConfig()->simulcast_layers[1].active = false;
   SendTask(RTC_FROM_HERE, task_queue(), [this]() {
     GetVideoSendStream()->ReconfigureVideoEncoder(
         GetVideoEncoderConfig()->Copy());
   });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(true));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kNonZero));
 
   // Turning off both simulcast layers should trigger a bitrate change of 0.
   GetVideoEncoderConfig()->simulcast_layers[0].active = false;
@@ -2294,20 +2297,31 @@ TEST_F(VideoSendStreamTest, VideoSendStreamUpdateActiveSimulcastLayers) {
     GetVideoSendStream()->UpdateActiveSimulcastLayers({false, false});
     EXPECT_FALSE(GetVideoSendStream()->started());
   });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(false));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kZero));
 
   // Re-activating a layer should resume sending and trigger a bitrate change.
   GetVideoEncoderConfig()->simulcast_layers[0].active = true;
   SendTask(RTC_FROM_HERE, task_queue(), [this]() {
     GetVideoSendStream()->UpdateActiveSimulcastLayers({true, false});
     EXPECT_TRUE(GetVideoSendStream()->started());
   });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(true));
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kNonZero));
 
-  // Stop and clean up.
-  SendTask(RTC_FROM_HERE, task_queue(),
-           [this]() { GetVideoSendStream()->Stop(); });
-  EXPECT_TRUE(encoder.WaitBitrateChanged(false));
+  // Stop the stream and make sure the bit rate goes to zero again.
+  SendTask(RTC_FROM_HERE, task_queue(), [this]() {
+    GetVideoSendStream()->Stop();
+    EXPECT_FALSE(GetVideoSendStream()->started());
+  });
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kZero));
+
+  // One last test to verify that after `Stop()` we can still implicitly start
+  // the stream if needed. This is what will happen when a send stream gets
+  // re-used. See crbug.com/1241213.
+  SendTask(RTC_FROM_HERE, task_queue(), [this]() {
+    GetVideoSendStream()->UpdateActiveSimulcastLayers({true, true});
+    EXPECT_TRUE(GetVideoSendStream()->started());
+  });
+  EXPECT_TRUE(encoder.WaitBitrateChanged(WaitUntil::kNonZero));
 
   SendTask(RTC_FROM_HERE, task_queue(), [this]() {
     DestroyStreams();

video/video_stream_encoder.cc

@@ -1780,6 +1780,9 @@ void VideoStreamEncoder::SendKeyFrame() {
   TRACE_EVENT0("webrtc", "OnKeyFrameRequest");
   RTC_DCHECK(!next_frame_types_.empty());
 
+  if (!encoder_)
+    return;  // Shutting down.
+
   // TODO(webrtc:10615): Map keyframe request to spatial layer.
   std::fill(next_frame_types_.begin(), next_frame_types_.end(),
             VideoFrameType::kVideoFrameKey);