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}
diff --git a/webrtc/media/base/mediachannel.h b/webrtc/media/base/mediachannel.h
index f92a732..ed565ee 100644
--- a/webrtc/media/base/mediachannel.h
+++ b/webrtc/media/base/mediachannel.h
@@ -837,6 +837,8 @@
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 @@
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 @@
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> {
};
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index e34056d..83f81f0 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -802,16 +802,18 @@
for (auto& kv : send_streams_) {
kv.second->SetSendParameters(changed_params);
}
- if (changed_params.codec) {
- // Update receive feedback parameters from new codec.
+ if (changed_params.codec || changed_params.rtcp_mode) {
+ // 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),
- HasRemb(send_codec_->codec),
- HasTransportCc(send_codec_->codec));
+ kv.second->SetFeedbackParameters(
+ HasNack(send_codec_->codec), HasRemb(send_codec_->codec),
+ HasTransportCc(send_codec_->codec),
+ params.rtcp.reduced_size ? webrtc::RtcpMode::kReducedSize
+ : webrtc::RtcpMode::kCompound);
}
}
}
@@ -882,13 +884,6 @@
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 @@
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::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 @@
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 @@
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();
diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h
index 850bcae..7ffd853 100644
--- a/webrtc/media/engine/webrtcvideoengine2.h
+++ b/webrtc/media/engine/webrtcvideoengine2.h
@@ -200,7 +200,6 @@
// 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 @@
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,
diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
index b44eb99..64c691f 100644
--- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc
+++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
@@ -2575,8 +2575,10 @@
EXPECT_EQ(webrtc::RtcpMode::kCompound, stream1->GetConfig().rtp.rtcp_mode);
// Now enable reduced size mode.
- recv_parameters_.rtcp.reduced_size = true;
- EXPECT_TRUE(channel_->SetRecvParameters(recv_parameters_));
+ // TODO(deadbeef): Once "recv_parameters" becomes "receiver_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);
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc
index 0873254..1bfa7cd 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc
@@ -47,7 +47,6 @@
: TMMBRHelp(),
_clock(clock),
receiver_only_(receiver_only),
- _method(RtcpMode::kOff),
_lastReceived(0),
_rtpRtcp(*owner),
_criticalSectionFeedbacks(
@@ -103,16 +102,6 @@
}
}
-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;
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h
index 1930670..475ab1e 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_receiver.h
@@ -38,9 +38,6 @@
ModuleRtpRtcpImpl* owner);
virtual ~RTCPReceiver();
- RtcpMode Status() const;
- void SetRTCPStatus(RtcpMode method);
-
int64_t LastReceived();
int64_t LastReceivedReceiverReport() const;
@@ -267,7 +264,6 @@
Clock* const _clock;
const bool receiver_only_;
- RtcpMode _method;
int64_t _lastReceived;
ModuleRtpRtcpImpl& _rtpRtcp;
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index ef720be..581aa51 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -497,16 +497,12 @@
}
RtcpMode ModuleRtpRtcpImpl::RTCP() const {
- if (rtcp_sender_.Status() != RtcpMode::kOff) {
- return rtcp_receiver_.Status();
- }
- return RtcpMode::kOff;
+ return rtcp_sender_.Status();
}
// 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) {
diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc
index 6a98f7d..a9d1b95 100644
--- a/webrtc/pc/mediasession.cc
+++ b/webrtc/pc/mediasession.cc
@@ -755,11 +755,9 @@
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
- // CreateOffer.
- // if (offer->type() == cricket::MEDIA_TYPE_VIDEO) {
- // 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 @@
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
- // CreateAnswer.
- // if (answer->type() == cricket::MEDIA_TYPE_VIDEO) {
- // 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;
diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc
index 21a22dc..6682396 100644
--- a/webrtc/video/video_send_stream.cc
+++ b/webrtc/video/video_send_stream.cc
@@ -69,6 +69,9 @@
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) {
