From 257e130a1639febeb3ffc4d42943be3cb58151c7 Mon Sep 17 00:00:00 2001
From: "pbos@webrtc.org" <pbos@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Fri, 25 Jul 2014 19:01:32 +0000
Subject: [PATCH] Set NACK/REMB when setting receive codecs.

Enabling an additional test to ensure that REMB can be both enabled and
disabled by setting VideoCodecs.

BUG=1788
R=wu@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/15049004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6785 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 talk/media/webrtc/webrtcvideoengine2.cc       | 16 ++++---
 .../webrtc/webrtcvideoengine2_unittest.cc     | 42 +++++++++----------
 2 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc
index 86c9acc16b..aeba897d1c 100644
--- a/talk/media/webrtc/webrtcvideoengine2.cc
+++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -99,6 +99,11 @@ static bool IsNackEnabled(const VideoCodec& codec) {
       FeedbackParam(kRtcpFbParamNack, kParamValueEmpty));
 }
 
+static bool IsRembEnabled(const VideoCodec& codec) {
+  return codec.HasFeedbackParam(
+      FeedbackParam(kRtcpFbParamRemb, kParamValueEmpty));
+}
+
 static VideoCodec DefaultVideoCodec() {
   VideoCodec default_codec(kDefaultVideoCodecPref.payload_type,
                            kDefaultVideoCodecPref.name,
@@ -736,8 +741,6 @@ static std::string RtpExtensionsToString(
 }  // namespace
 
 bool WebRtcVideoChannel2::SetRecvCodecs(const std::vector<VideoCodec>& codecs) {
-  // TODO(pbos): Must these receive codecs propagate to existing receive
-  // streams?
   LOG(LS_INFO) << "SetRecvCodecs: " << CodecVectorToString(codecs);
   if (!ValidateCodecFormats(codecs)) {
     return false;
@@ -951,11 +954,8 @@ void WebRtcVideoChannel2::ConfigureReceiverRtp(
   config->rtp.remote_ssrc = ssrc;
   config->rtp.local_ssrc = rtcp_receiver_report_ssrc_;
 
-  if (IsNackEnabled(recv_codecs_.begin()->codec)) {
-    config->rtp.nack.rtp_history_ms = kNackHistoryMs;
-  }
-  config->rtp.remb = true;
   config->rtp.extensions = recv_rtp_extensions_;
+
   // TODO(pbos): This protection is against setting the same local ssrc as
   // remote which is not permitted by the lower-level API. RTCP requires a
   // corresponding sender SSRC. Figure out what to do when we don't have
@@ -1705,6 +1705,10 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvCodecs(
 
   config_.rtp.fec = recv_codecs.front().fec;
 
+  config_.rtp.nack.rtp_history_ms =
+      IsNackEnabled(recv_codecs.begin()->codec) ? kNackHistoryMs : 0;
+  config_.rtp.remb = IsRembEnabled(recv_codecs.begin()->codec);
+
   RecreateWebRtcStream();
 }
 
diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
index 94c18c71fc..c2f36d0c08 100644
--- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc
+++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
@@ -793,24 +793,6 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_StartSendBitrate) {
 #endif
 }
 
-TEST_F(WebRtcVideoChannel2Test, DISABLED_RtcpEnabled) {
-  // Note(pbos): This is a receiver-side setting, dumbo.
-  FAIL() << "Not implemented.";  // TODO(pbos): Implement.
-}
-
-TEST_F(WebRtcVideoChannel2Test, DISABLED_KeyFrameRequestEnabled) {
-  FAIL() << "Not implemented.";  // TODO(pbos): Implement.
-}
-
-TEST_F(WebRtcVideoChannel2Test, RembIsEnabledByDefault) {
-  FakeVideoReceiveStream* stream = AddRecvStream();
-  EXPECT_TRUE(stream->GetConfig().rtp.remb);
-}
-
-TEST_F(WebRtcVideoChannel2Test, DISABLED_RembEnabledOnReceiveChannels) {
-  FAIL() << "Not implemented.";  // TODO(pbos): Implement.
-}
-
 TEST_F(WebRtcVideoChannel2Test, RecvStreamWithSimAndRtx) {
   EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs()));
   EXPECT_TRUE(channel_->SetSend(true));
@@ -1010,12 +992,28 @@ TEST_F(WebRtcVideoChannel2Test, AddRecvStreamOnlyUsesOneReceiveStream) {
   EXPECT_EQ(1u, fake_channel_->GetFakeCall()->GetVideoReceiveStreams().size());
 }
 
-TEST_F(WebRtcVideoChannel2Test, DISABLED_NoRembChangeAfterAddRecvStream) {
-  FAIL() << "Not implemented.";  // TODO(pbos): Implement.
+TEST_F(WebRtcVideoChannel2Test, RembIsEnabledByDefault) {
+  FakeVideoReceiveStream* stream = AddRecvStream();
+  EXPECT_TRUE(stream->GetConfig().rtp.remb);
 }
 
-TEST_F(WebRtcVideoChannel2Test, DISABLED_RembOnOff) {
-  FAIL() << "Not implemented.";  // TODO(pbos): Implement.
+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.
+  std::vector<VideoCodec> codecs;
+  codecs.push_back(kVp8Codec);
+  EXPECT_TRUE(codecs[0].feedback_params.params().empty());
+  EXPECT_TRUE(channel_->SetRecvCodecs(codecs));
+  stream = fake_channel_->GetFakeCall()->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()));
+  stream = fake_channel_->GetFakeCall()->GetVideoReceiveStreams()[0];
+  EXPECT_TRUE(stream->GetConfig().rtp.remb);
 }
 
 TEST_F(WebRtcVideoChannel2Test, NackIsEnabledByDefault) {