diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index c244d17077..8c0f8f6f72 100644 --- a/video/video_send_stream.cc +++ b/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(); })); } diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index 8cb43136e5..09dbb70c67 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -90,6 +90,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 { @@ -2153,7 +2156,7 @@ class StartStopBitrateObserver : public test::FakeEncoder { return encoder_init_.Wait(VideoSendStreamTest::kDefaultTimeoutMs); } - bool WaitBitrateChanged(bool non_zero) { + bool WaitBitrateChanged(WaitUntil until) { do { absl::optional bitrate_kbps; { @@ -2163,8 +2166,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)); @@ -2211,15 +2214,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(); @@ -2268,7 +2271,7 @@ 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; @@ -2276,7 +2279,7 @@ TEST_F(VideoSendStreamTest, VideoSendStreamUpdateActiveSimulcastLayers) { 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; @@ -2285,7 +2288,7 @@ 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; @@ -2293,12 +2296,23 @@ TEST_F(VideoSendStreamTest, VideoSendStreamUpdateActiveSimulcastLayers) { 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(); diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index be611fad63..15855da1f2 100644 --- a/video/video_stream_encoder.cc +++ b/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);