From 59ade0172fdb7d521bca8ca4e837abf7e90bc604 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Fri, 2 Dec 2022 08:09:37 +0000 Subject: [PATCH] Reland "Remame VideoSendStream::UpdateActiveSimulcastLayers to StartPerRtpStream" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 75170be4acc90fece7c65f1a5b9bef03a5cc3880. Reason for revert: Perf regression not affecting open source. Original change's description: > Revert "Remame VideoSendStream::UpdateActiveSimulcastLayers to StartPerRtpStream" > > This reverts commit d8c4de71722c9de38f942932be21d4015f32a3bc. > > Reason for revert: Tentative revert due to possible perf regression. b/260123362 > > Original change's description: > > Remame VideoSendStream::UpdateActiveSimulcastLayers to StartPerRtpStream > > > > VideoSendStreamImpl::Start and VideoSendStream::Start are not used by PeerConnections, only StartPerRtpStream. > > Therefore this cl: > > - Change implementation of VideoSendStream::Start to use VideoSendStream::StartPerRtpStream. VideoSendstream::Start is kept for convenience. > > - Remove VideoSendStreamImpl::Start() since it was only used by tests that use call and is confusing. > > - RtpVideoSender::SetActive is removed/changed to RtpVideoSender::Stop(). For normal operations RtpVideoSender::SetActiveModules is used. > > > > Bug: none > > Change-Id: I43b153250b07c02fe63c84e3c4cec18d4ec0d47a > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/283660 > > Reviewed-by: Erik Språng > > Commit-Queue: Per Kjellander > > Cr-Commit-Position: refs/heads/main@{#38698} > > Bug: none > Change-Id: I4f0d27679e51361b9ec54d2ae8e4d972527875d1 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/284940 > Reviewed-by: Per Kjellander > Commit-Queue: Erik Språng > Auto-Submit: Per Kjellander > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Reviewed-by: Erik Språng > Cr-Commit-Position: refs/heads/main@{#38725} Bug: b/260400659 Change-Id: Ie8e545edcad85284a7d612183a8e4201672d0b5e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/285900 Auto-Submit: Per Kjellander Reviewed-by: Erik Språng Commit-Queue: Erik Språng Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/heads/main@{#38794} --- call/rtp_video_sender.cc | 30 +++++++------- call/rtp_video_sender.h | 8 ++-- call/rtp_video_sender_interface.h | 8 ++-- call/rtp_video_sender_unittest.cc | 40 +++++++++--------- call/video_send_stream.h | 10 +++-- media/engine/fake_webrtc_call.cc | 2 +- media/engine/fake_webrtc_call.h | 2 +- media/engine/webrtc_video_engine.cc | 4 +- test/call_test.cc | 7 +++- test/scenario/video_stream.cc | 2 +- video/video_send_stream.cc | 31 ++++---------- video/video_send_stream.h | 2 +- video/video_send_stream_impl.cc | 17 +------- video/video_send_stream_impl.h | 3 +- video/video_send_stream_impl_unittest.cc | 53 ++++++++++++++---------- 15 files changed, 102 insertions(+), 117 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 02155bc14a..3ca72b13ae 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -477,33 +477,24 @@ RtpVideoSender::~RtpVideoSender() { RTC_DCHECK(!registered_for_feedback_); } -void RtpVideoSender::SetActive(bool active) { +void RtpVideoSender::Stop() { RTC_DCHECK_RUN_ON(&transport_checker_); MutexLock lock(&mutex_); - if (active_ == active) + if (!active_) return; - const std::vector active_modules(rtp_streams_.size(), active); + const std::vector active_modules(rtp_streams_.size(), false); SetActiveModulesLocked(active_modules); - - auto* feedback_provider = transport_->GetStreamFeedbackProvider(); - if (active && !registered_for_feedback_) { - feedback_provider->RegisterStreamFeedbackObserver(rtp_config_.ssrcs, this); - registered_for_feedback_ = true; - } else if (!active && registered_for_feedback_) { - feedback_provider->DeRegisterStreamFeedbackObserver(this); - registered_for_feedback_ = false; - } } -void RtpVideoSender::SetActiveModules(const std::vector active_modules) { +void RtpVideoSender::SetActiveModules(const std::vector& active_modules) { RTC_DCHECK_RUN_ON(&transport_checker_); MutexLock lock(&mutex_); return SetActiveModulesLocked(active_modules); } void RtpVideoSender::SetActiveModulesLocked( - const std::vector active_modules) { + const std::vector& active_modules) { RTC_DCHECK_RUN_ON(&transport_checker_); RTC_DCHECK_EQ(rtp_streams_.size(), active_modules.size()); active_ = false; @@ -535,6 +526,17 @@ void RtpVideoSender::SetActiveModulesLocked( /*remb_candidate=*/true); } } + if (!active_) { + auto* feedback_provider = transport_->GetStreamFeedbackProvider(); + if (registered_for_feedback_) { + feedback_provider->DeRegisterStreamFeedbackObserver(this); + registered_for_feedback_ = false; + } + } else if (!registered_for_feedback_) { + auto* feedback_provider = transport_->GetStreamFeedbackProvider(); + feedback_provider->RegisterStreamFeedbackObserver(rtp_config_.ssrcs, this); + registered_for_feedback_ = true; + } } bool RtpVideoSender::IsActive() { diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index 88e55f8b13..9666b89916 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -95,13 +95,11 @@ class RtpVideoSender : public RtpVideoSenderInterface, RtpVideoSender(const RtpVideoSender&) = delete; RtpVideoSender& operator=(const RtpVideoSender&) = delete; - // RtpVideoSender will only route packets if being active, all packets will be - // dropped otherwise. - void SetActive(bool active) RTC_LOCKS_EXCLUDED(mutex_) override; // Sets the sending status of the rtp modules and appropriately sets the // payload router to active if any rtp modules are active. - void SetActiveModules(std::vector active_modules) + void SetActiveModules(const std::vector& active_modules) RTC_LOCKS_EXCLUDED(mutex_) override; + void Stop() RTC_LOCKS_EXCLUDED(mutex_) override; bool IsActive() RTC_LOCKS_EXCLUDED(mutex_) override; void OnNetworkAvailability(bool network_available) @@ -157,7 +155,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, private: bool IsActiveLocked() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); - void SetActiveModulesLocked(std::vector active_modules) + void SetActiveModulesLocked(const std::vector& active_modules) RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); void UpdateModuleSendingState() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); void ConfigureProtection(); diff --git a/call/rtp_video_sender_interface.h b/call/rtp_video_sender_interface.h index acb68e3ae2..3f2877155a 100644 --- a/call/rtp_video_sender_interface.h +++ b/call/rtp_video_sender_interface.h @@ -31,12 +31,12 @@ struct FecProtectionParams; class RtpVideoSenderInterface : public EncodedImageCallback, public FecControllerOverride { public: - // RtpVideoSender will only route packets if being active, all - // packets will be dropped otherwise. - virtual void SetActive(bool active) = 0; // Sets the sending status of the rtp modules and appropriately sets the // RtpVideoSender to active if any rtp modules are active. - virtual void SetActiveModules(std::vector active_modules) = 0; + // A module will only send packet if beeing active. + virtual void SetActiveModules(const std::vector& active_modules) = 0; + // Set the sending status of all rtp modules to inactive. + virtual void Stop() = 0; virtual bool IsActive() = 0; virtual void OnNetworkAvailability(bool network_available) = 0; diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 3ce9ca2ddc..a8595e38c9 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -187,14 +187,14 @@ class RtpVideoSenderTestFixture { /*frame_transformer=*/nullptr, field_trials) {} - ~RtpVideoSenderTestFixture() { SetActive(false); } + ~RtpVideoSenderTestFixture() { Stop(); } RtpVideoSender* router() { return router_.get(); } MockTransport& transport() { return transport_; } void AdvanceTime(TimeDelta delta) { time_controller_.AdvanceTime(delta); } - void SetActive(bool active) { - RunOnTransportQueue([&]() { router_->SetActive(active); }); + void Stop() { + RunOnTransportQueue([&]() { router_->Stop(); }); } void SetActiveModules(const std::vector& active_modules) { @@ -249,15 +249,15 @@ TEST(RtpVideoSenderTest, SendOnOneModule) { EXPECT_NE(EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image, nullptr).error); - test.SetActive(true); + test.SetActiveModules({true}); EXPECT_EQ(EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image, nullptr).error); - test.SetActive(false); + test.SetActiveModules({false}); EXPECT_NE(EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image, nullptr).error); - test.SetActive(true); + test.SetActiveModules({true}); EXPECT_EQ(EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image, nullptr).error); } @@ -276,7 +276,7 @@ TEST(RtpVideoSenderTest, SendSimulcastSetActive) { CodecSpecificInfo codec_info; codec_info.codecType = kVideoCodecVP8; - test.SetActive(true); + test.SetActiveModules({true, true}); EXPECT_EQ(EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image_1, &codec_info).error); @@ -286,7 +286,7 @@ TEST(RtpVideoSenderTest, SendSimulcastSetActive) { test.router()->OnEncodedImage(encoded_image_2, &codec_info).error); // Inactive. - test.SetActive(false); + test.Stop(); EXPECT_NE(EncodedImageCallback::Result::OK, test.router()->OnEncodedImage(encoded_image_1, &codec_info).error); EXPECT_NE(EncodedImageCallback::Result::OK, @@ -370,7 +370,7 @@ TEST( TEST(RtpVideoSenderTest, CreateWithNoPreviousStates) { RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true, true}); std::map initial_states = test.router()->GetRtpPayloadStates(); @@ -395,7 +395,7 @@ TEST(RtpVideoSenderTest, CreateWithPreviousStates) { RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, kPayloadType, states); - test.SetActive(true); + test.SetActiveModules({true, true}); std::map initial_states = test.router()->GetRtpPayloadStates(); @@ -435,7 +435,7 @@ TEST(RtpVideoSenderTest, FrameCountCallbacks) { test.router()->OnEncodedImage(encoded_image, nullptr).error); ::testing::Mock::VerifyAndClearExpectations(&callback); - test.SetActive(true); + test.SetActiveModules({true}); FrameCounts frame_counts; EXPECT_CALL(callback, FrameCountUpdated(_, kSsrc1)) @@ -464,7 +464,7 @@ TEST(RtpVideoSenderTest, FrameCountCallbacks) { TEST(RtpVideoSenderTest, DoesNotRetrasmitAckedPackets) { RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true, true}); constexpr uint8_t kPayload = 'a'; EncodedImage encoded_image; @@ -629,7 +629,7 @@ TEST(RtpVideoSenderTest, RetransmitsOnTransportWideLossInfo) { TEST(RtpVideoSenderTest, EarlyRetransmits) { RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true, true}); const uint8_t kPayload[1] = {'a'}; EncodedImage encoded_image; @@ -724,7 +724,7 @@ TEST(RtpVideoSenderTest, EarlyRetransmits) { TEST(RtpVideoSenderTest, SupportsDependencyDescriptor) { RtpVideoSenderTestFixture test({kSsrc1}, {}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true}); RtpHeaderExtensionMap extensions; extensions.Register( @@ -797,7 +797,7 @@ TEST(RtpVideoSenderTest, sent_packets.emplace_back(&extensions).Parse(packet, length)); return true; }); - test.SetActive(true); + test.SetActiveModules({true}); EncodedImage key_frame_image; key_frame_image._frameType = VideoFrameType::kVideoFrameKey; @@ -831,7 +831,7 @@ TEST(RtpVideoSenderTest, TEST(RtpVideoSenderTest, SupportsDependencyDescriptorForVp9) { RtpVideoSenderTestFixture test({kSsrc1}, {}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true}); RtpHeaderExtensionMap extensions; extensions.Register( @@ -887,7 +887,7 @@ TEST(RtpVideoSenderTest, SupportsDependencyDescriptorForVp9) { TEST(RtpVideoSenderTest, SupportsDependencyDescriptorForVp9NotProvidedByEncoder) { RtpVideoSenderTestFixture test({kSsrc1}, {}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true}); RtpHeaderExtensionMap extensions; extensions.Register( @@ -942,7 +942,7 @@ TEST(RtpVideoSenderTest, GenerateDependecyDescriptorForGenericCodecs) { test::ScopedKeyValueConfig field_trials( "WebRTC-GenericCodecDependencyDescriptor/Enabled/"); RtpVideoSenderTestFixture test({kSsrc1}, {}, kPayloadType, {}, &field_trials); - test.SetActive(true); + test.SetActiveModules({true}); RtpHeaderExtensionMap extensions; extensions.Register( @@ -988,7 +988,7 @@ TEST(RtpVideoSenderTest, GenerateDependecyDescriptorForGenericCodecs) { TEST(RtpVideoSenderTest, SupportsStoppingUsingDependencyDescriptor) { RtpVideoSenderTestFixture test({kSsrc1}, {}, kPayloadType, {}); - test.SetActive(true); + test.SetActiveModules({true}); RtpHeaderExtensionMap extensions; extensions.Register( @@ -1073,7 +1073,7 @@ TEST(RtpVideoSenderTest, OverheadIsSubtractedFromTargetBitrate) { kRtpHeaderSizeBytes + kTransportPacketOverheadBytes; RtpVideoSenderTestFixture test({kSsrc1}, {}, kPayloadType, {}, &field_trials); test.router()->OnTransportOverheadChanged(kTransportPacketOverheadBytes); - test.SetActive(true); + test.SetActiveModules({true}); { test.router()->OnBitrateUpdated(CreateBitrateAllocationUpdate(300000), diff --git a/call/video_send_stream.h b/call/video_send_stream.h index e54a51d901..9608281f2f 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h @@ -216,11 +216,15 @@ class VideoSendStream { // Note: This starts stream activity if it is inactive and one of the layers // is active. This stops stream activity if it is active and all layers are // inactive. - virtual void UpdateActiveSimulcastLayers(std::vector active_layers) = 0; + // `active_layers` should have the same size as the number of configured + // simulcast layers or one if only one rtp stream is used. + virtual void StartPerRtpStream(std::vector active_layers) = 0; // Starts stream activity. // When a stream is active, it can receive, process and deliver packets. + // Prefer to use StartPerRtpStream. virtual void Start() = 0; + // Stops stream activity. // When a stream is stopped, it can't receive, process or deliver packets. virtual void Stop() = 0; @@ -228,9 +232,9 @@ class VideoSendStream { // Accessor for determining if the stream is active. This is an inexpensive // call that must be made on the same thread as `Start()` and `Stop()` methods // are called on and will return `true` iff activity has been started either - // via `Start()` or `UpdateActiveSimulcastLayers()`. If activity is either + // via `Start()` or `StartPerRtpStream()`. If activity is either // stopped or is in the process of being stopped as a result of a call to - // either `Stop()` or `UpdateActiveSimulcastLayers()` where all layers were + // either `Stop()` or `StartPerRtpStream()` where all layers were // deactivated, the return value will be `false`. virtual bool started() = 0; diff --git a/media/engine/fake_webrtc_call.cc b/media/engine/fake_webrtc_call.cc index 063be93269..8046c3ad3a 100644 --- a/media/engine/fake_webrtc_call.cc +++ b/media/engine/fake_webrtc_call.cc @@ -338,7 +338,7 @@ void FakeVideoSendStream::ReconfigureVideoEncoder( webrtc::InvokeSetParametersCallback(callback, webrtc::RTCError::OK()); } -void FakeVideoSendStream::UpdateActiveSimulcastLayers( +void FakeVideoSendStream::StartPerRtpStream( const std::vector active_layers) { sending_ = false; for (const bool active_layer : active_layers) { diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index a3876b3ca8..370b70700f 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -202,7 +202,7 @@ class FakeVideoSendStream final void OnFrame(const webrtc::VideoFrame& frame) override; // webrtc::VideoSendStream implementation. - void UpdateActiveSimulcastLayers(std::vector active_layers) override; + void StartPerRtpStream(std::vector active_layers) override; void Start() override; void Stop() override; bool started() override { return IsSending(); } diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 8dfc46c641..d4bdeff992 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2439,7 +2439,7 @@ void WebRtcVideoChannel::WebRtcVideoSendStream::UpdateSendState() { } // This updates what simulcast layers are sending, and possibly starts // or stops the VideoSendStream. - stream_->UpdateActiveSimulcastLayers(active_layers); + stream_->StartPerRtpStream(active_layers); } else { if (stream_ != nullptr) { stream_->Stop(); @@ -2846,7 +2846,7 @@ void WebRtcVideoChannel::WebRtcVideoSendStream::RecreateWebRtcStream() { parameters_.encoder_config.encoder_specific_settings = NULL; - // Calls stream_->UpdateActiveSimulcastLayers() to start the VideoSendStream + // Calls stream_->StartPerRtpStream() to start the VideoSendStream // if necessary conditions are met. UpdateSendState(); diff --git a/test/call_test.cc b/test/call_test.cc index 7e7c9bb674..156b8a7f9e 100644 --- a/test/call_test.cc +++ b/test/call_test.cc @@ -591,8 +591,11 @@ void CallTest::Start() { } void CallTest::StartVideoStreams() { - for (VideoSendStream* video_send_stream : video_send_streams_) - video_send_stream->Start(); + for (size_t i = 0; i < video_send_streams_.size(); ++i) { + std::vector active_rtp_streams( + video_send_configs_[i].rtp.ssrcs.size(), true); + video_send_streams_[i]->StartPerRtpStream(active_rtp_streams); + } for (VideoReceiveStreamInterface* video_recv_stream : video_receive_streams_) video_recv_stream->Start(); } diff --git a/test/scenario/video_stream.cc b/test/scenario/video_stream.cc index ad352f9ab9..96ced83b04 100644 --- a/test/scenario/video_stream.cc +++ b/test/scenario/video_stream.cc @@ -480,7 +480,7 @@ void SendVideoStream::UpdateActiveLayers(std::vector active_layers) { MutexLock lock(&mutex_); if (config_.encoder.codec == VideoStreamConfig::Encoder::Codec::kVideoCodecVP8) { - send_stream_->UpdateActiveSimulcastLayers(active_layers); + send_stream_->StartPerRtpStream(active_layers); } VideoEncoderConfig encoder_config = CreateVideoEncoderConfig(config_); RTC_CHECK_EQ(encoder_config.simulcast_layers.size(), active_layers.size()); diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 30598c1103..e5545e761c 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -209,8 +209,12 @@ VideoSendStream::~VideoSendStream() { transport_->DestroyRtpVideoSender(rtp_video_sender_); } -void VideoSendStream::UpdateActiveSimulcastLayers( - const std::vector active_layers) { +void VideoSendStream::Start() { + const std::vector active_layers(config_.rtp.ssrcs.size(), true); + StartPerRtpStream(active_layers); +} + +void VideoSendStream::StartPerRtpStream(const std::vector active_layers) { RTC_DCHECK_RUN_ON(&thread_checker_); // Keep our `running_` flag expected state in sync with active layers since @@ -232,35 +236,16 @@ void VideoSendStream::UpdateActiveSimulcastLayers( } } active_layers_string << "}"; - RTC_LOG(LS_INFO) << "UpdateActiveSimulcastLayers: " - << active_layers_string.str(); + RTC_LOG(LS_INFO) << "StartPerRtpStream: " << active_layers_string.str(); rtp_transport_queue_->RunOrPost( SafeTask(transport_queue_safety_, [this, active_layers] { - send_stream_.UpdateActiveSimulcastLayers(active_layers); + send_stream_.StartPerRtpStream(active_layers); })); running_ = running; } -void VideoSendStream::Start() { - RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DLOG(LS_INFO) << "VideoSendStream::Start"; - if (running_) - return; - - running_ = true; - - // It is expected that after VideoSendStream::Start has been called, incoming - // frames are not dropped in VideoStreamEncoder. To ensure this, Start has to - // be synchronized. - // TODO(tommi): ^^^ Validate if this still holds. - rtp_transport_queue_->RunSynchronous([this] { - transport_queue_safety_->SetAlive(); - send_stream_.Start(); - }); -} - void VideoSendStream::Stop() { RTC_DCHECK_RUN_ON(&thread_checker_); if (!running_) diff --git a/video/video_send_stream.h b/video/video_send_stream.h index a052b856ec..a7ce112b21 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -78,8 +78,8 @@ class VideoSendStream : public webrtc::VideoSendStream { void DeliverRtcp(const uint8_t* packet, size_t length); // webrtc::VideoSendStream implementation. - void UpdateActiveSimulcastLayers(std::vector active_layers) override; void Start() override; + void StartPerRtpStream(std::vector active_layers) override; void Stop() override; bool started() override; diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index 06f6a05e2f..f34388e56a 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -312,31 +312,18 @@ void VideoSendStreamImpl::DeliverRtcp(const uint8_t* packet, size_t length) { rtp_video_sender_->DeliverRtcp(packet, length); } -void VideoSendStreamImpl::UpdateActiveSimulcastLayers( +void VideoSendStreamImpl::StartPerRtpStream( const std::vector active_layers) { RTC_DCHECK_RUN_ON(rtp_transport_queue_); bool previously_active = rtp_video_sender_->IsActive(); rtp_video_sender_->SetActiveModules(active_layers); if (!rtp_video_sender_->IsActive() && previously_active) { - // Payload router switched from active to inactive. StopVideoSendStream(); } else if (rtp_video_sender_->IsActive() && !previously_active) { - // Payload router switched from inactive to active. StartupVideoSendStream(); } } -void VideoSendStreamImpl::Start() { - RTC_DCHECK_RUN_ON(rtp_transport_queue_); - RTC_LOG(LS_INFO) << "VideoSendStream::Start"; - if (rtp_video_sender_->IsActive()) - return; - - TRACE_EVENT_INSTANT0("webrtc", "VideoSendStream::Start"); - rtp_video_sender_->SetActive(true); - StartupVideoSendStream(); -} - void VideoSendStreamImpl::StartupVideoSendStream() { RTC_DCHECK_RUN_ON(rtp_transport_queue_); transport_queue_safety_->SetAlive(); @@ -378,7 +365,7 @@ void VideoSendStreamImpl::Stop() { RTC_DCHECK(transport_queue_safety_->alive()); TRACE_EVENT_INSTANT0("webrtc", "VideoSendStream::Stop"); - rtp_video_sender_->SetActive(false); + rtp_video_sender_->Stop(); StopVideoSendStream(); } diff --git a/video/video_send_stream_impl.h b/video/video_send_stream_impl.h index d444eabc21..f145450655 100644 --- a/video/video_send_stream_impl.h +++ b/video/video_send_stream_impl.h @@ -79,8 +79,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, ~VideoSendStreamImpl() override; void DeliverRtcp(const uint8_t* packet, size_t length); - void UpdateActiveSimulcastLayers(std::vector active_layers); - void Start(); + void StartPerRtpStream(std::vector active_layers); void Stop(); // TODO(holmer): Move these to RtpTransportControllerSend. diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index 8a88ba0f16..c38dcd0e1e 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -66,8 +66,8 @@ std::string GetAlrProbingExperimentString() { } class MockRtpVideoSender : public RtpVideoSenderInterface { public: - MOCK_METHOD(void, SetActive, (bool), (override)); - MOCK_METHOD(void, SetActiveModules, (const std::vector), (override)); + MOCK_METHOD(void, SetActiveModules, (const std::vector&), (override)); + MOCK_METHOD(void, Stop, (), (override)); MOCK_METHOD(bool, IsActive, (), (override)); MOCK_METHOD(void, OnNetworkAvailability, (bool), (override)); MOCK_METHOD((std::map), @@ -139,12 +139,19 @@ class VideoSendStreamImplTest : public ::testing::Test { .WillRepeatedly(Return(&packet_router_)); EXPECT_CALL(transport_controller_, CreateRtpVideoSender) .WillRepeatedly(Return(&rtp_video_sender_)); - EXPECT_CALL(rtp_video_sender_, SetActive(_)) - .WillRepeatedly(::testing::Invoke( - [&](bool active) { rtp_video_sender_active_ = active; })); - EXPECT_CALL(rtp_video_sender_, IsActive()) - .WillRepeatedly( - ::testing::Invoke([&]() { return rtp_video_sender_active_; })); + ON_CALL(rtp_video_sender_, Stop()).WillByDefault(::testing::Invoke([&] { + active_modules_.clear(); + })); + ON_CALL(rtp_video_sender_, IsActive()) + .WillByDefault(::testing::Invoke([&]() { + for (bool enabled : active_modules_) { + if (enabled) + return true; + } + return false; + })); + ON_CALL(rtp_video_sender_, SetActiveModules) + .WillByDefault(::testing::SaveArg<0>(&active_modules_)); ON_CALL(transport_controller_, GetWorkerQueue()) .WillByDefault(Return(&worker_queue_)); } @@ -183,8 +190,8 @@ class VideoSendStreamImplTest : public ::testing::Test { NiceMock bitrate_allocator_; NiceMock video_stream_encoder_; NiceMock rtp_video_sender_; + std::vector active_modules_; - bool rtp_video_sender_active_ = false; RtcEventLogNull event_log_; VideoSendStream::Config config_; SendDelayStats send_delay_stats_; @@ -210,7 +217,7 @@ TEST_F(VideoSendStreamImplTest, RegistersAsBitrateObserverOnStart) { EXPECT_EQ(config.bitrate_priority, kDefaultBitratePriority); })); worker_queue_.RunSynchronous([&] { - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); EXPECT_CALL(bitrate_allocator_, RemoveObserver(vss_impl.get())).Times(1); vss_impl->Stop(); }); @@ -225,7 +232,7 @@ TEST_F(VideoSendStreamImplTest, UpdatesObserverOnConfigurationChange) { kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kRealtimeVideo); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); // QVGA + VGA configuration matching defaults in // media/engine/simulcast.cc. @@ -291,7 +298,7 @@ TEST_F(VideoSendStreamImplTest, UpdatesObserverOnConfigurationChangeWithAlr) { auto vss_impl = CreateVideoSendStreamImpl( kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kScreen); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); // Simulcast screenshare. VideoStream low_stream; @@ -357,7 +364,7 @@ TEST_F(VideoSendStreamImplTest, kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kRealtimeVideo); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); // 2-layer video simulcast. VideoStream low_stream; low_stream.width = 320; @@ -422,7 +429,7 @@ TEST_F(VideoSendStreamImplTest, SetsScreensharePacingFactorWithFeedback) { kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kScreen); worker_queue_.RunSynchronous([&] { - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); vss_impl->Stop(); }); } @@ -434,7 +441,7 @@ TEST_F(VideoSendStreamImplTest, DoesNotSetPacingFactorWithoutFeedback) { VideoEncoderConfig::ContentType::kScreen); worker_queue_.RunSynchronous([&] { EXPECT_CALL(transport_controller_, SetPacingFactor(_)).Times(0); - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); vss_impl->Stop(); }); } @@ -447,7 +454,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) { EXPECT_CALL(transport_controller_, SetPacingFactor(_)).Times(0); VideoStreamEncoderInterface::EncoderSink* const sink = static_cast(vss_impl.get()); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); // Populate a test instance of video bitrate allocation. VideoBitrateAllocation alloc; alloc.SetBitrate(0, 0, 10000); @@ -494,7 +501,7 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) { kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kScreen); worker_queue_.RunSynchronous([&] { - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); // Unpause encoder, to allows allocations to be passed through. const uint32_t kBitrateBps = 100000; EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps()) @@ -556,7 +563,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) { VideoEncoderConfig::ContentType::kScreen); worker_queue_.RunSynchronous([&] { - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); // Unpause encoder, to allows allocations to be passed through. const uint32_t kBitrateBps = 100000; EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps()) @@ -599,7 +606,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kScreen); worker_queue_.RunSynchronous([&] { - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); const uint32_t kBitrateBps = 100000; // Unpause encoder, to allows allocations to be passed through. EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps()) @@ -709,7 +716,7 @@ TEST_F(VideoSendStreamImplTest, CallsVideoStreamEncoderOnBitrateUpdate) { auto vss_impl = CreateVideoSendStreamImpl( kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kRealtimeVideo); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); VideoStream qvga_stream; qvga_stream.width = 320; qvga_stream.height = 180; @@ -842,7 +849,7 @@ TEST_F(VideoSendStreamImplTest, DisablesPaddingOnPausedEncoder) { int min_transmit_bitrate_bps = 30000; config_.rtp.ssrcs.emplace_back(1); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); // Starts without padding. EXPECT_EQ(0, padding_bitrate); encoder_queue_->PostTask([&] { @@ -893,7 +900,7 @@ TEST_F(VideoSendStreamImplTest, KeepAliveOnDroppedFrame) { VideoEncoderConfig::ContentType::kRealtimeVideo); EXPECT_CALL(bitrate_allocator_, RemoveObserver(vss_impl.get())).Times(0); worker_queue_.RunSynchronous([&] { - vss_impl->Start(); + vss_impl->StartPerRtpStream({true}); const uint32_t kBitrateBps = 100000; EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps()) .Times(1) @@ -941,7 +948,7 @@ TEST_F(VideoSendStreamImplTest, ConfiguresBitratesForSvc) { ? VideoEncoderConfig::ContentType::kScreen : VideoEncoderConfig::ContentType::kRealtimeVideo); - worker_queue_.RunSynchronous([&] { vss_impl->Start(); }); + worker_queue_.RunSynchronous([&] { vss_impl->StartPerRtpStream({true}); }); // Svc VideoStream stream;