From 05e7b44b83f9f12a827646c496f5d6ae796b4b99 Mon Sep 17 00:00:00 2001 From: "wu@webrtc.org" Date: Tue, 1 Apr 2014 17:44:24 +0000 Subject: [PATCH] (Auto)update libjingle 63948945-> 64147530 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5825 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/media/webrtc/fakewebrtcvideoengine.h | 2 +- talk/media/webrtc/fakewebrtcvoiceengine.h | 5 + talk/media/webrtc/webrtcvideoengine.cc | 138 ++++++++++++++---- .../webrtc/webrtcvideoengine_unittest.cc | 23 ++- talk/media/webrtc/webrtcvoiceengine.cc | 36 ++++- .../webrtc/webrtcvoiceengine_unittest.cc | 19 +++ talk/p2p/base/p2ptransportchannel.cc | 43 ++++-- talk/p2p/base/p2ptransportchannel.h | 1 + 8 files changed, 224 insertions(+), 43 deletions(-) diff --git a/talk/media/webrtc/fakewebrtcvideoengine.h b/talk/media/webrtc/fakewebrtcvideoengine.h index 4096ff1e4b..1b6031a887 100644 --- a/talk/media/webrtc/fakewebrtcvideoengine.h +++ b/talk/media/webrtc/fakewebrtcvideoengine.h @@ -433,7 +433,7 @@ class FakeWebRtcVideoEngine void set_fail_alloc_capturer(bool fail_alloc_capturer) { fail_alloc_capturer_ = fail_alloc_capturer; } - int num_set_send_codecs() const { return num_set_send_codecs_; } + int GetNumSetSendCodecs() const { return num_set_send_codecs_; } int GetCaptureId(int channel) const { WEBRTC_ASSERT_CHANNEL(channel); diff --git a/talk/media/webrtc/fakewebrtcvoiceengine.h b/talk/media/webrtc/fakewebrtcvoiceengine.h index bac53310cd..b552b49aa0 100644 --- a/talk/media/webrtc/fakewebrtcvoiceengine.h +++ b/talk/media/webrtc/fakewebrtcvoiceengine.h @@ -151,6 +151,7 @@ class FakeWebRtcVoiceEngine fail_create_channel_(false), codecs_(codecs), num_codecs_(num_codecs), + num_set_send_codecs_(0), ec_enabled_(false), ec_metrics_enabled_(false), cng_enabled_(false), @@ -297,6 +298,8 @@ class FakeWebRtcVoiceEngine return channels_[channel]->receive_absolute_sender_time_ext_; } + int GetNumSetSendCodecs() const { return num_set_send_codecs_; } + WEBRTC_STUB(Release, ()); // webrtc::VoEBase @@ -401,6 +404,7 @@ class FakeWebRtcVoiceEngine return -1; } channels_[channel]->send_codec = codec; + ++num_set_send_codecs_; return 0; } WEBRTC_FUNC(GetSendCodec, (int channel, webrtc::CodecInst& codec)) { @@ -1082,6 +1086,7 @@ class FakeWebRtcVoiceEngine bool fail_create_channel_; const cricket::AudioCodec* const* codecs_; int num_codecs_; + int num_set_send_codecs_; // how many times we call SetSendCodec(). bool ec_enabled_; bool ec_metrics_enabled_; bool cng_enabled_; diff --git a/talk/media/webrtc/webrtcvideoengine.cc b/talk/media/webrtc/webrtcvideoengine.cc index 53c03836e2..98e65caacd 100644 --- a/talk/media/webrtc/webrtcvideoengine.cc +++ b/talk/media/webrtc/webrtcvideoengine.cc @@ -164,6 +164,73 @@ static bool IsRembEnabled(const VideoCodec& codec) { kParamValueEmpty)); } +// TODO(mallinath) - Remove this after trunk of webrtc is pushed to GTP. +#if !defined(USE_WEBRTC_DEV_BRANCH) +bool operator==(const webrtc::VideoCodecVP8& lhs, + const webrtc::VideoCodecVP8& rhs) { + return lhs.pictureLossIndicationOn == rhs.pictureLossIndicationOn && + lhs.feedbackModeOn == rhs.feedbackModeOn && + lhs.complexity == rhs.complexity && + lhs.resilience == rhs.resilience && + lhs.numberOfTemporalLayers == rhs.numberOfTemporalLayers && + lhs.denoisingOn == rhs.denoisingOn && + lhs.errorConcealmentOn == rhs.errorConcealmentOn && + lhs.automaticResizeOn == rhs.automaticResizeOn && + lhs.frameDroppingOn == rhs.frameDroppingOn && + lhs.keyFrameInterval == rhs.keyFrameInterval; +} + +bool operator!=(const webrtc::VideoCodecVP8& lhs, + const webrtc::VideoCodecVP8& rhs) { + return !(lhs == rhs); +} + +bool operator==(const webrtc::SimulcastStream& lhs, + const webrtc::SimulcastStream& rhs) { + return lhs.width == rhs.width && + lhs.height == rhs.height && + lhs.numberOfTemporalLayers == rhs.numberOfTemporalLayers && + lhs.maxBitrate == rhs.maxBitrate && + lhs.targetBitrate == rhs.targetBitrate && + lhs.minBitrate == rhs.minBitrate && + lhs.qpMax == rhs.qpMax; +} + +bool operator!=(const webrtc::SimulcastStream& lhs, + const webrtc::SimulcastStream& rhs) { + return !(lhs == rhs); +} + +bool operator==(const webrtc::VideoCodec& lhs, + const webrtc::VideoCodec& rhs) { + bool ret = lhs.codecType == rhs.codecType && + (_stricmp(lhs.plName, rhs.plName) == 0) && + lhs.plType == rhs.plType && + lhs.width == rhs.width && + lhs.height == rhs.height && + lhs.startBitrate == rhs.startBitrate && + lhs.maxBitrate == rhs.maxBitrate && + lhs.minBitrate == rhs.minBitrate && + lhs.maxFramerate == rhs.maxFramerate && + lhs.qpMax == rhs.qpMax && + lhs.numberOfSimulcastStreams == rhs.numberOfSimulcastStreams && + lhs.mode == rhs.mode; + if (ret && lhs.codecType == webrtc::kVideoCodecVP8) { + ret &= (lhs.codecSpecific.VP8 == rhs.codecSpecific.VP8); + } + + for (unsigned char i = 0; i < rhs.numberOfSimulcastStreams && ret; ++i) { + ret &= (lhs.simulcastStream[i] == rhs.simulcastStream[i]); + } + return ret; +} + +bool operator!=(const webrtc::VideoCodec& lhs, + const webrtc::VideoCodec& rhs) { + return !(lhs == rhs); +} +#endif + struct FlushBlackFrameData : public talk_base::MessageData { FlushBlackFrameData(uint32 s, int64 t) : ssrc(s), timestamp(t) { } @@ -1647,6 +1714,8 @@ bool WebRtcVideoMediaChannel::SetSendCodecs( ConvertToCricketVideoCodec(*send_codec_, ¤t); } std::map primary_rtx_pt_mapping; + bool nack_enabled = nack_enabled_; + bool remb_enabled = remb_enabled_; for (std::vector::const_iterator iter = codecs.begin(); iter != codecs.end(); ++iter) { if (_stricmp(iter->name.c_str(), kRedPayloadName) == 0) { @@ -1663,8 +1732,8 @@ bool WebRtcVideoMediaChannel::SetSendCodecs( webrtc::VideoCodec wcodec; if (engine()->ConvertFromCricketVideoCodec(checked_codec, &wcodec)) { if (send_codecs.empty()) { - nack_enabled_ = IsNackEnabled(checked_codec); - remb_enabled_ = IsRembEnabled(checked_codec); + nack_enabled = IsNackEnabled(checked_codec); + remb_enabled = IsRembEnabled(checked_codec); } send_codecs.push_back(wcodec); } @@ -1680,35 +1749,43 @@ bool WebRtcVideoMediaChannel::SetSendCodecs( } // Recv protection. - for (RecvChannelMap::iterator it = recv_channels_.begin(); - it != recv_channels_.end(); ++it) { - int channel_id = it->second->channel_id(); - if (!SetNackFec(channel_id, send_red_type_, send_fec_type_, - nack_enabled_)) { - return false; - } - if (engine_->vie()->rtp()->SetRembStatus(channel_id, - kNotSending, - remb_enabled_) != 0) { - LOG_RTCERR3(SetRembStatus, channel_id, kNotSending, remb_enabled_); - return false; + // Do not update if the status is same as previously configured. + if (nack_enabled_ != nack_enabled) { + for (RecvChannelMap::iterator it = recv_channels_.begin(); + it != recv_channels_.end(); ++it) { + int channel_id = it->second->channel_id(); + if (!SetNackFec(channel_id, send_red_type_, send_fec_type_, + nack_enabled)) { + return false; + } + if (engine_->vie()->rtp()->SetRembStatus(channel_id, + kNotSending, + remb_enabled_) != 0) { + LOG_RTCERR3(SetRembStatus, channel_id, kNotSending, remb_enabled_); + return false; + } } + nack_enabled_ = nack_enabled; } // Send settings. - for (SendChannelMap::iterator iter = send_channels_.begin(); - iter != send_channels_.end(); ++iter) { - int channel_id = iter->second->channel_id(); - if (!SetNackFec(channel_id, send_red_type_, send_fec_type_, - nack_enabled_)) { - return false; - } - if (engine_->vie()->rtp()->SetRembStatus(channel_id, - remb_enabled_, - remb_enabled_) != 0) { - LOG_RTCERR3(SetRembStatus, channel_id, remb_enabled_, remb_enabled_); - return false; + // Do not update if the status is same as previously configured. + if (remb_enabled_ != remb_enabled) { + for (SendChannelMap::iterator iter = send_channels_.begin(); + iter != send_channels_.end(); ++iter) { + int channel_id = iter->second->channel_id(); + if (!SetNackFec(channel_id, send_red_type_, send_fec_type_, + nack_enabled_)) { + return false; + } + if (engine_->vie()->rtp()->SetRembStatus(channel_id, + remb_enabled, + remb_enabled) != 0) { + LOG_RTCERR3(SetRembStatus, channel_id, remb_enabled, remb_enabled); + return false; + } } + remb_enabled_ = remb_enabled; } // Select the first matched codec. @@ -3646,6 +3723,15 @@ bool WebRtcVideoMediaChannel::SetSendCodec( << "for ssrc: " << ssrc << "."; } else { MaybeChangeStartBitrate(channel_id, &target_codec); + webrtc::VideoCodec current_codec; + if (!engine()->vie()->codec()->GetSendCodec(channel_id, current_codec)) { + // Compare against existing configured send codec. + if (current_codec == target_codec) { + // Codec is already configured on channel. no need to apply. + return true; + } + } + if (0 != engine()->vie()->codec()->SetSendCodec(channel_id, target_codec)) { LOG_RTCERR2(SetSendCodec, channel_id, target_codec.plName); return false; diff --git a/talk/media/webrtc/webrtcvideoengine_unittest.cc b/talk/media/webrtc/webrtcvideoengine_unittest.cc index a61b8d8a3d..32e742c2f9 100644 --- a/talk/media/webrtc/webrtcvideoengine_unittest.cc +++ b/talk/media/webrtc/webrtcvideoengine_unittest.cc @@ -313,9 +313,28 @@ TEST_F(WebRtcVideoEngineTestFake, SetSendCodecs) { VerifyVP8SendCodec(channel_num, kVP8Codec.width, kVP8Codec.height); EXPECT_TRUE(vie_.GetHybridNackFecStatus(channel_num)); EXPECT_FALSE(vie_.GetNackStatus(channel_num)); + EXPECT_EQ(1, vie_.GetNumSetSendCodecs()); // TODO(juberti): Check RTCP, PLI, TMMBR. } +// Test that ViE Channel doesn't call SetSendCodec again if same codec is tried +// to apply. +TEST_F(WebRtcVideoEngineTestFake, DontResetSetSendCodec) { + EXPECT_TRUE(SetupEngine()); + int channel_num = vie_.GetLastChannel(); + std::vector codecs(engine_.codecs()); + EXPECT_TRUE(channel_->SetSendCodecs(codecs)); + VerifyVP8SendCodec(channel_num, kVP8Codec.width, kVP8Codec.height); + EXPECT_TRUE(vie_.GetHybridNackFecStatus(channel_num)); + EXPECT_FALSE(vie_.GetNackStatus(channel_num)); + EXPECT_EQ(1, vie_.GetNumSetSendCodecs()); + // Try setting same code again. + EXPECT_TRUE(channel_->SetSendCodecs(codecs)); + // Since it's exact same codec which is already set, media channel shouldn't + // send the codec to ViE. + EXPECT_EQ(1, vie_.GetNumSetSendCodecs()); +} + TEST_F(WebRtcVideoEngineTestFake, SetSendCodecsWithMinMaxBitrate) { EXPECT_TRUE(SetupEngine()); int channel_num = vie_.GetLastChannel(); @@ -1654,7 +1673,7 @@ TEST_F(WebRtcVideoEngineTestFake, ResetCodecOnScreencast) { cricket::StreamParams::CreateLegacy(123))); EXPECT_TRUE(channel_->SetSendCodecs(codec_list)); EXPECT_TRUE(channel_->SetSend(true)); - EXPECT_EQ(1, vie_.num_set_send_codecs()); + EXPECT_EQ(1, vie_.GetNumSetSendCodecs()); webrtc::VideoCodec gcodec; memset(&gcodec, 0, sizeof(gcodec)); @@ -1665,7 +1684,7 @@ TEST_F(WebRtcVideoEngineTestFake, ResetCodecOnScreencast) { // Send a screencast frame with the same size. // Verify that denoising is turned off. SendI420ScreencastFrame(kVP8Codec.width, kVP8Codec.height); - EXPECT_EQ(2, vie_.num_set_send_codecs()); + EXPECT_EQ(2, vie_.GetNumSetSendCodecs()); EXPECT_EQ(0, vie_.GetSendCodec(channel_num, gcodec)); EXPECT_FALSE(gcodec.codecSpecific.VP8.denoisingOn); } diff --git a/talk/media/webrtc/webrtcvoiceengine.cc b/talk/media/webrtc/webrtcvoiceengine.cc index b308e4e6d9..bf4cef6e01 100644 --- a/talk/media/webrtc/webrtcvoiceengine.cc +++ b/talk/media/webrtc/webrtcvoiceengine.cc @@ -220,6 +220,22 @@ static bool IsNackEnabled(const AudioCodec& codec) { kParamValueEmpty)); } +// TODO(mallinath) - Remove this after trunk of webrtc is pushed to GTP. +#if !defined(USE_WEBRTC_DEV_BRANCH) +bool operator==(const webrtc::CodecInst& lhs, const webrtc::CodecInst& rhs) { + return lhs.pltype == rhs.pltype && + (_stricmp(lhs.plname, rhs.plname) == 0) && + lhs.plfreq == rhs.plfreq && + lhs.pacsize == rhs.pacsize && + lhs.channels == rhs.channels && + lhs.rate == rhs.rate; +} + +bool operator!=(const webrtc::CodecInst& lhs, const webrtc::CodecInst& rhs) { + return !(lhs == rhs); +} +#endif + // Gets the default set of options applied to the engine. Historically, these // were supplied as a combination of flags from the channel manager (ec, agc, // ns, and highpass) and the rest hardcoded in InitInternal. @@ -1981,6 +1997,8 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( webrtc::CodecInst send_codec; memset(&send_codec, 0, sizeof(send_codec)); + bool nack_enabled = nack_enabled_; + // Set send codec (the first non-telephone-event/CN codec) for (std::vector::const_iterator it = codecs.begin(); it != codecs.end(); ++it) { @@ -2052,13 +2070,17 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( } } else { send_codec = voe_codec; - nack_enabled_ = IsNackEnabled(*it); - SetNack(channel, nack_enabled_); + nack_enabled = IsNackEnabled(*it); } found_send_codec = true; break; } + if (nack_enabled_ != nack_enabled) { + SetNack(channel, nack_enabled); + nack_enabled_ = nack_enabled; + } + if (!found_send_codec) { LOG(LS_WARNING) << "Received empty list of codecs."; return false; @@ -2142,7 +2164,6 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( } } } - return true; } @@ -2167,8 +2188,8 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs( } } + // Set nack status on receive channels and update |nack_enabled_|. SetNack(receive_channels_, nack_enabled_); - return true; } @@ -2208,6 +2229,13 @@ bool WebRtcVoiceMediaChannel::SetSendCodec( LOG(LS_INFO) << "Send channel " << channel << " selected voice codec " << ToString(send_codec) << ", bitrate=" << send_codec.rate; + webrtc::CodecInst current_codec; + if (engine()->voe()->codec()->GetSendCodec(channel, current_codec) == 0 && + (send_codec == current_codec)) { + // Codec is already configured, we can return without setting it again. + return true; + } + if (engine()->voe()->codec()->SetSendCodec(channel, send_codec) == -1) { LOG_RTCERR2(SetSendCodec, channel, ToString(send_codec)); return false; diff --git a/talk/media/webrtc/webrtcvoiceengine_unittest.cc b/talk/media/webrtc/webrtcvoiceengine_unittest.cc index eba25be583..4dbeebaaa9 100644 --- a/talk/media/webrtc/webrtcvoiceengine_unittest.cc +++ b/talk/media/webrtc/webrtcvoiceengine_unittest.cc @@ -743,6 +743,7 @@ TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecs) { codecs[0].id = 96; codecs[0].bitrate = 48000; EXPECT_TRUE(channel_->SetSendCodecs(codecs)); + EXPECT_EQ(1, voe_.GetNumSetSendCodecs()); webrtc::CodecInst gcodec; EXPECT_EQ(0, voe_.GetSendCodec(channel_num, gcodec)); EXPECT_EQ(96, gcodec.pltype); @@ -755,6 +756,24 @@ TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecs) { EXPECT_EQ(106, voe_.GetSendTelephoneEventPayloadType(channel_num)); } +// Test that VoE Channel doesn't call SetSendCodec again if same codec is tried +// to apply. +TEST_F(WebRtcVoiceEngineTestFake, DontResetSetSendCodec) { + EXPECT_TRUE(SetupEngine()); + std::vector codecs; + codecs.push_back(kIsacCodec); + codecs.push_back(kPcmuCodec); + codecs.push_back(kRedCodec); + codecs[0].id = 96; + codecs[0].bitrate = 48000; + EXPECT_TRUE(channel_->SetSendCodecs(codecs)); + EXPECT_EQ(1, voe_.GetNumSetSendCodecs()); + // Calling SetSendCodec again with same codec which is already set. + // In this case media channel shouldn't send codec to VoE. + EXPECT_TRUE(channel_->SetSendCodecs(codecs)); + EXPECT_EQ(1, voe_.GetNumSetSendCodecs()); +} + // Test that if clockrate is not 48000 for opus, we fail. TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecOpusBadClockrate) { EXPECT_TRUE(SetupEngine()); diff --git a/talk/p2p/base/p2ptransportchannel.cc b/talk/p2p/base/p2ptransportchannel.cc index 99ca858c89..e7f9d45f02 100644 --- a/talk/p2p/base/p2ptransportchannel.cc +++ b/talk/p2p/base/p2ptransportchannel.cc @@ -629,7 +629,7 @@ void P2PTransportChannel::OnCandidate(const Candidate& candidate) { // Creates connections from all of the ports that we care about to the given // remote candidate. The return value is true if we created a connection from // the origin port. -bool P2PTransportChannel::CreateConnections(const Candidate &remote_candidate, +bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate, PortInterface* origin_port, bool readable) { ASSERT(worker_thread_ == talk_base::Thread::Current()); @@ -648,14 +648,25 @@ bool P2PTransportChannel::CreateConnections(const Candidate &remote_candidate, new_remote_candidate.set_password(remote_ice_pwd_); } + // If we've already seen the new remote candidate (in the current candidate + // generation), then we shouldn't try creating connections for it. + // We either already have a connection for it, or we previously created one + // and then later pruned it. If we don't return, the channel will again + // re-create any connections that were previously pruned, which will then + // immediately be re-pruned, churning the network for no purpose. + // This only applies to candidates received over signaling (i.e. origin_port + // is NULL). + if (!origin_port && IsDuplicateRemoteCandidate(new_remote_candidate)) { + // return true to indicate success, without creating any new connections. + return true; + } + // Add a new connection for this candidate to every port that allows such a // connection (i.e., if they have compatible protocols) and that does not // already have a connection to an equivalent candidate. We must be careful // to make sure that the origin port is included, even if it was pruned, // since that may be the only port that can create this connection. - bool created = false; - std::vector::reverse_iterator it; for (it = ports_.rbegin(); it != ports_.rend(); ++it) { if (CreateConnection(*it, new_remote_candidate, origin_port, readable)) { @@ -690,7 +701,11 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port, // It is not legal to try to change any of the parameters of an existing // connection; however, the other side can send a duplicate candidate. if (!remote_candidate.IsEquivalent(connection->remote_candidate())) { - LOG(INFO) << "Attempt to change a remote candidate"; + LOG(INFO) << "Attempt to change a remote candidate." + << " Existing remote candidate: " + << connection->remote_candidate().ToString() + << "New remote candidate: " + << remote_candidate.ToString(); return false; } } else { @@ -740,6 +755,17 @@ uint32 P2PTransportChannel::GetRemoteCandidateGeneration( return remote_candidate_generation_; } +// Check if remote candidate is already cached. +bool P2PTransportChannel::IsDuplicateRemoteCandidate( + const Candidate& candidate) { + for (uint32 i = 0; i < remote_candidates_.size(); ++i) { + if (remote_candidates_[i].IsEquivalent(candidate)) { + return true; + } + } + return false; +} + // Maintain our remote candidate list, adding this new remote one. void P2PTransportChannel::RememberRemoteCandidate( const Candidate& remote_candidate, PortInterface* origin_port) { @@ -757,12 +783,9 @@ void P2PTransportChannel::RememberRemoteCandidate( } // Make sure this candidate is not a duplicate. - for (uint32 i = 0; i < remote_candidates_.size(); ++i) { - if (remote_candidates_[i].IsEquivalent(remote_candidate)) { - LOG(INFO) << "Duplicate candidate: " - << remote_candidate.address().ToSensitiveString(); - return; - } + if (IsDuplicateRemoteCandidate(remote_candidate)) { + LOG(INFO) << "Duplicate candidate: " << remote_candidate.ToString(); + return; } // Try this candidate for all future ports. diff --git a/talk/p2p/base/p2ptransportchannel.h b/talk/p2p/base/p2ptransportchannel.h index 57fc3a61af..09dabd5677 100644 --- a/talk/p2p/base/p2ptransportchannel.h +++ b/talk/p2p/base/p2ptransportchannel.h @@ -189,6 +189,7 @@ class P2PTransportChannel : public TransportChannelImpl, bool FindConnection(cricket::Connection* connection) const; uint32 GetRemoteCandidateGeneration(const Candidate& candidate); + bool IsDuplicateRemoteCandidate(const Candidate& candidate); void RememberRemoteCandidate(const Candidate& remote_candidate, PortInterface* origin_port); bool IsPingable(Connection* conn);