Potential fix for rtx/red issue where red is removed only from the remote description.
BUG=5675
R=pbos@webrtc.org
Review URL: https://codereview.webrtc.org/1964473002 .
Cr-Commit-Position: refs/heads/master@{#12776}
diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc
index d4360b8..d81c849 100644
--- a/webrtc/media/engine/webrtcvideoengine2.cc
+++ b/webrtc/media/engine/webrtcvideoengine2.cc
@@ -263,39 +263,6 @@
return false;
}
-// Merges two fec configs and logs an error if a conflict arises
-// such that merging in different order would trigger a different output.
-static void MergeFecConfig(const webrtc::FecConfig& other,
- webrtc::FecConfig* output) {
- if (other.ulpfec_payload_type != -1) {
- if (output->ulpfec_payload_type != -1 &&
- output->ulpfec_payload_type != other.ulpfec_payload_type) {
- LOG(LS_WARNING) << "Conflict merging ulpfec_payload_type configs: "
- << output->ulpfec_payload_type << " and "
- << other.ulpfec_payload_type;
- }
- output->ulpfec_payload_type = other.ulpfec_payload_type;
- }
- if (other.red_payload_type != -1) {
- if (output->red_payload_type != -1 &&
- output->red_payload_type != other.red_payload_type) {
- LOG(LS_WARNING) << "Conflict merging red_payload_type configs: "
- << output->red_payload_type << " and "
- << other.red_payload_type;
- }
- output->red_payload_type = other.red_payload_type;
- }
- if (other.red_rtx_payload_type != -1) {
- if (output->red_rtx_payload_type != -1 &&
- output->red_rtx_payload_type != other.red_rtx_payload_type) {
- LOG(LS_WARNING) << "Conflict merging red_rtx_payload_type configs: "
- << output->red_rtx_payload_type << " and "
- << other.red_rtx_payload_type;
- }
- output->red_rtx_payload_type = other.red_rtx_payload_type;
- }
-}
-
// Returns true if the given codec is disallowed from doing simulcast.
bool IsCodecBlacklistedForSimulcast(const std::string& codec_name) {
return CodecNamesEq(codec_name, kH264CodecName) ||
@@ -682,7 +649,8 @@
video_config_(config.video),
external_encoder_factory_(external_encoder_factory),
external_decoder_factory_(external_decoder_factory),
- default_send_options_(options) {
+ default_send_options_(options),
+ red_disabled_by_remote_side_(false) {
RTC_DCHECK(thread_checker_.CalledOnValidThread());
rtcp_receiver_report_ssrc_ = kDefaultRtcpReceiverReportSsrc;
@@ -872,6 +840,19 @@
: webrtc::RtcpMode::kCompound);
}
}
+ if (changed_params.codec) {
+ bool red_was_disabled = red_disabled_by_remote_side_;
+ red_disabled_by_remote_side_ =
+ changed_params.codec->fec.red_payload_type == -1;
+ if (red_was_disabled != red_disabled_by_remote_side_) {
+ for (auto& kv : receive_streams_) {
+ // In practice VideoChannel::SetRemoteContent appears to most of the
+ // time also call UpdateRemoteStreams, which recreates the receive
+ // streams. If that's always true this call isn't needed.
+ kv.second->SetFecDisabledRemotely(red_disabled_by_remote_side_);
+ }
+ }
+ }
}
send_params_ = params;
return true;
@@ -1237,7 +1218,7 @@
receive_streams_[ssrc] = new WebRtcVideoReceiveStream(
call_, sp, config, external_decoder_factory_, default_stream,
- recv_codecs_);
+ recv_codecs_, red_disabled_by_remote_side_);
return true;
}
@@ -1273,10 +1254,6 @@
}
for (size_t i = 0; i < recv_codecs_.size(); ++i) {
- MergeFecConfig(recv_codecs_[i].fec, &config->rtp.fec);
- }
-
- for (size_t i = 0; i < recv_codecs_.size(); ++i) {
uint32_t rtx_ssrc;
if (recv_codecs_[i].rtx_payload_type != -1 &&
sp.GetFidSsrc(ssrc, &rtx_ssrc)) {
@@ -2239,13 +2216,15 @@
const webrtc::VideoReceiveStream::Config& config,
WebRtcVideoDecoderFactory* external_decoder_factory,
bool default_stream,
- const std::vector<VideoCodecSettings>& recv_codecs)
+ const std::vector<VideoCodecSettings>& recv_codecs,
+ bool red_disabled_by_remote_side)
: call_(call),
ssrcs_(sp.ssrcs),
ssrc_groups_(sp.ssrc_groups),
stream_(NULL),
default_stream_(default_stream),
config_(config),
+ red_disabled_by_remote_side_(red_disabled_by_remote_side),
external_decoder_factory_(external_decoder_factory),
sink_(NULL),
last_width_(-1),
@@ -2421,7 +2400,13 @@
if (stream_ != NULL) {
call_->DestroyVideoReceiveStream(stream_);
}
- stream_ = call_->CreateVideoReceiveStream(config_);
+ webrtc::VideoReceiveStream::Config config = config_;
+ if (red_disabled_by_remote_side_) {
+ config.rtp.fec.red_payload_type = -1;
+ config.rtp.fec.ulpfec_payload_type = -1;
+ config.rtp.fec.red_rtx_payload_type = -1;
+ }
+ stream_ = call_->CreateVideoReceiveStream(config);
stream_->Start();
}
@@ -2529,6 +2514,12 @@
return info;
}
+void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetFecDisabledRemotely(
+ bool disable) {
+ red_disabled_by_remote_side_ = disable;
+ RecreateWebRtcStream();
+}
+
WebRtcVideoChannel2::VideoCodecSettings::VideoCodecSettings()
: rtx_payload_type(-1) {}
diff --git a/webrtc/media/engine/webrtcvideoengine2.h b/webrtc/media/engine/webrtcvideoengine2.h
index f6dd938..2d1b192 100644
--- a/webrtc/media/engine/webrtcvideoengine2.h
+++ b/webrtc/media/engine/webrtcvideoengine2.h
@@ -422,7 +422,8 @@
const webrtc::VideoReceiveStream::Config& config,
WebRtcVideoDecoderFactory* external_decoder_factory,
bool default_stream,
- const std::vector<VideoCodecSettings>& recv_codecs);
+ const std::vector<VideoCodecSettings>& recv_codecs,
+ bool red_disabled_by_remote_side);
~WebRtcVideoReceiveStream();
const std::vector<uint32_t>& GetSsrcs() const;
@@ -442,6 +443,14 @@
VideoReceiverInfo GetVideoReceiverInfo();
+ // Used to disable RED/FEC when the remote description doesn't contain those
+ // codecs. This is needed to be able to work around an RTX bug which is only
+ // happening if the remote side doesn't send RED, but the local side is
+ // configured to receive RED.
+ // TODO(holmer): Remove this after a couple of Chrome versions, M53-54
+ // time frame.
+ void SetFecDisabledRemotely(bool disable);
+
private:
struct AllocatedDecoder {
AllocatedDecoder(webrtc::VideoDecoder* decoder,
@@ -472,6 +481,7 @@
webrtc::VideoReceiveStream* stream_;
const bool default_stream_;
webrtc::VideoReceiveStream::Config config_;
+ bool red_disabled_by_remote_side_;
WebRtcVideoDecoderFactory* const external_decoder_factory_;
std::vector<AllocatedDecoder> allocated_decoders_;
@@ -542,6 +552,7 @@
VideoSendParameters send_params_;
VideoOptions default_send_options_;
VideoRecvParameters recv_params_;
+ bool red_disabled_by_remote_side_;
};
} // namespace cricket
diff --git a/webrtc/media/engine/webrtcvideoengine2_unittest.cc b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
index db34dd1..fd7a646 100644
--- a/webrtc/media/engine/webrtcvideoengine2_unittest.cc
+++ b/webrtc/media/engine/webrtcvideoengine2_unittest.cc
@@ -2678,6 +2678,7 @@
TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithoutFecDisablesFec) {
cricket::VideoSendParameters send_parameters;
send_parameters.codecs.push_back(kVp8Codec);
+ send_parameters.codecs.push_back(kRedCodec);
send_parameters.codecs.push_back(kUlpfecCodec);
ASSERT_TRUE(channel_->SetSendParameters(send_parameters));
@@ -2696,6 +2697,41 @@
<< "SetSendCodec without FEC should disable current FEC.";
}
+TEST_F(WebRtcVideoChannel2Test, SetSendParamsWithoutFecDisablesReceivingFec) {
+ FakeVideoReceiveStream* stream = AddRecvStream();
+ webrtc::VideoReceiveStream::Config config = stream->GetConfig();
+
+ EXPECT_EQ(kUlpfecCodec.id, config.rtp.fec.ulpfec_payload_type);
+
+ cricket::VideoRecvParameters recv_parameters;
+ recv_parameters.codecs.push_back(kVp8Codec);
+ recv_parameters.codecs.push_back(kRedCodec);
+ recv_parameters.codecs.push_back(kUlpfecCodec);
+ ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters));
+ stream = fake_call_->GetVideoReceiveStreams()[0];
+ ASSERT_TRUE(stream != NULL);
+ config = stream->GetConfig();
+ EXPECT_EQ(kUlpfecCodec.id, config.rtp.fec.ulpfec_payload_type)
+ << "FEC should be enabled on the recieve stream.";
+
+ cricket::VideoSendParameters send_parameters;
+ send_parameters.codecs.push_back(kVp8Codec);
+ ASSERT_TRUE(channel_->SetSendParameters(send_parameters));
+ stream = fake_call_->GetVideoReceiveStreams()[0];
+ config = stream->GetConfig();
+ EXPECT_EQ(-1, config.rtp.fec.ulpfec_payload_type)
+ << "FEC should have been disabled when we know the other side won't do "
+ "FEC.";
+
+ send_parameters.codecs.push_back(kRedCodec);
+ send_parameters.codecs.push_back(kUlpfecCodec);
+ ASSERT_TRUE(channel_->SetSendParameters(send_parameters));
+ stream = fake_call_->GetVideoReceiveStreams()[0];
+ config = stream->GetConfig();
+ EXPECT_EQ(kUlpfecCodec.id, config.rtp.fec.ulpfec_payload_type)
+ << "FEC should be enabled on the recieve stream.";
+}
+
TEST_F(WebRtcVideoChannel2Test, SetSendCodecsRejectDuplicateFecPayloads) {
cricket::VideoRecvParameters parameters;
parameters.codecs.push_back(kVp8Codec);
