Set voice RTCP mode based on the RemoteContent and not based on the LocalContent.

The RTCP mode is a send property for both send and receive channels. Send properties should be configured based on what peers support/prefer, which is described by the remote description (content).


Bug: webrtc:340041654
Change-Id: I18cd59e98aecfbbd8f4919b98381836184c10d77
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/368980
Reviewed-by: Jakob Ivarsson‎ <jakobi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Tomas Lundqvist <tomasl@google.com>
Cr-Commit-Position: refs/heads/main@{#43449}

media/base/fake_media_engine.h

@@ -498,9 +498,10 @@ class FakeVoiceMediaReceiveChannel
   void SetDefaultRawAudioSink(
       std::unique_ptr<webrtc::AudioSinkInterface> sink) override;
 
+  webrtc::RtcpMode RtcpMode() const override { return recv_rtcp_mode_; }
+  void SetRtcpMode(webrtc::RtcpMode mode) override { recv_rtcp_mode_ = mode; }
   std::vector<webrtc::RtpSource> 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<uint32_t, std::unique_ptr<VoiceChannelAudioSink>> local_sinks_;
   std::unique_ptr<webrtc::AudioSinkInterface> sink_;
   int max_bps_;
+  webrtc::RtcpMode recv_rtcp_mode_ = webrtc::RtcpMode::kCompound;
 };
 
 class FakeVoiceMediaSendChannel

media/base/media_channel.h

@@ -938,8 +938,9 @@ class VoiceMediaReceiveChannelInterface : public MediaReceiveChannelInterface {
   virtual void SetDefaultRawAudioSink(
       std::unique_ptr<webrtc::AudioSinkInterface> 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;
 };
 

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<std::string>& stream_ids,
     const std::vector<webrtc::RtpExtension>& /* 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_,

media/engine/webrtc_voice_engine.h

@@ -421,8 +421,9 @@ class WebRtcVoiceReceiveChannel final
       rtc::scoped_refptr<webrtc::FrameTransformerInterface> 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:

media/engine/webrtc_voice_engine_unittest.cc

@@ -306,14 +306,6 @@ class WebRtcVoiceEngineTestFake : public ::testing::TestWithParam<bool> {
              receive_channel_.get()](const std::set<uint32_t>& 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<bool> {
   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());

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",

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(

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();
 }

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