From ab65d8aab5fe63619033371fca1ce2711c2c2137 Mon Sep 17 00:00:00 2001 From: Ilya Nikolaevskiy Date: Thu, 28 Mar 2019 14:53:04 +0100 Subject: [PATCH] Fix target bitrate RTCP messages behavior for SVC streams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this CL for SVC streams (e.g VP9) still 3 separate RTP_RTCP senders were created. The RTCP target bitrate messages were treated as simulcast and were split and send for each separate spatial layer in a separate SSRC. To fix that an svc flag is now wired to VideoSendStream config and filled based on the encoder config in WebrtcVideoEngine. This flag is used to differentiate between simulcast and SVC mode in RtpVideoSender. Bug: webrtc:10485 Change-Id: Ifa01d12a7d4f01fcbe448ad11e0cc39ab2d1df55 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/129929 Commit-Queue: Ilya Nikolaevskiy Reviewed-by: Niels Moller Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#27345} --- call/rtp_transport_controller_send.cc | 3 ++- call/rtp_transport_controller_send.h | 1 + call/rtp_transport_controller_send_interface.h | 1 + call/rtp_video_sender.cc | 6 ++++-- call/rtp_video_sender.h | 3 +++ call/rtp_video_sender_unittest.cc | 2 +- call/test/mock_rtp_transport_controller_send.h | 3 ++- call/video_send_stream.h | 4 ++++ media/engine/webrtc_video_engine.cc | 1 + video/video_quality_test.cc | 3 +++ video/video_send_stream_impl.cc | 1 + video/video_send_stream_impl_unittest.cc | 2 +- 12 files changed, 24 insertions(+), 6 deletions(-) diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index ab95907ac8..2816550086 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -104,13 +104,14 @@ RtpVideoSenderInterface* RtpTransportControllerSend::CreateRtpVideoSender( const RtpConfig& rtp_config, int rtcp_report_interval_ms, Transport* send_transport, + bool is_svc, const RtpSenderObservers& observers, RtcEventLog* event_log, std::unique_ptr fec_controller, const RtpSenderFrameEncryptionConfig& frame_encryption_config) { video_rtp_senders_.push_back(absl::make_unique( clock_, suspended_ssrcs, states, rtp_config, rtcp_report_interval_ms, - send_transport, observers, + send_transport, is_svc, observers, // TODO(holmer): Remove this circular dependency by injecting // the parts of RtpTransportControllerSendInterface that are really used. this, event_log, &retransmission_rate_limiter_, std::move(fec_controller), diff --git a/call/rtp_transport_controller_send.h b/call/rtp_transport_controller_send.h index c8a9f2cd4e..44135e4c71 100644 --- a/call/rtp_transport_controller_send.h +++ b/call/rtp_transport_controller_send.h @@ -60,6 +60,7 @@ class RtpTransportControllerSend final const RtpConfig& rtp_config, int rtcp_report_interval_ms, Transport* send_transport, + bool is_svc, const RtpSenderObservers& observers, RtcEventLog* event_log, std::unique_ptr fec_controller, diff --git a/call/rtp_transport_controller_send_interface.h b/call/rtp_transport_controller_send_interface.h index d56403542a..32862d8dd4 100644 --- a/call/rtp_transport_controller_send_interface.h +++ b/call/rtp_transport_controller_send_interface.h @@ -104,6 +104,7 @@ class RtpTransportControllerSendInterface { const RtpConfig& rtp_config, int rtcp_report_interval_ms, Transport* send_transport, + bool is_svc, const RtpSenderObservers& observers, RtcEventLog* event_log, std::unique_ptr fec_controller, diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 87edf10a86..748196d06d 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -212,6 +212,7 @@ RtpVideoSender::RtpVideoSender( const RtpConfig& rtp_config, int rtcp_report_interval_ms, Transport* send_transport, + bool is_svc, const RtpSenderObservers& observers, RtpTransportControllerSendInterface* transport, RtcEventLog* event_log, @@ -253,7 +254,8 @@ RtpVideoSender::RtpVideoSender( overhead_bytes_per_packet_(0), encoder_target_rate_bps_(0), frame_counts_(rtp_config.ssrcs.size()), - frame_count_observer_(observers.frame_count_observer) { + frame_count_observer_(observers.frame_count_observer), + is_svc_(is_svc) { RTC_DCHECK_EQ(rtp_config.ssrcs.size(), rtp_streams_.size()); module_process_thread_checker_.DetachFromThread(); // SSRCs are assumed to be sorted in the same order as |rtp_modules|. @@ -448,7 +450,7 @@ void RtpVideoSender::OnBitrateAllocationUpdated( const VideoBitrateAllocation& bitrate) { rtc::CritScope lock(&crit_); if (IsActive()) { - if (rtp_streams_.size() == 1) { + if (rtp_streams_.size() == 1 || is_svc_) { // If spatial scalability is enabled, it is covered by a single stream. rtp_streams_[0].rtp_rtcp->SetVideoBitrateAllocation(bitrate); } else { diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index d50cb7c2d7..5c0094f34e 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -76,6 +76,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, const RtpConfig& rtp_config, int rtcp_report_interval_ms, Transport* send_transport, + bool is_svc, const RtpSenderObservers& observers, RtpTransportControllerSendInterface* transport, RtcEventLog* event_log, @@ -190,6 +191,8 @@ class RtpVideoSender : public RtpVideoSenderInterface, std::vector frame_counts_ RTC_GUARDED_BY(crit_); FrameCountObserver* const frame_count_observer_; + const bool is_svc_; + RTC_DISALLOW_COPY_AND_ASSIGN(RtpVideoSender); }; diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 1a1dd9bdde..e51e72af86 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -99,7 +99,7 @@ class RtpVideoSenderTestFixture { std::map suspended_ssrcs; router_ = absl::make_unique( &clock_, suspended_ssrcs, suspended_payload_states, config_.rtp, - config_.rtcp_report_interval_ms, &transport_, + config_.rtcp_report_interval_ms, &transport_, config_.is_svc, CreateObservers(&call_stats_, &encoder_feedback_, &stats_proxy_, &stats_proxy_, &stats_proxy_, frame_count_observer, &stats_proxy_, &stats_proxy_, &send_delay_stats_), diff --git a/call/test/mock_rtp_transport_controller_send.h b/call/test/mock_rtp_transport_controller_send.h index b5a0a856a6..af44008352 100644 --- a/call/test/mock_rtp_transport_controller_send.h +++ b/call/test/mock_rtp_transport_controller_send.h @@ -32,13 +32,14 @@ namespace webrtc { class MockRtpTransportControllerSend : public RtpTransportControllerSendInterface { public: - MOCK_METHOD9( + MOCK_METHOD10( CreateRtpVideoSender, RtpVideoSenderInterface*(std::map, const std::map&, const RtpConfig&, int rtcp_report_interval_ms, Transport*, + bool, const RtpSenderObservers&, RtcEventLog*, std::unique_ptr, diff --git a/call/video_send_stream.h b/call/video_send_stream.h index 5daec19449..e4f96d7610 100644 --- a/call/video_send_stream.h +++ b/call/video_send_stream.h @@ -151,6 +151,10 @@ class VideoSendStream { // Per PeerConnection cryptography options. CryptoOptions crypto_options; + // Forces spatial scalability to be implemented via spatial layers + // instead of simulcast. + bool is_svc; + private: // Access to the copy constructor is private to force use of the Copy() // method for those exceptional cases where we do use it. diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index a591ee3b49..9dbdc81a5d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2301,6 +2301,7 @@ void WebRtcVideoChannel::WebRtcVideoSendStream::RecreateWebRtcStream() { "payload type the set codec. Ignoring RTX."; config.rtp.rtx.ssrcs.clear(); } + config.is_svc = parameters_.encoder_config.number_of_streams == 1; stream_ = call_->CreateVideoSendStream(std::move(config), parameters_.encoder_config.Copy()); diff --git a/video/video_quality_test.cc b/video/video_quality_test.cc index 23c703e659..b8cf006c87 100644 --- a/video/video_quality_test.cc +++ b/video/video_quality_test.cc @@ -762,6 +762,9 @@ void VideoQualityTest::SetupVideo(Transport* send_transport, video_send_configs_[video_idx].suspend_below_min_bitrate = params_.video[video_idx].suspend_below_min_bitrate; + video_send_configs_[video_idx].is_svc = + params_.ss[video_idx].streams.size() == 1; + video_encoder_configs_[video_idx].number_of_streams = params_.ss[video_idx].streams.size(); video_encoder_configs_[video_idx].max_bitrate_bps = 0; diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index ae905df95a..33ee854271 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -236,6 +236,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( config_->rtp, config_->rtcp_report_interval_ms, config_->send_transport, + config_->is_svc, CreateObservers(call_stats, &encoder_feedback_, stats_proxy_, diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index e8eab842b6..47a1e6656a 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -96,7 +96,7 @@ class VideoSendStreamImplTest : public ::testing::Test { EXPECT_CALL(transport_controller_, packet_router()) .WillRepeatedly(Return(&packet_router_)); EXPECT_CALL(transport_controller_, - CreateRtpVideoSender(_, _, _, _, _, _, _, _, _)) + CreateRtpVideoSender(_, _, _, _, _, _, _, _, _, _)) .WillRepeatedly(Return(&rtp_video_sender_)); EXPECT_CALL(rtp_video_sender_, SetActive(_)) .WillRepeatedly(testing::Invoke(