diff --git a/media/base/fake_media_engine.h b/media/base/fake_media_engine.h index 2c7cc8b853..44c046ba38 100644 --- a/media/base/fake_media_engine.h +++ b/media/base/fake_media_engine.h @@ -498,9 +498,10 @@ class FakeVoiceMediaReceiveChannel void SetDefaultRawAudioSink( std::unique_ptr sink) override; + webrtc::RtcpMode RtcpMode() const override { return recv_rtcp_mode_; } + void SetRtcpMode(webrtc::RtcpMode mode) override { recv_rtcp_mode_ = mode; } std::vector GetSources(uint32_t ssrc) const override; void SetReceiveNackEnabled(bool /* enabled */) override {} - void SetRtcpMode(webrtc::RtcpMode /* mode */) override {} void SetReceiveNonSenderRttEnabled(bool /* enabled */) override {} private: @@ -534,6 +535,7 @@ class FakeVoiceMediaReceiveChannel std::map> local_sinks_; std::unique_ptr sink_; int max_bps_; + webrtc::RtcpMode recv_rtcp_mode_ = webrtc::RtcpMode::kCompound; }; class FakeVoiceMediaSendChannel diff --git a/media/base/media_channel.h b/media/base/media_channel.h index ac7eaab207..55ec4ff2e9 100644 --- a/media/base/media_channel.h +++ b/media/base/media_channel.h @@ -938,8 +938,9 @@ class VoiceMediaReceiveChannelInterface : public MediaReceiveChannelInterface { virtual void SetDefaultRawAudioSink( std::unique_ptr sink) = 0; virtual bool GetStats(VoiceMediaReceiveInfo* stats, bool reset_legacy) = 0; - virtual void SetReceiveNackEnabled(bool enabled) = 0; + virtual webrtc::RtcpMode RtcpMode() const = 0; virtual void SetRtcpMode(webrtc::RtcpMode mode) = 0; + virtual void SetReceiveNackEnabled(bool enabled) = 0; virtual void SetReceiveNonSenderRttEnabled(bool enabled) = 0; }; diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index e2a9cd2490..f63a887fad 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -287,6 +287,7 @@ webrtc::AudioReceiveStreamInterface::Config BuildReceiveStreamConfig( uint32_t local_ssrc, bool use_nack, bool enable_non_sender_rtt, + webrtc::RtcpMode rtcp_mode, const std::vector& stream_ids, const std::vector& /* extensions */, webrtc::Transport* rtcp_send_transport, @@ -303,7 +304,7 @@ webrtc::AudioReceiveStreamInterface::Config BuildReceiveStreamConfig( config.rtp.remote_ssrc = remote_ssrc; config.rtp.local_ssrc = local_ssrc; config.rtp.nack.rtp_history_ms = use_nack ? kNackRtpHistoryMs : 0; - config.rtp.rtcp_mode = webrtc::RtcpMode::kCompound; + config.rtp.rtcp_mode = rtcp_mode; if (!stream_ids.empty()) { config.sync_group = stream_ids[0]; } @@ -2133,8 +2134,10 @@ bool WebRtcVoiceReceiveChannel::SetReceiverParameters( recv_rtp_extension_map_ = webrtc::RtpHeaderExtensionMap(recv_rtp_extensions_); } - SetRtcpMode(params.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize - : webrtc::RtcpMode::kCompound); + // RTCP mode, NACK, and receive-side RTT are not configured here because they + // enable send functionality in the receive channels. This functionality is + // instead configured using the SetReceiveRtcpMode, SetReceiveNackEnabled, and + // SetReceiveNonSenderRttEnabled methods. return true; } @@ -2281,18 +2284,6 @@ bool WebRtcVoiceReceiveChannel::SetRecvCodecs( return true; } -void WebRtcVoiceReceiveChannel::SetReceiveNackEnabled(bool enabled) { - // Check if the NACK status has changed on the - // preferred send codec, and in that case reconfigure all receive streams. - if (recv_nack_enabled_ != enabled) { - RTC_LOG(LS_INFO) << "Changing NACK status on receive streams."; - recv_nack_enabled_ = enabled; - for (auto& kv : recv_streams_) { - kv.second->SetUseNack(recv_nack_enabled_); - } - } -} - void WebRtcVoiceReceiveChannel::SetRtcpMode(webrtc::RtcpMode mode) { // Check if the reduced size RTCP status changed on the // preferred send codec, and in that case reconfigure all receive streams. @@ -2305,6 +2296,18 @@ void WebRtcVoiceReceiveChannel::SetRtcpMode(webrtc::RtcpMode mode) { } } +void WebRtcVoiceReceiveChannel::SetReceiveNackEnabled(bool enabled) { + // Check if the NACK status has changed on the + // preferred send codec, and in that case reconfigure all receive streams. + if (recv_nack_enabled_ != enabled) { + RTC_LOG(LS_INFO) << "Changing NACK status on receive streams."; + recv_nack_enabled_ = enabled; + for (auto& kv : recv_streams_) { + kv.second->SetUseNack(recv_nack_enabled_); + } + } +} + void WebRtcVoiceReceiveChannel::SetReceiveNonSenderRttEnabled(bool enabled) { // Check if the receive-side RTT status has changed on the preferred send // codec, in that case reconfigure all receive streams. @@ -2366,7 +2369,7 @@ bool WebRtcVoiceReceiveChannel::AddRecvStream(const StreamParams& sp) { // Create a new channel for receiving audio data. auto config = BuildReceiveStreamConfig( ssrc, receiver_reports_ssrc_, recv_nack_enabled_, enable_non_sender_rtt_, - sp.stream_ids(), recv_rtp_extensions_, transport(), + recv_rtcp_mode_, sp.stream_ids(), recv_rtp_extensions_, transport(), engine()->decoder_factory_, decoder_map_, codec_pair_id_, engine()->audio_jitter_buffer_max_packets_, engine()->audio_jitter_buffer_fast_accelerate_, diff --git a/media/engine/webrtc_voice_engine.h b/media/engine/webrtc_voice_engine.h index 0fa1728a10..cb5e22100f 100644 --- a/media/engine/webrtc_voice_engine.h +++ b/media/engine/webrtc_voice_engine.h @@ -421,8 +421,9 @@ class WebRtcVoiceReceiveChannel final rtc::scoped_refptr frame_transformer) override; - void SetReceiveNackEnabled(bool enabled) override; + webrtc::RtcpMode RtcpMode() const override { return recv_rtcp_mode_; } void SetRtcpMode(webrtc::RtcpMode mode) override; + void SetReceiveNackEnabled(bool enabled) override; void SetReceiveNonSenderRttEnabled(bool enabled) override; private: diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc index 1e6f03e284..8a208d7b88 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -306,14 +306,6 @@ class WebRtcVoiceEngineTestFake : public ::testing::TestWithParam { receive_channel_.get()](const std::set& choices) { receive_channel->ChooseReceiverReportSsrc(choices); }); - send_channel_->SetSendCodecChangedCallback( - [receive_channel = receive_channel_.get(), - send_channel = send_channel_.get()]() { - receive_channel->SetReceiveNackEnabled( - send_channel->SendCodecHasNack()); - receive_channel->SetReceiveNonSenderRttEnabled( - send_channel->SenderNonSenderRttEnabled()); - }); return true; } @@ -403,6 +395,15 @@ class WebRtcVoiceEngineTestFake : public ::testing::TestWithParam { void SetSenderParameters(const cricket::AudioSenderParameter& params) { ASSERT_TRUE(send_channel_); EXPECT_TRUE(send_channel_->SetSenderParameters(params)); + if (receive_channel_) { + receive_channel_->SetRtcpMode(params.rtcp.reduced_size + ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound); + receive_channel_->SetReceiveNackEnabled( + send_channel_->SendCodecHasNack()); + receive_channel_->SetReceiveNonSenderRttEnabled( + send_channel_->SenderNonSenderRttEnabled()); + } } void SetAudioSend(uint32_t ssrc, @@ -2029,6 +2030,73 @@ TEST_P(WebRtcVoiceEngineTestFake, AddRecvStreamEnableNack) { EXPECT_EQ(kRtpHistoryMs, GetRecvStreamConfig(kSsrcZ).rtp.nack.rtp_history_ms); } +// Test that we can enable RTCP reduced size mode with opus as callee. +TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecEnableRtcpReducedSizeAsCallee) { + EXPECT_TRUE(SetupRecvStream()); + cricket::AudioSenderParameter parameters; + parameters.codecs.push_back(kOpusCodec); + parameters.rtcp.reduced_size = true; + EXPECT_EQ(webrtc::RtcpMode::kCompound, + GetRecvStreamConfig(kSsrcX).rtp.rtcp_mode); + SetSenderParameters(parameters); + // Reduced size mode should be enabled even with no send stream. + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, + GetRecvStreamConfig(kSsrcX).rtp.rtcp_mode); + + EXPECT_TRUE(send_channel_->AddSendStream( + cricket::StreamParams::CreateLegacy(kSsrcX))); +} + +// Test that we can enable RTCP reduced size mode on receive streams. +TEST_P(WebRtcVoiceEngineTestFake, + SetSendCodecEnableRtcpReducedSizeRecvStreams) { + EXPECT_TRUE(SetupSendStream()); + EXPECT_TRUE(AddRecvStream(kSsrcY)); + cricket::AudioSenderParameter parameters; + parameters.codecs.push_back(kOpusCodec); + parameters.rtcp.reduced_size = true; + EXPECT_EQ(webrtc::RtcpMode::kCompound, + GetRecvStreamConfig(kSsrcY).rtp.rtcp_mode); + SetSenderParameters(parameters); + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, + GetRecvStreamConfig(kSsrcY).rtp.rtcp_mode); +} + +// Test that we can disable RTCP reduced size mode on receive streams. +TEST_P(WebRtcVoiceEngineTestFake, + SetSendCodecDisableRtcpReducedSizeRecvStreams) { + EXPECT_TRUE(SetupSendStream()); + EXPECT_TRUE(AddRecvStream(kSsrcY)); + cricket::AudioSenderParameter parameters; + parameters.codecs.push_back(kOpusCodec); + parameters.rtcp.reduced_size = true; + SetSenderParameters(parameters); + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, + GetRecvStreamConfig(kSsrcY).rtp.rtcp_mode); + + parameters.rtcp.reduced_size = false; + SetSenderParameters(parameters); + EXPECT_EQ(webrtc::RtcpMode::kCompound, + GetRecvStreamConfig(kSsrcY).rtp.rtcp_mode); +} + +// Test that RTCP reduced size mode is enabled on a new receive stream. +TEST_P(WebRtcVoiceEngineTestFake, AddRecvStreamEnableRtcpReducedSize) { + EXPECT_TRUE(SetupSendStream()); + cricket::AudioSenderParameter parameters; + parameters.codecs.push_back(kOpusCodec); + parameters.codecs.push_back(kCn16000Codec); + parameters.rtcp.reduced_size = true; + SetSenderParameters(parameters); + + EXPECT_TRUE(AddRecvStream(kSsrcY)); + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, + GetRecvStreamConfig(kSsrcY).rtp.rtcp_mode); + EXPECT_TRUE(AddRecvStream(kSsrcZ)); + EXPECT_EQ(webrtc::RtcpMode::kReducedSize, + GetRecvStreamConfig(kSsrcZ).rtp.rtcp_mode); +} + // Test that we can switch back and forth between Opus and PCMU with CN. TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsOpusPcmuSwitching) { EXPECT_TRUE(SetupSendStream()); diff --git a/pc/BUILD.gn b/pc/BUILD.gn index c7831a16ad..3dbc6b6583 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -75,6 +75,7 @@ rtc_source_set("channel") { ":rtp_transport_internal", ":session_description", "../api:libjingle_peerconnection_api", + "../api:rtp_headers", "../api:rtp_parameters", "../api:rtp_transceiver_direction", "../api:scoped_refptr", diff --git a/pc/channel.cc b/pc/channel.cc index 9156ad1d61..4252730171 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -24,6 +24,7 @@ #include "api/crypto/crypto_options.h" #include "api/jsep.h" #include "api/media_types.h" +#include "api/rtp_headers.h" #include "api/rtp_parameters.h" #include "api/sequence_checker.h" #include "api/task_queue/pending_task_safety_flag.h" @@ -971,6 +972,12 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content, mid().c_str()); return false; } + // The receive channel can send RTCP packets in the reverse direction. It + // should use the reduced size mode if a peer has requested it through the + // remote content. + media_receive_channel()->SetRtcpMode(content->rtcp_reduced_size() + ? webrtc::RtcpMode::kReducedSize + : webrtc::RtcpMode::kCompound); // Update Receive channel based on Send channel's codec information. // TODO(bugs.webrtc.org/14911): This is silly. Stop doing it. media_receive_channel()->SetReceiveNackEnabled( diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index e4ab8fd928..2a5399a803 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -19,6 +19,7 @@ #include "absl/functional/any_invocable.h" #include "api/array_view.h" #include "api/audio_options.h" +#include "api/rtp_headers.h" #include "api/rtp_parameters.h" #include "api/task_queue/pending_task_safety_flag.h" #include "media/base/codec.h" @@ -690,6 +691,37 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { EXPECT_TRUE(channel2_->SetRemoteContent(&content, SdpType::kAnswer, err)); } + // Test that SetLocalContent and SetRemoteContent properly set RTCP + // reduced_size. + void TestSetContentsRtcpReducedSize() { + CreateChannels(0, 0); + typename T::Content content; + CreateContent(0, kPcmuCodec, kH264Codec, &content); + // Both sides agree on reduced size. + content.set_rtcp_reduced_size(true); + std::string err; + // The RTCP mode is a send property and should be configured based on + // the remote content and not the local content. + EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, err)); + EXPECT_EQ(media_receive_channel1_impl()->RtcpMode(), + webrtc::RtcpMode::kCompound); + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, err)); + EXPECT_EQ(media_receive_channel1_impl()->RtcpMode(), + webrtc::RtcpMode::kReducedSize); + // Only initiator supports reduced size. + EXPECT_TRUE(channel2_->SetLocalContent(&content, SdpType::kOffer, err)); + EXPECT_EQ(media_receive_channel2_impl()->RtcpMode(), + webrtc::RtcpMode::kCompound); + content.set_rtcp_reduced_size(false); + EXPECT_TRUE(channel2_->SetRemoteContent(&content, SdpType::kAnswer, err)); + EXPECT_EQ(media_receive_channel2_impl()->RtcpMode(), + webrtc::RtcpMode::kCompound); + // Peer renegotiates without reduced size. + EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, err)); + EXPECT_EQ(media_receive_channel1_impl()->RtcpMode(), + webrtc::RtcpMode::kCompound); + } + // Test that SetLocalContent and SetRemoteContent properly // handles adding and removing StreamParams when the action is a full // SdpType::kOffer / SdpType::kAnswer. @@ -1729,6 +1761,10 @@ TEST_F(VoiceChannelSingleThreadTest, TestSetContentsRtcpMuxWithPrAnswer) { Base::TestSetContentsRtcpMux(); } +TEST_F(VoiceChannelSingleThreadTest, TestSetContentsRtcpReducedSize) { + Base::TestSetContentsRtcpReducedSize(); +} + TEST_F(VoiceChannelSingleThreadTest, TestChangeStreamParamsInContent) { Base::TestChangeStreamParamsInContent(); } @@ -1866,6 +1902,10 @@ TEST_F(VoiceChannelDoubleThreadTest, TestSetContentsRtcpMuxWithPrAnswer) { Base::TestSetContentsRtcpMux(); } +TEST_F(VoiceChannelDoubleThreadTest, TestSetContentsRtcpReducedSize) { + Base::TestSetContentsRtcpReducedSize(); +} + TEST_F(VoiceChannelDoubleThreadTest, TestChangeStreamParamsInContent) { Base::TestChangeStreamParamsInContent(); } diff --git a/pc/test/mock_voice_media_receive_channel_interface.h b/pc/test/mock_voice_media_receive_channel_interface.h index a86420f520..57c759f47e 100644 --- a/pc/test/mock_voice_media_receive_channel_interface.h +++ b/pc/test/mock_voice_media_receive_channel_interface.h @@ -67,8 +67,9 @@ class MockVoiceMediaReceiveChannelInterface GetStats, (VoiceMediaReceiveInfo * stats, bool reset_legacy), (override)); - MOCK_METHOD(void, SetReceiveNackEnabled, (bool enabled), (override)); + MOCK_METHOD(webrtc::RtcpMode, RtcpMode, (), (const, override)); MOCK_METHOD(void, SetRtcpMode, (webrtc::RtcpMode mode), (override)); + MOCK_METHOD(void, SetReceiveNackEnabled, (bool enabled), (override)); MOCK_METHOD(void, SetReceiveNonSenderRttEnabled, (bool enabled), (override)); // MediaReceiveChannelInterface