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 <tommi@webrtc.org>
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34809}
diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc
index c244d17..8c0f8f6 100644
--- a/video/video_send_stream.cc
+++ b/video/video_send_stream.cc
@@ -244,7 +244,10 @@
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 6d6ab05..7edb332 100644
--- a/video/video_send_stream_tests.cc
+++ b/video/video_send_stream_tests.cc
@@ -95,6 +95,9 @@
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 @@
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 @@
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 @@
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,7 +2280,7 @@
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;
@@ -2285,7 +2288,7 @@
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,7 +2297,7 @@
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;
@@ -2302,12 +2305,23 @@
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 be611fa..15855da 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -1780,6 +1780,9 @@
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);
