From 126c03ea02d8a99bfa3d1e6d6fe04516183d31af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Mon, 11 May 2015 12:48:12 +0200 Subject: [PATCH] Base decision to send REMB on send codecs. Fixes bug where Chromium would send REMB even though the remote party doesn't announce support for it (because it was based on local codec settings instead of remote ones). BUG=4626 R=stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/54389004 Cr-Commit-Position: refs/heads/master@{#9170} --- talk/media/webrtc/webrtcvideoengine2.cc | 27 ++++++++++++++----- talk/media/webrtc/webrtcvideoengine2.h | 1 + .../webrtc/webrtcvideoengine2_unittest.cc | 6 ++--- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 4417e1942..8cbe029b0 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -937,12 +937,13 @@ bool WebRtcVideoChannel2::SetSendCodecs(const std::vector& codecs) { send_codec_.Set(supported_codecs.front()); rtc::CritScope stream_lock(&stream_crit_); - for (std::map::iterator it = - send_streams_.begin(); - it != send_streams_.end(); - ++it) { - DCHECK(it->second != NULL); - it->second->SetCodec(supported_codecs.front()); + for (auto& kv : send_streams_) { + DCHECK(kv.second != nullptr); + kv.second->SetCodec(supported_codecs.front()); + } + for (auto& kv : receive_streams_) { + DCHECK(kv.second != nullptr); + kv.second->SetRemb(HasRemb(supported_codecs.front().codec)); } // TODO(holmer): Changing the codec parameters shouldn't necessarily mean that @@ -1170,6 +1171,12 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp, config.audio_channel_id = voice_channel_id_; } + config.rtp.remb = false; + VideoCodecSettings send_codec; + if (send_codec_.Get(&send_codec)) { + config.rtp.remb = HasRemb(send_codec.codec); + } + receive_streams_[ssrc] = new WebRtcVideoReceiveStream( call_.get(), sp.ssrcs, external_decoder_factory_, default_stream, config, recv_codecs_); @@ -2286,12 +2293,18 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvCodecs( config_.rtp.fec = recv_codecs.front().fec; config_.rtp.nack.rtp_history_ms = HasNack(recv_codecs.begin()->codec) ? kNackHistoryMs : 0; - config_.rtp.remb = HasRemb(recv_codecs.begin()->codec); ClearDecoders(&old_decoders); RecreateWebRtcStream(); } +void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRemb(bool enabled) { + if (config_.rtp.remb == enabled) + return; + config_.rtp.remb = enabled; + RecreateWebRtcStream(); +} + void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRtpExtensions( const std::vector& extensions) { config_.rtp.extensions = extensions; diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index b7775dbfb..3bff8c2c4 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -412,6 +412,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, const std::vector& GetSsrcs() const; + void SetRemb(bool enabled); void SetRecvCodecs(const std::vector& recv_codecs); void SetRtpExtensions(const std::vector& extensions); diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc index 86700bbdc..89074ccd1 100644 --- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc @@ -1277,17 +1277,17 @@ TEST_F(WebRtcVideoChannel2Test, RembCanBeEnabledAndDisabled) { FakeVideoReceiveStream* stream = AddRecvStream(); EXPECT_TRUE(stream->GetConfig().rtp.remb); - // Verify that REMB is turned off when codecs without REMB are set. + // Verify that REMB is turned off when send(!) codecs without REMB are set. std::vector codecs; codecs.push_back(kVp8Codec); EXPECT_TRUE(codecs[0].feedback_params.params().empty()); - EXPECT_TRUE(channel_->SetRecvCodecs(codecs)); + EXPECT_TRUE(channel_->SetSendCodecs(codecs)); stream = fake_call_->GetVideoReceiveStreams()[0]; EXPECT_FALSE(stream->GetConfig().rtp.remb); // Verify that REMB is turned on when setting default codecs since the // default codecs have REMB enabled. - EXPECT_TRUE(channel_->SetRecvCodecs(engine_.codecs())); + EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs())); stream = fake_call_->GetVideoReceiveStreams()[0]; EXPECT_TRUE(stream->GetConfig().rtp.remb); }