From dcef6410b3691c0454c043671f913d629c708972 Mon Sep 17 00:00:00 2001 From: Per Kjellander Date: Wed, 7 Oct 2020 15:09:05 +0200 Subject: [PATCH] Stop using VideoBitrateAllocationObserver in VideoStreamEncoder. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VideoBitrateAllocation is instead reported through the EncoderSink. Enable VideoBitrateAllocation reporting from WebRtcVideoChannel::AddSendStream in preparation for using the extension RtpVideoLayersAllocationExtension instead of RTCP XR. Bug: webrtc:12000 Change-Id: I5ea8e4f237a1c4e84a89cbfd97ac4353d4c2984f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/186940 Commit-Queue: Per Kjellander Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#32347} --- api/video/video_stream_encoder_interface.h | 8 +- api/video/video_stream_encoder_settings.h | 11 ++ media/engine/webrtc_video_engine.cc | 9 ++ .../extended_reports_tests.cc | 18 +++- video/test/mock_video_stream_encoder.h | 4 - video/video_send_stream.cc | 9 -- video/video_send_stream_impl.h | 11 +- video/video_send_stream_impl_unittest.cc | 46 ++++---- video/video_stream_encoder.cc | 24 ++--- video/video_stream_encoder.h | 5 - video/video_stream_encoder_unittest.cc | 100 +++++++++++------- 11 files changed, 142 insertions(+), 103 deletions(-) diff --git a/api/video/video_stream_encoder_interface.h b/api/video/video_stream_encoder_interface.h index d8dd8e1599..ed9989c37e 100644 --- a/api/video/video_stream_encoder_interface.h +++ b/api/video/video_stream_encoder_interface.h @@ -49,6 +49,9 @@ class VideoStreamEncoderInterface : public rtc::VideoSinkInterface { bool is_svc, VideoEncoderConfig::ContentType content_type, int min_transmit_bitrate_bps) = 0; + + virtual void OnBitrateAllocationUpdated( + const VideoBitrateAllocation& allocation) = 0; }; // If the resource is overusing, the VideoStreamEncoder will try to reduce @@ -110,11 +113,6 @@ class VideoStreamEncoderInterface : public rtc::VideoSinkInterface { int64_t round_trip_time_ms, double cwnd_reduce_ratio) = 0; - // Register observer for the bitrate allocation between the temporal - // and spatial layers. - virtual void SetBitrateAllocationObserver( - VideoBitrateAllocationObserver* bitrate_observer) = 0; - // Set a FecControllerOverride, through which the encoder may override // decisions made by FecController. virtual void SetFecControllerOverride( diff --git a/api/video/video_stream_encoder_settings.h b/api/video/video_stream_encoder_settings.h index 743524b352..cbeed3d07a 100644 --- a/api/video/video_stream_encoder_settings.h +++ b/api/video/video_stream_encoder_settings.h @@ -39,6 +39,12 @@ class EncoderSwitchRequestCallback { }; struct VideoStreamEncoderSettings { + enum class BitrateAllocationCallbackType { + kVideoBitrateAllocation, + kVideoBitrateAllocationWhenScreenSharing, + kVideoLayersAllocation + }; + explicit VideoStreamEncoderSettings( const VideoEncoder::Capabilities& capabilities) : capabilities(capabilities) {} @@ -59,6 +65,11 @@ struct VideoStreamEncoderSettings { // Negotiated capabilities which the VideoEncoder may expect the other // side to use. VideoEncoder::Capabilities capabilities; + + // TODO(bugs.webrtc.org/12000): Reporting of VideoBitrateAllocation is beeing + // deprecated. Instead VideoLayersAllocation should be reported. + BitrateAllocationCallbackType allocation_cb_type = + BitrateAllocationCallbackType::kVideoBitrateAllocationWhenScreenSharing; }; } // namespace webrtc diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 43ecf9dc4b..8ac97d56bf 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1297,6 +1297,15 @@ bool WebRtcVideoChannel::AddSendStream(const StreamParams& sp) { video_config_.periodic_alr_bandwidth_probing; config.encoder_settings.experiment_cpu_load_estimator = video_config_.experiment_cpu_load_estimator; + // TODO(bugs.webrtc.org/12000): Enable allocation callback type + // VideoLayersAllocation if RtpVideoLayersAllocationExtension has been + // negotiated in `send_rtp_extensions_`. + config.encoder_settings.allocation_cb_type = + IsEnabled(call_->trials(), "WebRTC-Target-Bitrate-Rtcp") + ? webrtc::VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation + : webrtc::VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocationWhenScreenSharing; config.encoder_settings.encoder_factory = encoder_factory_; config.encoder_settings.bitrate_allocator_factory = bitrate_allocator_factory_; diff --git a/video/end_to_end_tests/extended_reports_tests.cc b/video/end_to_end_tests/extended_reports_tests.cc index ed73800fd7..b5e162e413 100644 --- a/video/end_to_end_tests/extended_reports_tests.cc +++ b/video/end_to_end_tests/extended_reports_tests.cc @@ -62,11 +62,13 @@ class RtcpXrObserver : public test::EndToEndTest { RtcpXrObserver(bool enable_rrtr, bool expect_target_bitrate, bool enable_zero_target_bitrate, + bool enable_target_bitrate, VideoEncoderConfig::ContentType content_type) : EndToEndTest(test::CallTest::kDefaultTimeoutMs), enable_rrtr_(enable_rrtr), expect_target_bitrate_(expect_target_bitrate), enable_zero_target_bitrate_(enable_zero_target_bitrate), + enable_target_bitrate_(enable_target_bitrate), content_type_(content_type), sent_rtcp_sr_(0), sent_rtcp_rr_(0), @@ -175,6 +177,12 @@ class RtcpXrObserver : public test::EndToEndTest { VideoSendStream::Config* send_config, std::vector* receive_configs, VideoEncoderConfig* encoder_config) override { + if (enable_target_bitrate_) { + send_config->encoder_settings.allocation_cb_type = + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation; + } + if (enable_zero_target_bitrate_) { // Configure VP8 to be able to use simulcast. send_config->rtp.payload_name = "VP8"; @@ -202,6 +210,7 @@ class RtcpXrObserver : public test::EndToEndTest { const bool enable_rrtr_; const bool expect_target_bitrate_; const bool enable_zero_target_bitrate_; + const bool enable_target_bitrate_; const VideoEncoderConfig::ContentType content_type_; int sent_rtcp_sr_; int sent_rtcp_rr_ RTC_GUARDED_BY(&mutex_); @@ -217,6 +226,7 @@ TEST_F(ExtendedReportsEndToEndTest, TestExtendedReportsWithRrtrWithoutTargetBitrate) { RtcpXrObserver test(/*enable_rrtr=*/true, /*expect_target_bitrate=*/false, /*enable_zero_target_bitrate=*/false, + /*enable_target_bitrate=*/false, VideoEncoderConfig::ContentType::kRealtimeVideo); RunBaseTest(&test); } @@ -225,6 +235,7 @@ TEST_F(ExtendedReportsEndToEndTest, TestExtendedReportsWithoutRrtrWithoutTargetBitrate) { RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/false, /*enable_zero_target_bitrate=*/false, + /*enable_target_bitrate=*/false, VideoEncoderConfig::ContentType::kRealtimeVideo); RunBaseTest(&test); } @@ -233,6 +244,7 @@ TEST_F(ExtendedReportsEndToEndTest, TestExtendedReportsWithRrtrWithTargetBitrate) { RtcpXrObserver test(/*enable_rrtr=*/true, /*expect_target_bitrate=*/true, /*enable_zero_target_bitrate=*/false, + /*enable_target_bitrate=*/false, VideoEncoderConfig::ContentType::kScreen); RunBaseTest(&test); } @@ -241,15 +253,16 @@ TEST_F(ExtendedReportsEndToEndTest, TestExtendedReportsWithoutRrtrWithTargetBitrate) { RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/true, /*enable_zero_target_bitrate=*/false, + /*enable_target_bitrate=*/false, VideoEncoderConfig::ContentType::kScreen); RunBaseTest(&test); } TEST_F(ExtendedReportsEndToEndTest, - TestExtendedReportsWithoutRrtrWithTargetBitrateFromFieldTrial) { - test::ScopedFieldTrials field_trials("WebRTC-Target-Bitrate-Rtcp/Enabled/"); + TestExtendedReportsWithoutRrtrWithTargetBitrateExplicitlySet) { RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/true, /*enable_zero_target_bitrate=*/false, + /*enable_target_bitrate=*/true, VideoEncoderConfig::ContentType::kRealtimeVideo); RunBaseTest(&test); } @@ -258,6 +271,7 @@ TEST_F(ExtendedReportsEndToEndTest, TestExtendedReportsCanSignalZeroTargetBitrate) { RtcpXrObserver test(/*enable_rrtr=*/false, /*expect_target_bitrate=*/true, /*enable_zero_target_bitrate=*/true, + /*enable_target_bitrate=*/false, VideoEncoderConfig::ContentType::kScreen); RunBaseTest(&test); } diff --git a/video/test/mock_video_stream_encoder.h b/video/test/mock_video_stream_encoder.h index c9efc76598..2af613e3ad 100644 --- a/video/test/mock_video_stream_encoder.h +++ b/video/test/mock_video_stream_encoder.h @@ -44,10 +44,6 @@ class MockVideoStreamEncoder : public VideoStreamEncoderInterface { (DataRate, DataRate, DataRate, uint8_t, int64_t, double), (override)); MOCK_METHOD(void, OnFrame, (const VideoFrame&), (override)); - MOCK_METHOD(void, - SetBitrateAllocationObserver, - (VideoBitrateAllocationObserver*), - (override)); MOCK_METHOD(void, SetFecControllerOverride, (FecControllerOverride*), diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 30ed86dbd1..d6e1b6bbfa 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -29,8 +29,6 @@ namespace webrtc { namespace { -constexpr char kTargetBitrateRtcpFieldTrial[] = "WebRTC-Target-Bitrate-Rtcp"; - size_t CalculateMaxHeaderSize(const RtpConfig& config) { size_t header_size = kRtpHeaderSize; size_t extensions_size = 0; @@ -113,13 +111,6 @@ VideoSendStream::VideoSendStream( // it was created on. thread_sync_event_.Wait(rtc::Event::kForever); send_stream_->RegisterProcessThread(module_process_thread); - // TODO(sprang): Enable this also for regular video calls by default, if it - // works well. - if (encoder_config.content_type == VideoEncoderConfig::ContentType::kScreen || - field_trial::IsEnabled(kTargetBitrateRtcpFieldTrial)) { - video_stream_encoder_->SetBitrateAllocationObserver(send_stream_.get()); - } - ReconfigureVideoEncoder(std::move(encoder_config)); } diff --git a/video/video_send_stream_impl.h b/video/video_send_stream_impl.h index 1f79b9365f..d3f50584f6 100644 --- a/video/video_send_stream_impl.h +++ b/video/video_send_stream_impl.h @@ -67,8 +67,7 @@ struct PacingConfig { // An encoder may deliver frames through the EncodedImageCallback on an // arbitrary thread. class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, - public VideoStreamEncoderInterface::EncoderSink, - public VideoBitrateAllocationObserver { + public VideoStreamEncoderInterface::EncoderSink { public: VideoSendStreamImpl( Clock* clock, @@ -113,12 +112,16 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, // Implements BitrateAllocatorObserver. uint32_t OnBitrateUpdated(BitrateAllocationUpdate update) override; + // Implements VideoStreamEncoderInterface::EncoderSink void OnEncoderConfigurationChanged( std::vector streams, bool is_svc, VideoEncoderConfig::ContentType content_type, int min_transmit_bitrate_bps) override; + void OnBitrateAllocationUpdated( + const VideoBitrateAllocation& allocation) override; + // Implements EncodedImageCallback. The implementation routes encoded frames // to the |payload_router_| and |config.pre_encode_callback| if set. // Called on an arbitrary encoder callback thread. @@ -129,10 +132,6 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, // Implements EncodedImageCallback. void OnDroppedFrame(EncodedImageCallback::DropReason reason) override; - // Implements VideoBitrateAllocationObserver. - void OnBitrateAllocationUpdated( - const VideoBitrateAllocation& allocation) override; - // Starts monitoring and sends a keyframe. void StartupVideoSendStream(); // Removes the bitrate observer, stops monitoring and notifies the video diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index bdda0f8e96..68813ca21b 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -435,9 +435,10 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) { auto vss_impl = CreateVideoSendStreamImpl( kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kScreen); + VideoStreamEncoderInterface::EncoderSink* const sink = + static_cast( + vss_impl.get()); vss_impl->Start(); - VideoBitrateAllocationObserver* const observer = - static_cast(vss_impl.get()); // Populate a test instance of video bitrate allocation. VideoBitrateAllocation alloc; @@ -449,7 +450,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) { // Encoder starts out paused, don't forward allocation. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(0); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); // Unpause encoder, allocation should be passed through. const uint32_t kBitrateBps = 100000; @@ -460,7 +461,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) { ->OnBitrateUpdated(CreateAllocation(kBitrateBps)); EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); // Pause encoder again, and block allocations. EXPECT_CALL(rtp_video_sender_, GetPayloadBitrateBps()) @@ -470,7 +471,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationWhenEnabled) { ->OnBitrateUpdated(CreateAllocation(0)); EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(0); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); vss_impl->Stop(); }, @@ -491,8 +492,9 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) { .WillOnce(Return(kBitrateBps)); static_cast(vss_impl.get()) ->OnBitrateUpdated(CreateAllocation(kBitrateBps)); - VideoBitrateAllocationObserver* const observer = - static_cast(vss_impl.get()); + VideoStreamEncoderInterface::EncoderSink* const sink = + static_cast( + vss_impl.get()); // Populate a test instance of video bitrate allocation. VideoBitrateAllocation alloc; @@ -504,7 +506,7 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) { // Initial value. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); VideoBitrateAllocation updated_alloc = alloc; // Needs 10% increase in bitrate to trigger immediate forward. @@ -514,14 +516,14 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) { // Too small increase, don't forward. updated_alloc.SetBitrate(0, 0, base_layer_min_update_bitrate_bps - 1); EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(_)).Times(0); - observer->OnBitrateAllocationUpdated(updated_alloc); + sink->OnBitrateAllocationUpdated(updated_alloc); // Large enough increase, do forward. updated_alloc.SetBitrate(0, 0, base_layer_min_update_bitrate_bps); EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(updated_alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(updated_alloc); + sink->OnBitrateAllocationUpdated(updated_alloc); // This is now a decrease compared to last forward allocation, forward // immediately. @@ -529,7 +531,7 @@ TEST_F(VideoSendStreamImplTest, ThrottlesVideoBitrateAllocationWhenTooSimilar) { EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(updated_alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(updated_alloc); + sink->OnBitrateAllocationUpdated(updated_alloc); vss_impl->Stop(); }, @@ -550,8 +552,9 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) { .WillOnce(Return(kBitrateBps)); static_cast(vss_impl.get()) ->OnBitrateUpdated(CreateAllocation(kBitrateBps)); - VideoBitrateAllocationObserver* const observer = - static_cast(vss_impl.get()); + VideoStreamEncoderInterface::EncoderSink* const sink = + static_cast( + vss_impl.get()); // Populate a test instance of video bitrate allocation. VideoBitrateAllocation alloc; @@ -563,7 +566,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) { // Initial value. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); // Move some bitrate from one layer to a new one, but keep sum the same. // Since layout has changed, immediately trigger forward. @@ -574,7 +577,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) { EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(updated_alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(updated_alloc); + sink->OnBitrateAllocationUpdated(updated_alloc); vss_impl->Stop(); }, @@ -595,8 +598,9 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { .WillRepeatedly(Return(kBitrateBps)); static_cast(vss_impl.get()) ->OnBitrateUpdated(CreateAllocation(kBitrateBps)); - VideoBitrateAllocationObserver* const observer = - static_cast(vss_impl.get()); + VideoStreamEncoderInterface::EncoderSink* const sink = + static_cast( + vss_impl.get()); // Populate a test instance of video bitrate allocation. VideoBitrateAllocation alloc; @@ -618,14 +622,14 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { // Initial value. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); } { // Sending same allocation again, this one should be throttled. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(0); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); } clock_.AdvanceTimeMicroseconds(kMaxVbaThrottleTimeMs * 1000); @@ -634,14 +638,14 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { // Sending similar allocation again after timeout, should forward. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(1); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); } { // Sending similar allocation again without timeout, throttle. EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(0); - observer->OnBitrateAllocationUpdated(alloc); + sink->OnBitrateAllocationUpdated(alloc); } { diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 1265c17d1d..6a8b57cf49 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -26,6 +26,7 @@ #include "api/video/video_adaptation_reason.h" #include "api/video/video_bitrate_allocator_factory.h" #include "api/video/video_codec_constants.h" +#include "api/video/video_layers_allocation.h" #include "api/video_codecs/video_encoder.h" #include "call/adaptation/resource_adaptation_processor.h" #include "call/adaptation/video_stream_adapter.h" @@ -324,7 +325,6 @@ VideoStreamEncoder::VideoStreamEncoder( animation_start_time_(Timestamp::PlusInfinity()), cap_resolution_due_to_video_content_(false), expect_resize_state_(ExpectResizeState::kNoResize), - bitrate_observer_(nullptr), fec_controller_override_(nullptr), force_disable_frame_dropper_(false), input_framerate_(kFrameRateAvergingWindowSizeMs, 1000), @@ -418,23 +418,12 @@ void VideoStreamEncoder::Stop() { resource_adaptation_processor_.reset(); } rate_allocator_ = nullptr; - bitrate_observer_ = nullptr; ReleaseEncoder(); shutdown_event.Set(); }); shutdown_event.Wait(rtc::Event::kForever); } -void VideoStreamEncoder::SetBitrateAllocationObserver( - VideoBitrateAllocationObserver* bitrate_observer) { - RTC_DCHECK_RUN_ON(main_queue_); - encoder_queue_.PostTask([this, bitrate_observer] { - RTC_DCHECK_RUN_ON(&encoder_queue_); - RTC_DCHECK(!bitrate_observer_); - bitrate_observer_ = bitrate_observer; - }); -} - void VideoStreamEncoder::SetFecControllerOverride( FecControllerOverride* fec_controller_override) { encoder_queue_.PostTask([this, fec_controller_override] { @@ -1136,8 +1125,15 @@ void VideoStreamEncoder::SetEncoderRates( static_cast(rate_settings.rate_control.framerate_fps + 0.5)); stream_resource_manager_.SetEncoderRates(rate_settings.rate_control); } - if (bitrate_observer_) { - bitrate_observer_->OnBitrateAllocationUpdated( + if ((settings_.allocation_cb_type == + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation) || + (encoder_config_.content_type == + VideoEncoderConfig::ContentType::kScreen && + settings_.allocation_cb_type == + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocationWhenScreenSharing)) { + sink_->OnBitrateAllocationUpdated( // Update allocation according to info from encoder. An encoder may // choose to not use all layers due to for example HW. UpdateAllocationFromEncoderInfo( diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 4c0f84b105..7dfc990846 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -80,9 +80,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // TODO(perkj): Can we remove VideoCodec.startBitrate ? void SetStartBitrate(int start_bitrate_bps) override; - void SetBitrateAllocationObserver( - VideoBitrateAllocationObserver* bitrate_observer) override; - void SetFecControllerOverride( FecControllerOverride* fec_controller_override) override; @@ -301,8 +298,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, kFirstFrameAfterResize // Resize observed. } expect_resize_state_ RTC_GUARDED_BY(&encoder_queue_); - VideoBitrateAllocationObserver* bitrate_observer_ - RTC_GUARDED_BY(&encoder_queue_); FecControllerOverride* fec_controller_override_ RTC_GUARDED_BY(&encoder_queue_); absl::optional last_parameters_update_ms_ diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 51f177cb85..5b144b18a3 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -607,14 +607,6 @@ class MockableSendStatisticsProxy : public SendStatisticsProxy { std::function on_frame_dropped_; }; -class MockBitrateObserver : public VideoBitrateAllocationObserver { - public: - MOCK_METHOD(void, - OnBitrateAllocationUpdated, - (const VideoBitrateAllocation&), - (override)); -}; - class MockEncoderSelector : public VideoEncoderFactory::EncoderSelectorInterface { public: @@ -687,12 +679,18 @@ class VideoStreamEncoderTest : public ::testing::Test { video_stream_encoder_->WaitUntilTaskQueueIsIdle(); } - void ResetEncoder(const std::string& payload_name, - size_t num_streams, - size_t num_temporal_layers, - unsigned char num_spatial_layers, - bool screenshare) { + void ResetEncoder( + const std::string& payload_name, + size_t num_streams, + size_t num_temporal_layers, + unsigned char num_spatial_layers, + bool screenshare, + VideoStreamEncoderSettings::BitrateAllocationCallbackType + allocation_cb_type = + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocationWhenScreenSharing) { video_send_config_.rtp.payload_name = payload_name; + video_send_config_.encoder_settings.allocation_cb_type = allocation_cb_type; VideoEncoderConfig video_encoder_config; video_encoder_config.codec_type = PayloadStringToCodecType(payload_name); @@ -812,11 +810,6 @@ class VideoStreamEncoderTest : public ::testing::Test { } void VerifyAllocatedBitrate(const VideoBitrateAllocation& expected_bitrate) { - MockBitrateObserver bitrate_observer; - video_stream_encoder_->SetBitrateAllocationObserver(&bitrate_observer); - - EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate)) - .Times(1); video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( DataRate::BitsPerSec(kTargetBitrateBps), DataRate::BitsPerSec(kTargetBitrateBps), @@ -825,6 +818,7 @@ class VideoStreamEncoderTest : public ::testing::Test { video_source_.IncomingCapturedFrame( CreateFrame(1, codec_width_, codec_height_)); WaitForEncodedFrame(1); + EXPECT_EQ(expected_bitrate, sink_.GetLastVideoBitrateAllocation()); } void WaitForEncodedFrame(int64_t expected_ntp_time) { @@ -1264,6 +1258,16 @@ class VideoStreamEncoderTest : public ::testing::Test { return std::move(last_encoded_image_data_); } + VideoBitrateAllocation GetLastVideoBitrateAllocation() { + MutexLock lock(&mutex_); + return last_bitrate_allocation_; + } + + int number_of_bitrate_allocations() const { + MutexLock lock(&mutex_); + return number_of_bitrate_allocations_; + } + private: Result OnEncodedImage( const EncodedImage& encoded_image, @@ -1299,6 +1303,13 @@ class VideoStreamEncoderTest : public ::testing::Test { min_transmit_bitrate_bps_ = min_transmit_bitrate_bps; } + void OnBitrateAllocationUpdated( + const VideoBitrateAllocation& allocation) override { + MutexLock lock(&mutex_); + ++number_of_bitrate_allocations_; + last_bitrate_allocation_ = allocation; + } + TimeController* const time_controller_; mutable Mutex mutex_; TestEncoder* test_encoder_; @@ -1314,6 +1325,8 @@ class VideoStreamEncoderTest : public ::testing::Test { bool expect_frames_ = true; int number_of_reconfigurations_ = 0; int min_transmit_bitrate_bps_ = 0; + VideoBitrateAllocation last_bitrate_allocation_ RTC_GUARDED_BY(&mutex_); + int number_of_bitrate_allocations_ RTC_GUARDED_BY(&mutex_) = 0; }; class VideoBitrateAllocatorProxyFactory @@ -3915,9 +3928,10 @@ TEST_F(VideoStreamEncoderTest, metrics::NumSamples("WebRTC.Video.CpuLimitedResolutionInPercent")); } -TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { - MockBitrateObserver bitrate_observer; - video_stream_encoder_->SetBitrateAllocationObserver(&bitrate_observer); +TEST_F(VideoStreamEncoderTest, ReportsVideoBitrateAllocation) { + ResetEncoder("FAKE", 1, 1, 1, /*screenshare*/ false, + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation); const int kDefaultFps = 30; const VideoBitrateAllocation expected_bitrate = @@ -3925,8 +3939,6 @@ TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { .Allocate(VideoBitrateAllocationParameters(kLowTargetBitrateBps, kDefaultFps)); - EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate)) - .Times(1); video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( DataRate::BitsPerSec(kLowTargetBitrateBps), DataRate::BitsPerSec(kLowTargetBitrateBps), @@ -3935,23 +3947,23 @@ TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { video_source_.IncomingCapturedFrame( CreateFrame(CurrentTimeMs(), codec_width_, codec_height_)); WaitForEncodedFrame(CurrentTimeMs()); + EXPECT_EQ(sink_.GetLastVideoBitrateAllocation(), expected_bitrate); + EXPECT_EQ(sink_.number_of_bitrate_allocations(), 1); + VideoBitrateAllocation bitrate_allocation = fake_encoder_.GetAndResetLastRateControlSettings()->bitrate; // Check that encoder has been updated too, not just allocation observer. EXPECT_EQ(bitrate_allocation.get_sum_bps(), kLowTargetBitrateBps); AdvanceTime(TimeDelta::Seconds(1) / kDefaultFps); - // Not called on second frame. - EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate)) - .Times(0); + // VideoBitrateAllocation not updated on second frame. video_source_.IncomingCapturedFrame( CreateFrame(CurrentTimeMs(), codec_width_, codec_height_)); WaitForEncodedFrame(CurrentTimeMs()); + EXPECT_EQ(sink_.number_of_bitrate_allocations(), 1); AdvanceTime(TimeDelta::Millis(1) / kDefaultFps); - // Called after a process interval. - EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(expected_bitrate)) - .Times(AtLeast(3)); + // VideoBitrateAllocation updated after a process interval. const int64_t start_time_ms = CurrentTimeMs(); while (CurrentTimeMs() - start_time_ms < 5 * kProcessIntervalMs) { video_source_.IncomingCapturedFrame( @@ -3959,6 +3971,7 @@ TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { WaitForEncodedFrame(CurrentTimeMs()); AdvanceTime(TimeDelta::Millis(1) / kDefaultFps); } + EXPECT_GT(sink_.number_of_bitrate_allocations(), 3); video_stream_encoder_->Stop(); } @@ -3966,7 +3979,9 @@ TEST_F(VideoStreamEncoderTest, CallsBitrateObserver) { TEST_F(VideoStreamEncoderTest, TemporalLayersNotDisabledIfSupported) { // 2 TLs configured, temporal layers supported by encoder. const int kNumTemporalLayers = 2; - ResetEncoder("VP8", 1, kNumTemporalLayers, 1, /*screenshare*/ false); + ResetEncoder("VP8", 1, kNumTemporalLayers, 1, /*screenshare*/ false, + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation); fake_encoder_.SetTemporalLayersSupported(0, true); // Bitrate allocated across temporal layers. @@ -3988,7 +4003,9 @@ TEST_F(VideoStreamEncoderTest, TemporalLayersNotDisabledIfSupported) { TEST_F(VideoStreamEncoderTest, TemporalLayersDisabledIfNotSupported) { // 2 TLs configured, temporal layers not supported by encoder. - ResetEncoder("VP8", 1, /*num_temporal_layers*/ 2, 1, /*screenshare*/ false); + ResetEncoder("VP8", 1, /*num_temporal_layers*/ 2, 1, /*screenshare*/ false, + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation); fake_encoder_.SetTemporalLayersSupported(0, false); // Temporal layers not supported by the encoder. @@ -4001,8 +4018,16 @@ TEST_F(VideoStreamEncoderTest, TemporalLayersDisabledIfNotSupported) { } TEST_F(VideoStreamEncoderTest, VerifyBitrateAllocationForTwoStreams) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-Video-QualityScalerSettings/" + "initial_bitrate_interval_ms:1000,initial_bitrate_factor:0.2/"); + // Reset encoder for field trials to take effect. + ConfigureEncoder(video_encoder_config_.Copy()); + // 2 TLs configured, temporal layers only supported for first stream. - ResetEncoder("VP8", 2, /*num_temporal_layers*/ 2, 1, /*screenshare*/ false); + ResetEncoder("VP8", 2, /*num_temporal_layers*/ 2, 1, /*screenshare*/ false, + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation); fake_encoder_.SetTemporalLayersSupported(0, true); fake_encoder_.SetTemporalLayersSupported(1, false); @@ -5294,9 +5319,10 @@ TEST_F(VideoStreamEncoderTest, DoesNotUpdateBitrateAllocationWhenSuspended) { const int kFrameWidth = 1280; const int kFrameHeight = 720; const int kTargetBitrateBps = 1000000; + ResetEncoder("FAKE", 1, 1, 1, false, + VideoStreamEncoderSettings::BitrateAllocationCallbackType:: + kVideoBitrateAllocation); - MockBitrateObserver bitrate_observer; - video_stream_encoder_->SetBitrateAllocationObserver(&bitrate_observer); video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( DataRate::BitsPerSec(kTargetBitrateBps), DataRate::BitsPerSec(kTargetBitrateBps), @@ -5305,10 +5331,10 @@ TEST_F(VideoStreamEncoderTest, DoesNotUpdateBitrateAllocationWhenSuspended) { // Insert a first video frame, causes another bitrate update. int64_t timestamp_ms = CurrentTimeMs(); - EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(_)).Times(1); video_source_.IncomingCapturedFrame( CreateFrame(timestamp_ms, kFrameWidth, kFrameHeight)); WaitForEncodedFrame(timestamp_ms); + EXPECT_EQ(sink_.number_of_bitrate_allocations(), 1); // Next, simulate video suspension due to pacer queue overrun. video_stream_encoder_->OnBitrateUpdatedAndWaitForManagedResources( @@ -5319,11 +5345,11 @@ TEST_F(VideoStreamEncoderTest, DoesNotUpdateBitrateAllocationWhenSuspended) { timestamp_ms += kProcessIntervalMs; AdvanceTime(TimeDelta::Millis(kProcessIntervalMs)); - // Bitrate observer should not be called. - EXPECT_CALL(bitrate_observer, OnBitrateAllocationUpdated(_)).Times(0); + // No more allocations has been made. video_source_.IncomingCapturedFrame( CreateFrame(timestamp_ms, kFrameWidth, kFrameHeight)); ExpectDroppedFrame(); + EXPECT_EQ(sink_.number_of_bitrate_allocations(), 1); video_stream_encoder_->Stop(); }