From cc7125f2400b8dc8f3be0011abbe584551afe145 Mon Sep 17 00:00:00 2001 From: Seth Hampson Date: Fri, 2 Feb 2018 08:46:16 -0800 Subject: [PATCH] Sets sending status for active RtpRtcp modules. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a simulcast stream is enabled or disabled, we want this state change to be reflected properly in the RtpRtcp modules. Each video send stream can contain multiple rtp_rtcp_modules pertaining to different simulcast streams. These modules are currently all turned on/off when the send stream is started and stopped. This change allows for individual modules to be turned on/off. This means if a module stops sending it will send a bye message, so the receiving side will not expect more frames to be sent when the stream is inactive and the encoder is no longer encoding/sending images. Bug: webrtc:8653 Change-Id: Ib6d00240f627b4ff1714646e847026f24c7c3aa4 Reviewed-on: https://webrtc-review.googlesource.com/42841 Commit-Queue: Seth Hampson Reviewed-by: Taylor Brandstetter Reviewed-by: Stefan Holmer Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#21880} --- call/video_send_stream.h | 10 ++ media/engine/fakewebrtccall.cc | 11 ++ media/engine/fakewebrtccall.h | 2 + media/engine/webrtcvideoengine.cc | 7 +- test/encoder_settings.cc | 9 +- video/payload_router.cc | 23 ++++- video/payload_router.h | 3 + video/payload_router_unittest.cc | 74 +++++++++++++- video/video_send_stream.cc | 45 ++++++++- video/video_send_stream.h | 2 + video/video_send_stream_tests.cc | 160 ++++++++++++++++++++---------- video/video_stream_encoder.cc | 4 + 12 files changed, 291 insertions(+), 59 deletions(-) diff --git a/call/video_send_stream.h b/call/video_send_stream.h index ff9c477f78..b5daa3e2a9 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h @@ -252,6 +252,16 @@ class VideoSendStream { Config(const Config&); }; + // Updates the sending state for all simulcast layers that the video send + // stream owns. This can mean updating the activity one or for multiple + // layers. The ordering of active layers is the order in which the + // rtp modules are stored in the 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( + const std::vector active_layers) = 0; + // Starts stream activity. // When a stream is active, it can receive, process and deliver packets. virtual void Start() = 0; diff --git a/media/engine/fakewebrtccall.cc b/media/engine/fakewebrtccall.cc index 5dcfd762e9..2a5f08678d 100644 --- a/media/engine/fakewebrtccall.cc +++ b/media/engine/fakewebrtccall.cc @@ -249,6 +249,17 @@ void FakeVideoSendStream::ReconfigureVideoEncoder( ++num_encoder_reconfigurations_; } +void FakeVideoSendStream::UpdateActiveSimulcastLayers( + const std::vector active_layers) { + sending_ = false; + for (const bool active_layer : active_layers) { + if (active_layer) { + sending_ = true; + break; + } + } +} + void FakeVideoSendStream::Start() { sending_ = true; } diff --git a/media/engine/fakewebrtccall.h b/media/engine/fakewebrtccall.h index d38a27b3f8..011276588a 100644 --- a/media/engine/fakewebrtccall.h +++ b/media/engine/fakewebrtccall.h @@ -158,6 +158,8 @@ class FakeVideoSendStream final void OnFrame(const webrtc::VideoFrame& frame) override; // webrtc::VideoSendStream implementation. + void UpdateActiveSimulcastLayers( + const std::vector active_layers) override; void Start() override; void Stop() override; void SetSource(rtc::VideoSourceInterface* source, diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc index 42a2a0b86c..6c10ca8152 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -1798,6 +1798,9 @@ webrtc::RTCError WebRtcVideoChannel::WebRtcVideoSendStream::SetRtpParameters( return error; } + // TODO(bugs.webrtc.org/8807): The bitrate priority really doesn't require an + // entire encoder reconfiguration, it just needs to update the bitrate + // allocator. bool reconfigure_encoder = (new_parameters.encodings[0].max_bitrate_bps != rtp_parameters_.encodings[0].max_bitrate_bps) || (new_parameters.encodings[0].bitrate_priority != @@ -1843,7 +1846,9 @@ WebRtcVideoChannel::WebRtcVideoSendStream::ValidateRtpParameters( void WebRtcVideoChannel::WebRtcVideoSendStream::UpdateSendState() { RTC_DCHECK_RUN_ON(&thread_checker_); - // TODO(zstein): Handle multiple encodings. + // TODO(bugs.webrtc.org/8653): Handle multiple encodings by creating a + // vector of bools corresponding to the appropriate active streams, and call + // stream_->UpdateActiveSimulcastLayers(). if (sending_ && rtp_parameters_.encodings[0].active) { RTC_DCHECK(stream_ != nullptr); stream_->Start(); diff --git a/test/encoder_settings.cc b/test/encoder_settings.cc index 205af7bdac..685ec63c85 100644 --- a/test/encoder_settings.cc +++ b/test/encoder_settings.cc @@ -50,7 +50,13 @@ std::vector CreateVideoStreams( std::min(bitrate_left_bps, DefaultVideoStreamFactory::kMaxBitratePerStream[i]); stream_settings[i].max_qp = 56; - stream_settings[i].active = true; + if (i < encoder_config.simulcast_layers.size()) { + // Higher level controls are setting the active configuration for the + // VideoStream. + stream_settings[i].active = encoder_config.simulcast_layers[i].active; + } else { + stream_settings[i].active = true; + } bitrate_left_bps -= stream_settings[i].target_bitrate_bps; } @@ -78,6 +84,7 @@ void FillEncoderConfiguration(size_t num_streams, configuration->video_stream_factory = new rtc::RefCountedObject(); configuration->max_bitrate_bps = 0; + configuration->simulcast_layers = std::vector(num_streams); for (size_t i = 0; i < num_streams; ++i) { configuration->max_bitrate_bps += DefaultVideoStreamFactory::kMaxBitratePerStream[i]; diff --git a/video/payload_router.cc b/video/payload_router.cc index 1d16fbd8c0..4ecb620c7c 100644 --- a/video/payload_router.cc +++ b/video/payload_router.cc @@ -151,11 +151,22 @@ void PayloadRouter::SetActive(bool active) { rtc::CritScope lock(&crit_); if (active_ == active) return; - active_ = active; + const std::vector active_modules(rtp_modules_.size(), active); + SetActiveModules(active_modules); +} - for (auto& module : rtp_modules_) { - module->SetSendingStatus(active_); - module->SetSendingMediaStatus(active_); +void PayloadRouter::SetActiveModules(const std::vector active_modules) { + rtc::CritScope lock(&crit_); + RTC_DCHECK_EQ(rtp_modules_.size(), active_modules.size()); + active_ = false; + for (size_t i = 0; i < active_modules.size(); ++i) { + if (active_modules[i]) { + active_ = true; + } + // Sends a kRtcpByeCode when going from true to false. + rtp_modules_[i]->SetSendingStatus(active_modules[i]); + // If set to false this module won't send media. + rtp_modules_[i]->SetSendingMediaStatus(active_modules[i]); } } @@ -217,6 +228,10 @@ EncodedImageCallback::Result PayloadRouter::OnEncodedImage( params_[stream_index].Set(&rtp_video_header); } uint32_t frame_id; + if (!rtp_modules_[stream_index]->Sending()) { + // The payload router could be active but this module isn't sending. + return Result(Result::ERROR_SEND_FAILED); + } bool send_result = rtp_modules_[stream_index]->SendOutgoingData( encoded_image._frameType, payload_type_, encoded_image._timeStamp, encoded_image.capture_time_ms_, encoded_image._buffer, diff --git a/video/payload_router.h b/video/payload_router.h index 13b6cae7ce..68779eff4d 100644 --- a/video/payload_router.h +++ b/video/payload_router.h @@ -40,6 +40,9 @@ class PayloadRouter : public EncodedImageCallback { // PayloadRouter will only route packets if being active, all packets will be // dropped otherwise. void SetActive(bool active); + // Sets the sending status of the rtp modules and appropriately sets the + // payload router to active if any rtp modules are active. + void SetActiveModules(const std::vector active_modules); bool IsActive(); std::map GetRtpPayloadStates() const; diff --git a/video/payload_router_unittest.cc b/video/payload_router_unittest.cc index a512582670..b193da8cda 100644 --- a/video/payload_router_unittest.cc +++ b/video/payload_router_unittest.cc @@ -70,6 +70,7 @@ TEST(PayloadRouterTest, SendOnOneModule) { encoded_image._length, nullptr, _, _)) .Times(1) .WillOnce(Return(true)); + EXPECT_CALL(rtp, Sending()).WillOnce(Return(true)); EXPECT_EQ( EncodedImageCallback::Result::OK, payload_router.OnEncodedImage(encoded_image, nullptr, nullptr).error); @@ -91,12 +92,13 @@ TEST(PayloadRouterTest, SendOnOneModule) { encoded_image._length, nullptr, _, _)) .Times(1) .WillOnce(Return(true)); + EXPECT_CALL(rtp, Sending()).WillOnce(Return(true)); EXPECT_EQ( EncodedImageCallback::Result::OK, payload_router.OnEncodedImage(encoded_image, nullptr, nullptr).error); } -TEST(PayloadRouterTest, SendSimulcast) { +TEST(PayloadRouterTest, SendSimulcastSetActive) { NiceMock rtp_1; NiceMock rtp_2; std::vector modules = {&rtp_1, &rtp_2}; @@ -117,6 +119,7 @@ TEST(PayloadRouterTest, SendSimulcast) { codec_info_1.codecSpecific.VP8.simulcastIdx = 0; payload_router.SetActive(true); + EXPECT_CALL(rtp_1, Sending()).WillOnce(Return(true)); EXPECT_CALL(rtp_1, SendOutgoingData(encoded_image._frameType, kPayloadType, encoded_image._timeStamp, encoded_image.capture_time_ms_, &payload, @@ -133,6 +136,7 @@ TEST(PayloadRouterTest, SendSimulcast) { codec_info_2.codecType = kVideoCodecVP8; codec_info_2.codecSpecific.VP8.simulcastIdx = 1; + EXPECT_CALL(rtp_2, Sending()).WillOnce(Return(true)); EXPECT_CALL(rtp_2, SendOutgoingData(encoded_image._frameType, kPayloadType, encoded_image._timeStamp, encoded_image.capture_time_ms_, &payload, @@ -159,6 +163,65 @@ TEST(PayloadRouterTest, SendSimulcast) { .error); } +// Tests how setting individual rtp modules to active affects the overall +// behavior of the payload router. First sets one module to active and checks +// that outgoing data can be sent on this module, and checks that no data can be +// sent if both modules are inactive. +TEST(PayloadRouterTest, SendSimulcastSetActiveModules) { + NiceMock rtp_1; + NiceMock rtp_2; + std::vector modules = {&rtp_1, &rtp_2}; + + uint8_t payload = 'a'; + EncodedImage encoded_image; + encoded_image._timeStamp = 1; + encoded_image.capture_time_ms_ = 2; + encoded_image._frameType = kVideoFrameKey; + encoded_image._buffer = &payload; + encoded_image._length = 1; + PayloadRouter payload_router(modules, {kSsrc1, kSsrc2}, kPayloadType, {}); + CodecSpecificInfo codec_info_1; + memset(&codec_info_1, 0, sizeof(CodecSpecificInfo)); + codec_info_1.codecType = kVideoCodecVP8; + codec_info_1.codecSpecific.VP8.simulcastIdx = 0; + CodecSpecificInfo codec_info_2; + memset(&codec_info_2, 0, sizeof(CodecSpecificInfo)); + codec_info_2.codecType = kVideoCodecVP8; + codec_info_2.codecSpecific.VP8.simulcastIdx = 1; + + // Only setting one stream to active will still set the payload router to + // active and allow sending data on the active stream. + std::vector active_modules({true, false}); + payload_router.SetActiveModules(active_modules); + + EXPECT_CALL(rtp_1, Sending()).WillOnce(Return(true)); + EXPECT_CALL(rtp_1, SendOutgoingData(encoded_image._frameType, kPayloadType, + encoded_image._timeStamp, + encoded_image.capture_time_ms_, &payload, + encoded_image._length, nullptr, _, _)) + .Times(1) + .WillOnce(Return(true)); + EXPECT_EQ(EncodedImageCallback::Result::OK, + payload_router.OnEncodedImage(encoded_image, &codec_info_1, nullptr) + .error); + + // Setting both streams to inactive will turn the payload router to inactive. + active_modules = {false, false}; + payload_router.SetActiveModules(active_modules); + // An incoming encoded image will not ask the module to send outgoing data + // because the payload router is inactive. + EXPECT_CALL(rtp_1, SendOutgoingData(_, _, _, _, _, _, _, _, _)).Times(0); + EXPECT_CALL(rtp_1, Sending()).Times(0); + EXPECT_CALL(rtp_2, SendOutgoingData(_, _, _, _, _, _, _, _, _)).Times(0); + EXPECT_CALL(rtp_2, Sending()).Times(0); + EXPECT_NE(EncodedImageCallback::Result::OK, + payload_router.OnEncodedImage(encoded_image, &codec_info_1, nullptr) + .error); + EXPECT_NE(EncodedImageCallback::Result::OK, + payload_router.OnEncodedImage(encoded_image, &codec_info_2, nullptr) + .error); +} + TEST(PayloadRouterTest, SimulcastTargetBitrate) { NiceMock rtp_1; NiceMock rtp_2; @@ -262,6 +325,7 @@ TEST(PayloadRouterTest, InfoMappedToRtpVideoHeader_Vp8) { codec_info.codecSpecific.VP8.layerSync = true; codec_info.codecSpecific.VP8.nonReference = true; + EXPECT_CALL(rtp2, Sending()).WillOnce(Return(true)); EXPECT_CALL(rtp2, SendOutgoingData(_, _, _, _, _, _, nullptr, _, _)) .WillOnce(Invoke([](Unused, Unused, Unused, Unused, Unused, Unused, Unused, const RTPVideoHeader* header, Unused) { @@ -296,6 +360,7 @@ TEST(PayloadRouterTest, InfoMappedToRtpVideoHeader_H264) { codec_info.codecSpecific.H264.packetization_mode = H264PacketizationMode::SingleNalUnit; + EXPECT_CALL(rtp1, Sending()).WillOnce(Return(true)); EXPECT_CALL(rtp1, SendOutgoingData(_, _, _, _, _, _, nullptr, _, _)) .WillOnce(Invoke([](Unused, Unused, Unused, Unused, Unused, Unused, Unused, const RTPVideoHeader* header, Unused) { @@ -389,6 +454,7 @@ TEST_F(TestWithForcedFallbackDisabled, PictureIdIsNotChangedForVp8) { EXPECT_EQ(kPictureId, header->codecHeader.VP8.pictureId); return true; })); + EXPECT_CALL(rtp, Sending()).WillOnce(Return(true)); EXPECT_EQ(EncodedImageCallback::Result::OK, router.OnEncodedImage(image_, &codec_info_, nullptr).error); @@ -408,6 +474,7 @@ TEST_F(TestWithForcedFallbackEnabled, PictureIdIsSetForVp8) { PayloadRouter router(modules, {kSsrc1, kSsrc2}, kPayloadType, states); router.SetActive(true); + // Modules are sending for this test. // OnEncodedImage, simulcastIdx: 0. codec_info_.codecType = kVideoCodecVP8; codec_info_.codecSpecific.VP8.pictureId = kPictureId; @@ -420,6 +487,7 @@ TEST_F(TestWithForcedFallbackEnabled, PictureIdIsSetForVp8) { EXPECT_EQ(kInitialPictureId1, header->codecHeader.VP8.pictureId); return true; })); + EXPECT_CALL(rtp1, Sending()).WillOnce(Return(true)); EXPECT_EQ(EncodedImageCallback::Result::OK, router.OnEncodedImage(image_, &codec_info_, nullptr).error); @@ -435,6 +503,7 @@ TEST_F(TestWithForcedFallbackEnabled, PictureIdIsSetForVp8) { EXPECT_EQ(kInitialPictureId2, header->codecHeader.VP8.pictureId); return true; })); + EXPECT_CALL(rtp2, Sending()).WillOnce(Return(true)); EXPECT_EQ(EncodedImageCallback::Result::OK, router.OnEncodedImage(image_, &codec_info_, nullptr).error); @@ -465,6 +534,7 @@ TEST_F(TestWithForcedFallbackEnabled, PictureIdWraps) { EXPECT_EQ(kMaxTwoBytePictureId, header->codecHeader.VP8.pictureId); return true; })); + EXPECT_CALL(rtp, Sending()).WillOnce(Return(true)); EXPECT_EQ(EncodedImageCallback::Result::OK, router.OnEncodedImage(image_, &codec_info_, nullptr).error); @@ -491,6 +561,7 @@ TEST_F(TestWithForcedFallbackEnabled, PictureIdIsNotSetIfNoPictureId) { EXPECT_EQ(kNoPictureId, header->codecHeader.VP8.pictureId); return true; })); + EXPECT_CALL(rtp, Sending()).WillOnce(Return(true)); EXPECT_EQ(EncodedImageCallback::Result::OK, router.OnEncodedImage(image_, &codec_info_, nullptr).error); @@ -512,6 +583,7 @@ TEST_F(TestWithForcedFallbackEnabled, PictureIdIsNotSetForVp9) { EXPECT_EQ(kPictureId, header->codecHeader.VP9.picture_id); return true; })); + EXPECT_CALL(rtp, Sending()).WillOnce(Return(true)); EXPECT_EQ(EncodedImageCallback::Result::OK, router.OnEncodedImage(image_, &codec_info_, nullptr).error); diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 800cca24e9..d5785d884d 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -280,6 +280,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, void SignalNetworkState(NetworkState state); bool DeliverRtcp(const uint8_t* packet, size_t length); + void UpdateActiveSimulcastLayers(const std::vector active_layers); void Start(); void Stop(); @@ -332,6 +333,12 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, // Implements VideoBitrateAllocationObserver. void OnBitrateAllocationUpdated(const BitrateAllocation& allocation) override; + // Starts monitoring and sends a keyframe. + void StartupVideoSendStream(); + // Removes the bitrate observer, stops monitoring and notifies the video + // encoder of the bitrate update. + void StopVideoSendStream(); + void ConfigureProtection(); void ConfigureSsrcs(); void SignalEncoderTimedOut(); @@ -622,6 +629,19 @@ VideoSendStream::~VideoSendStream() { RTC_DCHECK(!send_stream_); } +void VideoSendStream::UpdateActiveSimulcastLayers( + const std::vector active_layers) { + RTC_DCHECK_RUN_ON(&thread_checker_); + RTC_LOG(LS_INFO) << "VideoSendStream::UpdateActiveSimulcastLayers"; + VideoSendStreamImpl* send_stream = send_stream_.get(); + worker_queue_->PostTask([this, send_stream, active_layers] { + send_stream->UpdateActiveSimulcastLayers(active_layers); + thread_sync_event_.Set(); + }); + + thread_sync_event_.Wait(rtc::Event::kForever); +} + void VideoSendStream::Start() { RTC_DCHECK_RUN_ON(&thread_checker_); RTC_LOG(LS_INFO) << "VideoSendStream::Start"; @@ -912,6 +932,22 @@ bool VideoSendStreamImpl::DeliverRtcp(const uint8_t* packet, size_t length) { return true; } +void VideoSendStreamImpl::UpdateActiveSimulcastLayers( + const std::vector active_layers) { + RTC_DCHECK_RUN_ON(worker_queue_); + RTC_DCHECK_EQ(rtp_rtcp_modules_.size(), active_layers.size()); + RTC_LOG(LS_INFO) << "VideoSendStream::UpdateActiveSimulcastLayers"; + bool previously_active = payload_router_.IsActive(); + payload_router_.SetActiveModules(active_layers); + if (!payload_router_.IsActive() && previously_active) { + // Payload router switched from active to inactive. + StopVideoSendStream(); + } else if (payload_router_.IsActive() && !previously_active) { + // Payload router switched from inactive to active. + StartupVideoSendStream(); + } +} + void VideoSendStreamImpl::Start() { RTC_DCHECK_RUN_ON(worker_queue_); RTC_LOG(LS_INFO) << "VideoSendStream::Start"; @@ -919,12 +955,15 @@ void VideoSendStreamImpl::Start() { return; TRACE_EVENT_INSTANT0("webrtc", "VideoSendStream::Start"); payload_router_.SetActive(true); + StartupVideoSendStream(); +} +void VideoSendStreamImpl::StartupVideoSendStream() { + RTC_DCHECK_RUN_ON(worker_queue_); bitrate_allocator_->AddObserver( this, encoder_min_bitrate_bps_, encoder_max_bitrate_bps_, max_padding_bitrate_, !config_->suspend_below_min_bitrate, config_->track_id, encoder_bitrate_priority_); - // Start monitoring encoder activity. { rtc::CritScope lock(&encoder_activity_crit_sect_); @@ -945,6 +984,10 @@ void VideoSendStreamImpl::Stop() { return; TRACE_EVENT_INSTANT0("webrtc", "VideoSendStream::Stop"); payload_router_.SetActive(false); + StopVideoSendStream(); +} + +void VideoSendStreamImpl::StopVideoSendStream() { bitrate_allocator_->RemoveObserver(this); { rtc::CritScope lock(&encoder_activity_crit_sect_); diff --git a/video/video_send_stream.h b/video/video_send_stream.h index d62c302a85..9212370644 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -72,6 +72,8 @@ class VideoSendStream : public webrtc::VideoSendStream { bool DeliverRtcp(const uint8_t* packet, size_t length); // webrtc::VideoSendStream implementation. + void UpdateActiveSimulcastLayers( + const std::vector active_layers) override; void Start() override; void Stop() override; diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index 3512b8585f..26ac918219 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -1971,62 +1971,62 @@ TEST_F(VideoSendStreamTest, CanReconfigureToUseStartBitrateAbovePreviousMax) { DestroyStreams(); } +class StartStopBitrateObserver : public test::FakeEncoder { + public: + StartStopBitrateObserver() + : FakeEncoder(Clock::GetRealTimeClock()), + encoder_init_(false, false), + bitrate_changed_(false, false) {} + int32_t InitEncode(const VideoCodec* config, + int32_t number_of_cores, + size_t max_payload_size) override { + rtc::CritScope lock(&crit_); + encoder_init_.Set(); + return FakeEncoder::InitEncode(config, number_of_cores, max_payload_size); + } + + int32_t SetRateAllocation(const BitrateAllocation& bitrate, + uint32_t framerate) override { + rtc::CritScope lock(&crit_); + bitrate_kbps_ = bitrate.get_sum_kbps(); + bitrate_changed_.Set(); + return FakeEncoder::SetRateAllocation(bitrate, framerate); + } + + bool WaitForEncoderInit() { + return encoder_init_.Wait(VideoSendStreamTest::kDefaultTimeoutMs); + } + + bool WaitBitrateChanged(bool non_zero) { + do { + rtc::Optional bitrate_kbps; + { + rtc::CritScope lock(&crit_); + bitrate_kbps = bitrate_kbps_; + } + if (!bitrate_kbps) + continue; + + if ((non_zero && *bitrate_kbps > 0) || + (!non_zero && *bitrate_kbps == 0)) { + return true; + } + } while (bitrate_changed_.Wait(VideoSendStreamTest::kDefaultTimeoutMs)); + return false; + } + + private: + rtc::CriticalSection crit_; + rtc::Event encoder_init_; + rtc::Event bitrate_changed_; + rtc::Optional bitrate_kbps_ RTC_GUARDED_BY(crit_); +}; + // This test that if the encoder use an internal source, VideoEncoder::SetRates // will be called with zero bitrate during initialization and that // VideoSendStream::Stop also triggers VideoEncoder::SetRates Start to be called // with zero bitrate. TEST_F(VideoSendStreamTest, VideoSendStreamStopSetEncoderRateToZero) { - class StartStopBitrateObserver : public test::FakeEncoder { - public: - StartStopBitrateObserver() - : FakeEncoder(Clock::GetRealTimeClock()), - encoder_init_(false, false), - bitrate_changed_(false, false) {} - int32_t InitEncode(const VideoCodec* config, - int32_t number_of_cores, - size_t max_payload_size) override { - rtc::CritScope lock(&crit_); - encoder_init_.Set(); - return FakeEncoder::InitEncode(config, number_of_cores, max_payload_size); - } - - int32_t SetRateAllocation(const BitrateAllocation& bitrate, - uint32_t framerate) override { - rtc::CritScope lock(&crit_); - bitrate_kbps_ = bitrate.get_sum_kbps(); - bitrate_changed_.Set(); - return FakeEncoder::SetRateAllocation(bitrate, framerate); - } - - bool WaitForEncoderInit() { - return encoder_init_.Wait(VideoSendStreamTest::kDefaultTimeoutMs); - } - - bool WaitBitrateChanged(bool non_zero) { - do { - rtc::Optional bitrate_kbps; - { - rtc::CritScope lock(&crit_); - bitrate_kbps = bitrate_kbps_; - } - if (!bitrate_kbps) - continue; - - if ((non_zero && *bitrate_kbps > 0) || - (!non_zero && *bitrate_kbps == 0)) { - return true; - } - } while (bitrate_changed_.Wait(VideoSendStreamTest::kDefaultTimeoutMs)); - return false; - } - - private: - rtc::CriticalSection crit_; - rtc::Event encoder_init_; - rtc::Event bitrate_changed_; - rtc::Optional bitrate_kbps_ RTC_GUARDED_BY(crit_); - }; - test::NullTransport transport; StartStopBitrateObserver encoder; @@ -2065,6 +2065,64 @@ TEST_F(VideoSendStreamTest, VideoSendStreamStopSetEncoderRateToZero) { }); } +// Tests that when the encoder uses an internal source, the VideoEncoder will +// be updated with a new bitrate when turning the VideoSendStream on/off with +// VideoSendStream::UpdateActiveSimulcastLayers, and when the VideoStreamEncoder +// is reconfigured with new active layers. +TEST_F(VideoSendStreamTest, VideoSendStreamUpdateActiveSimulcastLayers) { + test::NullTransport transport; + StartStopBitrateObserver encoder; + + task_queue_.SendTask([this, &transport, &encoder]() { + CreateSenderCall(Call::Config(event_log_.get())); + // Create two simulcast streams. + CreateSendConfig(2, 0, 0, &transport); + + sender_call_->SignalChannelNetworkState(MediaType::VIDEO, kNetworkUp); + + video_send_config_.encoder_settings.encoder = &encoder; + video_send_config_.encoder_settings.internal_source = true; + video_send_config_.encoder_settings.payload_name = "VP8"; + + CreateVideoStreams(); + }); + + EXPECT_TRUE(encoder.WaitForEncoderInit()); + + // When we turn on the simulcast layers it will update the BitrateAllocator, + // which in turn updates the VideoEncoder's bitrate. + task_queue_.SendTask([this]() { + video_send_stream_->UpdateActiveSimulcastLayers({true, true}); + }); + EXPECT_TRUE(encoder.WaitBitrateChanged(true)); + + video_encoder_config_.simulcast_layers[0].active = true; + video_encoder_config_.simulcast_layers[1].active = false; + task_queue_.SendTask([this]() { + video_send_stream_->ReconfigureVideoEncoder(video_encoder_config_.Copy()); + }); + // TODO(bugs.webrtc.org/8807): Currently we require a hard reconfiguration to + // update the VideoBitrateAllocator and BitrateAllocator of which layers are + // active. Once the change is made for a "soft" reconfiguration we can remove + // the expecation for an encoder init. We can also test that bitrate changes + // when just updating individual active layers, which should change the + // bitrate set to the video encoder. + EXPECT_TRUE(encoder.WaitForEncoderInit()); + EXPECT_TRUE(encoder.WaitBitrateChanged(true)); + + // Turning off both simulcast layers should trigger a bitrate change of 0. + video_encoder_config_.simulcast_layers[0].active = false; + video_encoder_config_.simulcast_layers[1].active = false; + task_queue_.SendTask([this]() { + video_send_stream_->UpdateActiveSimulcastLayers({false, false}); + }); + EXPECT_TRUE(encoder.WaitBitrateChanged(false)); + + task_queue_.SendTask([this]() { + DestroyStreams(); + DestroyCalls(); + }); +} TEST_F(VideoSendStreamTest, CapturesTextureAndVideoFrames) { class FrameObserver : public rtc::VideoSinkInterface { public: diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 520388566d..778f740a20 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -564,6 +564,10 @@ void VideoStreamEncoder::ConfigureEncoderOnTaskQueue( } } +// TODO(bugs.webrtc.org/8807): Currently this always does a hard +// reconfiguration, but this isn't always necessary. Add in logic to only update +// the VideoBitrateAllocator and call OnEncoderConfigurationChanged with a +// "soft" reconfiguration. void VideoStreamEncoder::ReconfigureEncoder() { RTC_DCHECK_RUN_ON(&encoder_queue_); RTC_DCHECK(pending_encoder_reconfiguration_);