From a136ed40851a7a58739b70b071a4516f01198f24 Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 30 May 2022 15:08:13 +0200 Subject: [PATCH] Add SetTransportCc to ReceiveStreamInterface. Setting the transport cc flag was only possible post creation for audio receive streams, while video receive streams need to be recreated. This CL moves the setter for transport_cc() to where the getter is and adds boiler plate implementations for the video streams. For audio streams this splits "SetUseTransportCcAndNackHistory" into two methods, SetTransportCc and SetNackHistory. Bug: none Change-Id: Idbec8217aef10ee77907cebaecdc27b4b0fb18e4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264443 Commit-Queue: Tomas Gunnarsson Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/main@{#37038} --- audio/audio_receive_stream.cc | 24 ++++++++++++-------- audio/audio_receive_stream.h | 4 ++-- audio/audio_receive_stream_unittest.cc | 4 ++-- call/audio_receive_stream.h | 3 +-- call/flexfec_receive_stream_impl.h | 6 +++++ call/receive_stream.h | 2 ++ media/engine/fake_webrtc_call.cc | 5 +--- media/engine/fake_webrtc_call.h | 16 +++++++++---- media/engine/webrtc_video_engine_unittest.cc | 8 +++---- media/engine/webrtc_voice_engine.cc | 4 ++-- media/engine/webrtc_voice_engine_unittest.cc | 6 ++--- video/video_receive_stream2.cc | 11 +++++++++ video/video_receive_stream2.h | 3 ++- 13 files changed, 61 insertions(+), 35 deletions(-) diff --git a/audio/audio_receive_stream.cc b/audio/audio_receive_stream.cc index ecbf9bbcb5..afb7f1f894 100644 --- a/audio/audio_receive_stream.cc +++ b/audio/audio_receive_stream.cc @@ -214,6 +214,11 @@ bool AudioReceiveStreamImpl::transport_cc() const { return config_.rtp.transport_cc; } +void AudioReceiveStreamImpl::SetTransportCc(bool transport_cc) { + RTC_DCHECK_RUN_ON(&worker_thread_checker_); + config_.rtp.transport_cc = transport_cc; +} + bool AudioReceiveStreamImpl::IsRunning() const { RTC_DCHECK_RUN_ON(&worker_thread_checker_); return playing_; @@ -233,18 +238,17 @@ void AudioReceiveStreamImpl::SetDecoderMap( channel_receive_->SetReceiveCodecs(config_.decoder_map); } -void AudioReceiveStreamImpl::SetUseTransportCcAndNackHistory( - bool use_transport_cc, - int history_ms) { +void AudioReceiveStreamImpl::SetNackHistory(int history_ms) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); RTC_DCHECK_GE(history_ms, 0); - config_.rtp.transport_cc = use_transport_cc; - if (config_.rtp.nack.rtp_history_ms != history_ms) { - config_.rtp.nack.rtp_history_ms = history_ms; - // TODO(solenberg): Config NACK history window (which is a packet count), - // using the actual packet size for the configured codec. - channel_receive_->SetNACKStatus(history_ms != 0, history_ms / 20); - } + + if (config_.rtp.nack.rtp_history_ms == history_ms) + return; + + config_.rtp.nack.rtp_history_ms = history_ms; + // TODO(solenberg): Config NACK history window (which is a packet count), + // using the actual packet size for the configured codec. + channel_receive_->SetNACKStatus(history_ms != 0, history_ms / 20); } void AudioReceiveStreamImpl::SetNonSenderRttMeasurement(bool enabled) { diff --git a/audio/audio_receive_stream.h b/audio/audio_receive_stream.h index 6debdec7dd..427077fd94 100644 --- a/audio/audio_receive_stream.h +++ b/audio/audio_receive_stream.h @@ -86,13 +86,13 @@ class AudioReceiveStreamImpl final : public webrtc::AudioReceiveStreamInterface, void Start() override; void Stop() override; bool transport_cc() const override; + void SetTransportCc(bool transport_cc) override; bool IsRunning() const override; void SetDepacketizerToDecoderFrameTransformer( rtc::scoped_refptr frame_transformer) override; void SetDecoderMap(std::map decoder_map) override; - void SetUseTransportCcAndNackHistory(bool use_transport_cc, - int history_ms) override; + void SetNackHistory(int history_ms) override; void SetNonSenderRttMeasurement(bool enabled) override; void SetFrameDecryptor(rtc::scoped_refptr frame_decryptor) override; diff --git a/audio/audio_receive_stream_unittest.cc b/audio/audio_receive_stream_unittest.cc index 62d7180805..a581bbb391 100644 --- a/audio/audio_receive_stream_unittest.cc +++ b/audio/audio_receive_stream_unittest.cc @@ -399,8 +399,8 @@ TEST(AudioReceiveStreamTest, ReconfigureWithUpdatedConfig) { recv_stream->SetDecoderMap(new_config.decoder_map); EXPECT_CALL(channel_receive, SetNACKStatus(true, 15 + 1)).Times(1); - recv_stream->SetUseTransportCcAndNackHistory(new_config.rtp.transport_cc, - 300 + 20); + recv_stream->SetTransportCc(new_config.rtp.transport_cc); + recv_stream->SetNackHistory(300 + 20); recv_stream->UnregisterFromTransport(); } diff --git a/call/audio_receive_stream.h b/call/audio_receive_stream.h index a8a7c1e507..faa1f1286e 100644 --- a/call/audio_receive_stream.h +++ b/call/audio_receive_stream.h @@ -161,8 +161,7 @@ class AudioReceiveStreamInterface : public MediaReceiveStreamInterface { // Methods that support reconfiguring the stream post initialization. virtual void SetDecoderMap(std::map decoder_map) = 0; - virtual void SetUseTransportCcAndNackHistory(bool use_transport_cc, - int history_ms) = 0; + virtual void SetNackHistory(int history_ms) = 0; virtual void SetNonSenderRttMeasurement(bool enabled) = 0; // Returns true if the stream has been started. diff --git a/call/flexfec_receive_stream_impl.h b/call/flexfec_receive_stream_impl.h index fe8675468f..802f91d2a8 100644 --- a/call/flexfec_receive_stream_impl.h +++ b/call/flexfec_receive_stream_impl.h @@ -64,11 +64,17 @@ class FlexfecReceiveStreamImpl : public FlexfecReceiveStream { void SetLocalSsrc(uint32_t local_ssrc); uint32_t remote_ssrc() const { return remote_ssrc_; } + bool transport_cc() const override { RTC_DCHECK_RUN_ON(&packet_sequence_checker_); return transport_cc_; } + void SetTransportCc(bool transport_cc) override { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + transport_cc_ = transport_cc; + } + private: RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_sequence_checker_; diff --git a/call/receive_stream.h b/call/receive_stream.h index 5c40e085b7..eb04653000 100644 --- a/call/receive_stream.h +++ b/call/receive_stream.h @@ -67,6 +67,8 @@ class ReceiveStreamInterface { // that the value is read on (i.e. packet delivery). virtual bool transport_cc() const = 0; + virtual void SetTransportCc(bool transport_cc) = 0; + protected: virtual ~ReceiveStreamInterface() {} }; diff --git a/media/engine/fake_webrtc_call.cc b/media/engine/fake_webrtc_call.cc index b7ffd0f301..52f4965b9e 100644 --- a/media/engine/fake_webrtc_call.cc +++ b/media/engine/fake_webrtc_call.cc @@ -110,10 +110,7 @@ void FakeAudioReceiveStream::SetDecoderMap( config_.decoder_map = std::move(decoder_map); } -void FakeAudioReceiveStream::SetUseTransportCcAndNackHistory( - bool use_transport_cc, - int history_ms) { - config_.rtp.transport_cc = use_transport_cc; +void FakeAudioReceiveStream::SetNackHistory(int history_ms) { config_.rtp.nack.rtp_history_ms = history_ms; } diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index 8dca0dddcf..2a6072fbf6 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -111,8 +111,10 @@ class FakeAudioReceiveStream final config_.sync_group = std::string(sync_group); } - private: bool transport_cc() const override { return config_.rtp.transport_cc; } + void SetTransportCc(bool transport_cc) override { + config_.rtp.transport_cc = transport_cc; + } uint32_t remote_ssrc() const override { return config_.rtp.remote_ssrc; } void Start() override { started_ = true; } void Stop() override { started_ = false; } @@ -122,8 +124,7 @@ class FakeAudioReceiveStream final override; void SetDecoderMap( std::map decoder_map) override; - void SetUseTransportCcAndNackHistory(bool use_transport_cc, - int history_ms) override; + void SetNackHistory(int history_ms) override; void SetNonSenderRttMeasurement(bool enabled) override; void SetFrameDecryptor(rtc::scoped_refptr frame_decryptor) override; @@ -146,6 +147,7 @@ class FakeAudioReceiveStream final return std::vector(); } + private: int id_ = -1; webrtc::AudioReceiveStreamInterface::Config config_; webrtc::AudioReceiveStreamInterface::Stats stats_; @@ -271,11 +273,13 @@ class FakeVideoReceiveStream final } void GenerateKeyFrame() override {} - private: // webrtc::VideoReceiveStreamInterface implementation. void SetRtpExtensions(std::vector extensions) override; webrtc::RtpHeaderExtensionMap GetRtpExtensionMap() const override; bool transport_cc() const override { return config_.rtp.transport_cc; } + void SetTransportCc(bool transport_cc) override { + config_.rtp.transport_cc = transport_cc; + } void Start() override; void Stop() override; @@ -291,6 +295,7 @@ class FakeVideoReceiveStream final return base_mininum_playout_delay_ms_; } + private: webrtc::VideoReceiveStreamInterface::Config config_; bool receiving_; webrtc::VideoReceiveStreamInterface::Stats stats_; @@ -310,6 +315,9 @@ class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream { void SetRtpExtensions(std::vector extensions) override; webrtc::RtpHeaderExtensionMap GetRtpExtensionMap() const override; bool transport_cc() const override { return config_.rtp.transport_cc; } + void SetTransportCc(bool transport_cc) override { + config_.rtp.transport_cc = transport_cc; + } const webrtc::FlexfecReceiveStream::Config& GetConfig() const; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 5b8ddbcff6..52f5ca676d 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -3140,12 +3140,12 @@ TEST_F(WebRtcVideoChannelTest, RtcpIsCompoundByDefault) { TEST_F(WebRtcVideoChannelTest, TransportCcIsEnabledByDefault) { FakeVideoReceiveStream* stream = AddRecvStream(); - EXPECT_TRUE(stream->GetConfig().rtp.transport_cc); + EXPECT_TRUE(stream->transport_cc()); } TEST_F(WebRtcVideoChannelTest, TransportCcCanBeEnabledAndDisabled) { FakeVideoReceiveStream* stream = AddRecvStream(); - EXPECT_TRUE(stream->GetConfig().rtp.transport_cc); + EXPECT_TRUE(stream->transport_cc()); // Verify that transport cc feedback is turned off when send(!) codecs without // transport cc feedback are set. @@ -3154,14 +3154,14 @@ TEST_F(WebRtcVideoChannelTest, TransportCcCanBeEnabledAndDisabled) { EXPECT_TRUE(parameters.codecs[0].feedback_params.params().empty()); EXPECT_TRUE(channel_->SetSendParameters(parameters)); stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_FALSE(stream->GetConfig().rtp.transport_cc); + EXPECT_FALSE(stream->transport_cc()); // Verify that transport cc feedback is turned on when setting default codecs // since the default codecs have transport cc feedback enabled. parameters.codecs = engine_.send_codecs(); EXPECT_TRUE(channel_->SetSendParameters(parameters)); stream = fake_call_->GetVideoReceiveStreams()[0]; - EXPECT_TRUE(stream->GetConfig().rtp.transport_cc); + EXPECT_TRUE(stream->transport_cc()); } TEST_F(WebRtcVideoChannelTest, LossNotificationIsDisabledByDefault) { diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index 064b72dd6e..78ad7a16a1 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1180,8 +1180,8 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { void SetUseTransportCc(bool use_transport_cc, bool use_nack) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); - stream_->SetUseTransportCcAndNackHistory(use_transport_cc, - use_nack ? kNackRtpHistoryMs : 0); + stream_->SetTransportCc(use_transport_cc); + stream_->SetNackHistory(use_nack ? kNackRtpHistoryMs : 0); } void SetNonSenderRttMeasurement(bool enabled) { diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index cb6e2ac556..f61cfb9e49 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -1865,14 +1865,12 @@ TEST_P(WebRtcVoiceEngineTestFake, TransportCcCanBeEnabledAndDisabled) { EXPECT_TRUE(channel_->SetRecvParameters(recv_parameters)); EXPECT_TRUE(AddRecvStream(kSsrcX)); ASSERT_TRUE(call_.GetAudioReceiveStream(kSsrcX) != nullptr); - EXPECT_FALSE( - call_.GetAudioReceiveStream(kSsrcX)->GetConfig().rtp.transport_cc); + EXPECT_FALSE(call_.GetAudioReceiveStream(kSsrcX)->transport_cc()); send_parameters.codecs = engine_->send_codecs(); SetSendParameters(send_parameters); ASSERT_TRUE(call_.GetAudioReceiveStream(kSsrcX) != nullptr); - EXPECT_TRUE( - call_.GetAudioReceiveStream(kSsrcX)->GetConfig().rtp.transport_cc); + EXPECT_TRUE(call_.GetAudioReceiveStream(kSsrcX)->transport_cc()); } // Test that we can switch back and forth between Opus and ISAC with CN. diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 1bec357965..31552535f6 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -498,6 +498,17 @@ RtpHeaderExtensionMap VideoReceiveStream2::GetRtpExtensionMap() const { return rtp_video_stream_receiver_.GetRtpExtensions(); } +bool VideoReceiveStream2::transport_cc() const { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + return config_.rtp.transport_cc; +} + +void VideoReceiveStream2::SetTransportCc(bool transport_cc) { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + // TODO(tommi): Stop using the config struct for the internal state. + const_cast(config_.rtp.transport_cc) = transport_cc; +} + void VideoReceiveStream2::CreateAndRegisterExternalDecoder( const Decoder& decoder) { TRACE_EVENT0("webrtc", diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index 191ea37226..9add6a1d42 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -141,7 +141,8 @@ class VideoReceiveStream2 void SetRtpExtensions(std::vector extensions) override; RtpHeaderExtensionMap GetRtpExtensionMap() const override; - bool transport_cc() const override { return config_.rtp.transport_cc; } + bool transport_cc() const override; + void SetTransportCc(bool transport_cc) override; webrtc::VideoReceiveStreamInterface::Stats GetStats() const override;