Enabling rtcp-rsize negotiation and fixing some issues with it.

Sending of reduced size RTCP packets should be enabled only if it's
enabled in the send parameters (which corresponds to the remote description).

Since the RTCPReceiver's RtcpMode isn't used at all, I removed it to ease
confusion.

BUG=webrtc:4868
R=pbos@webrtc.org, pthatcher@google.com, pthatcher@webrtc.org, stefan@webrtc.org

Review URL: https://codereview.webrtc.org/1713493003 .

Cr-Commit-Position: refs/heads/master@{#12057}

webrtc/media/base/mediachannel.h

@@ -837,6 +837,8 @@ struct RtpParameters {
   RtcpParameters rtcp;
 };
 
+// TODO(deadbeef): Rename to RtpSenderParameters, since they're intended to
+// encapsulate all the parameters needed for an RtpSender.
 template <class Codec>
 struct RtpSendParameters : RtpParameters<Codec> {
   std::string ToString() const override {
@@ -934,6 +936,8 @@ class VoiceMediaChannel : public MediaChannel {
       std::unique_ptr<webrtc::AudioSinkInterface> sink) = 0;
 };
 
+// TODO(deadbeef): Rename to VideoSenderParameters, since they're intended to
+// encapsulate all the parameters needed for a video RtpSender.
 struct VideoSendParameters : RtpSendParameters<VideoCodec> {
   // Use conference mode? This flag comes from the remote
   // description's SDP line 'a=x-google-flag:conference', copied over
@@ -944,6 +948,8 @@ struct VideoSendParameters : RtpSendParameters<VideoCodec> {
   bool conference_mode = false;
 };
 
+// TODO(deadbeef): Rename to VideoReceiverParameters, since they're intended to
+// encapsulate all the parameters needed for a video RtpReceiver.
 struct VideoRecvParameters : RtpParameters<VideoCodec> {
 };
 

webrtc/media/engine/webrtcvideoengine2.cc

@@ -802,16 +802,18 @@ bool WebRtcVideoChannel2::SetSendParameters(const VideoSendParameters& params) {
     for (auto& kv : send_streams_) {
       kv.second->SetSendParameters(changed_params);
     }
-    if (changed_params.codec) {
+    if (changed_params.codec || changed_params.rtcp_mode) {
-      // Update receive feedback parameters from new codec.
+      // Update receive feedback parameters from new codec or RTCP mode.
       LOG(LS_INFO)
           << "SetFeedbackOptions on all the receive streams because the send "
-             "codec has changed.";
+             "codec or RTCP mode has changed.";
       for (auto& kv : receive_streams_) {
         RTC_DCHECK(kv.second != nullptr);
-        kv.second->SetFeedbackParameters(HasNack(send_codec_->codec),
+        kv.second->SetFeedbackParameters(
-                                         HasRemb(send_codec_->codec),
+            HasNack(send_codec_->codec), HasRemb(send_codec_->codec),
-                                         HasTransportCc(send_codec_->codec));
+            HasTransportCc(send_codec_->codec),
+            params.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize
+                                     : webrtc::RtcpMode::kCompound);
       }
     }
   }
@@ -882,13 +884,6 @@ bool WebRtcVideoChannel2::GetChangedRecvParameters(
         rtc::Optional<std::vector<webrtc::RtpExtension>>(filtered_extensions);
   }
 
-  // Handle RTCP mode.
-  if (params.rtcp.reduced_size != recv_params_.rtcp.reduced_size) {
-    changed_params->rtcp_mode = rtc::Optional<webrtc::RtcpMode>(
-        params.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize
-                                 : webrtc::RtcpMode::kCompound);
-  }
-
   return true;
 }
 
@@ -1159,7 +1154,12 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp(
   config->rtp.local_ssrc = rtcp_receiver_report_ssrc_;
 
   config->rtp.extensions = recv_rtp_extensions_;
-  config->rtp.rtcp_mode = recv_params_.rtcp.reduced_size
+  // Whether or not the receive stream sends reduced size RTCP is determined
+  // by the send params.
+  // TODO(deadbeef): Once we change "send_params" to "sender_params" and
+  // "recv_params" to "receiver_params", we should get this out of
+  // receiver_params_.
+  config->rtp.rtcp_mode = send_params_.rtcp.reduced_size
                               ? webrtc::RtcpMode::kReducedSize
                               : webrtc::RtcpMode::kCompound;
 
@@ -2294,11 +2294,13 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetLocalSsrc(
 void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters(
     bool nack_enabled,
     bool remb_enabled,
-    bool transport_cc_enabled) {
+    bool transport_cc_enabled,
+    webrtc::RtcpMode rtcp_mode) {
   int nack_history_ms = nack_enabled ? kNackHistoryMs : 0;
   if (config_.rtp.nack.rtp_history_ms == nack_history_ms &&
       config_.rtp.remb == remb_enabled &&
-      config_.rtp.transport_cc == transport_cc_enabled) {
+      config_.rtp.transport_cc == transport_cc_enabled &&
+      config_.rtp.rtcp_mode == rtcp_mode) {
     LOG(LS_INFO)
         << "Ignoring call to SetFeedbackParameters because parameters are "
            "unchanged; nack="
@@ -2309,6 +2311,7 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFeedbackParameters(
   config_.rtp.remb = remb_enabled;
   config_.rtp.nack.rtp_history_ms = nack_history_ms;
   config_.rtp.transport_cc = transport_cc_enabled;
+  config_.rtp.rtcp_mode = rtcp_mode;
   LOG(LS_INFO)
       << "RecreateWebRtcStream (recv) because of SetFeedbackParameters; nack="
       << nack_enabled << ", remb=" << remb_enabled
@@ -2328,10 +2331,6 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvParameters(
     config_.rtp.extensions = *params.rtp_header_extensions;
     needs_recreation = true;
   }
-  if (params.rtcp_mode) {
-    config_.rtp.rtcp_mode = *params.rtcp_mode;
-    needs_recreation = true;
-  }
   if (needs_recreation) {
     LOG(LS_INFO) << "RecreateWebRtcStream (recv) because of SetRecvParameters";
     RecreateWebRtcStream();

webrtc/media/engine/webrtcvideoengine2.h

@@ -200,7 +200,6 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
     // These optionals are unset if not changed.
     rtc::Optional<std::vector<VideoCodecSettings>> codec_settings;
     rtc::Optional<std::vector<webrtc::RtpExtension>> rtp_header_extensions;
-    rtc::Optional<webrtc::RtcpMode> rtcp_mode;
   };
 
   bool GetChangedSendParameters(const VideoSendParameters& params,
@@ -411,9 +410,11 @@ class WebRtcVideoChannel2 : public VideoMediaChannel, public webrtc::Transport {
     const std::vector<uint32_t>& GetSsrcs() const;
 
     void SetLocalSsrc(uint32_t local_ssrc);
+    // TODO(deadbeef): Move these feedback parameters into the recv parameters.
     void SetFeedbackParameters(bool nack_enabled,
                                bool remb_enabled,
-                               bool transport_cc_enabled);
+                               bool transport_cc_enabled,
+                               webrtc::RtcpMode rtcp_mode);
     void SetRecvParameters(const ChangedRecvParameters& recv_params);
 
     void RenderFrame(const webrtc::VideoFrame& frame,

webrtc/media/engine/webrtcvideoengine2_unittest.cc

@@ -2575,8 +2575,10 @@ TEST_F(WebRtcVideoChannel2Test, TestSetRecvRtcpReducedSize) {
   EXPECT_EQ(webrtc::RtcpMode::kCompound, stream1->GetConfig().rtp.rtcp_mode);
 
   // Now enable reduced size mode.
-  recv_parameters_.rtcp.reduced_size = true;
+  // TODO(deadbeef): Once "recv_parameters" becomes "receiver_parameters",
-  EXPECT_TRUE(channel_->SetRecvParameters(recv_parameters_));
+  // the reduced_size flag should come from that.
+  send_parameters_.rtcp.reduced_size = true;
+  EXPECT_TRUE(channel_->SetSendParameters(send_parameters_));
   stream1 = fake_call_->GetVideoReceiveStreams()[0];
   EXPECT_EQ(webrtc::RtcpMode::kReducedSize, stream1->GetConfig().rtp.rtcp_mode);
 

webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc

@@ -47,7 +47,6 @@ RTCPReceiver::RTCPReceiver(
     : TMMBRHelp(),
       _clock(clock),
       receiver_only_(receiver_only),
-      _method(RtcpMode::kOff),
       _lastReceived(0),
       _rtpRtcp(*owner),
       _criticalSectionFeedbacks(
@@ -103,16 +102,6 @@ RTCPReceiver::~RTCPReceiver() {
   }
 }
 
-RtcpMode RTCPReceiver::Status() const {
-  CriticalSectionScoped lock(_criticalSectionRTCPReceiver);
-  return _method;
-}
-
-void RTCPReceiver::SetRTCPStatus(RtcpMode method) {
-  CriticalSectionScoped lock(_criticalSectionRTCPReceiver);
-  _method = method;
-}
-
 int64_t RTCPReceiver::LastReceived() {
   CriticalSectionScoped lock(_criticalSectionRTCPReceiver);
   return _lastReceived;

webrtc/modules/rtp_rtcp/source/rtcp_receiver.h

@@ -38,9 +38,6 @@ public:
               ModuleRtpRtcpImpl* owner);
     virtual ~RTCPReceiver();
 
-    RtcpMode Status() const;
-    void SetRTCPStatus(RtcpMode method);
-
     int64_t LastReceived();
     int64_t LastReceivedReceiverReport() const;
 
@@ -267,7 +264,6 @@ protected:
 
   Clock* const _clock;
   const bool receiver_only_;
-  RtcpMode _method;
   int64_t _lastReceived;
   ModuleRtpRtcpImpl& _rtpRtcp;
 

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -497,16 +497,12 @@ int32_t ModuleRtpRtcpImpl::SetMaxTransferUnit(const uint16_t mtu) {
 }
 
 RtcpMode ModuleRtpRtcpImpl::RTCP() const {
-  if (rtcp_sender_.Status() != RtcpMode::kOff) {
+  return rtcp_sender_.Status();
-    return rtcp_receiver_.Status();
-  }
-  return RtcpMode::kOff;
 }
 
 // Configure RTCP status i.e on/off.
 void ModuleRtpRtcpImpl::SetRTCPStatus(const RtcpMode method) {
   rtcp_sender_.SetRTCPStatus(method);
-  rtcp_receiver_.SetRTCPStatus(method);
 }
 
 int32_t ModuleRtpRtcpImpl::SetCNAME(const char* c_name) {

webrtc/pc/mediasession.cc

@@ -755,11 +755,9 @@ static bool CreateMediaContentOffer(
     offer->set_crypto_required(CT_SDES);
   }
   offer->set_rtcp_mux(options.rtcp_mux_enabled);
-  // TODO(deadbeef): Once we're sure this works correctly, enable it in
+  if (offer->type() == cricket::MEDIA_TYPE_VIDEO) {
-  // CreateOffer.
+    offer->set_rtcp_reduced_size(true);
-  // if (offer->type() == cricket::MEDIA_TYPE_VIDEO) {
+  }
-  //   offer->set_rtcp_reduced_size(true);
-  // }
   offer->set_multistream(options.is_muc);
   offer->set_rtp_header_extensions(rtp_extensions);
 
@@ -1053,11 +1051,9 @@ static bool CreateMediaContentAnswer(
   answer->set_rtp_header_extensions(negotiated_rtp_extensions);
 
   answer->set_rtcp_mux(options.rtcp_mux_enabled && offer->rtcp_mux());
-  // TODO(deadbeef): Once we're sure this works correctly, enable it in
+  if (answer->type() == cricket::MEDIA_TYPE_VIDEO) {
-  // CreateAnswer.
+    answer->set_rtcp_reduced_size(offer->rtcp_reduced_size());
-  // if (answer->type() == cricket::MEDIA_TYPE_VIDEO) {
+  }
-  //   answer->set_rtcp_reduced_size(offer->rtcp_reduced_size());
-  // }
 
   if (sdes_policy != SEC_DISABLED) {
     CryptoParams crypto;

webrtc/video/video_send_stream.cc

@@ -69,6 +69,9 @@ std::string VideoSendStream::Config::Rtp::ToString() const {
       ss << ", ";
   }
   ss << ']';
+  ss << ", rtcp_mode: "
+     << (rtcp_mode == RtcpMode::kCompound ? "RtcpMode::kCompound"
+                                          : "RtcpMode::kReducedSize");
   ss << ", max_packet_size: " << max_packet_size;
   ss << ", extensions: [";
   for (size_t i = 0; i < extensions.size(); ++i) {