From 67c9df79828991c5aab96b9253ae4e7ba7ed508e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= <pbos@webrtc.org>
Date: Mon, 11 May 2015 14:34:58 +0200
Subject: [PATCH] Base NACK on send codecs.

Addressing discrepancy where NACK used to be set from send codecs in
WebRtcVideoEngine(1), and before this change, from recv codecs in
WebRtcVideoEngine2. This should address that NACK might be sent even if
the remote side does not support it.

BUG=4626
R=stefan@webrtc.org

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

Cr-Commit-Position: refs/heads/master@{#9171}
---
 talk/media/webrtc/webrtcvideoengine2.cc       | 19 +++++++-----
 talk/media/webrtc/webrtcvideoengine2.h        |  2 +-
 .../webrtc/webrtcvideoengine2_unittest.cc     | 31 ++++++++++++-------
 3 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc
index 8cbe029b0d..0c69ec581e 100644
--- a/talk/media/webrtc/webrtcvideoengine2.cc
+++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -943,7 +943,8 @@ bool WebRtcVideoChannel2::SetSendCodecs(const std::vector<VideoCodec>& codecs) {
   }
   for (auto& kv : receive_streams_) {
     DCHECK(kv.second != nullptr);
-    kv.second->SetRemb(HasRemb(supported_codecs.front().codec));
+    kv.second->SetNackAndRemb(HasNack(supported_codecs.front().codec),
+                              HasRemb(supported_codecs.front().codec));
   }
 
   // TODO(holmer): Changing the codec parameters shouldn't necessarily mean that
@@ -1920,9 +1921,8 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodecAndOptions(
     }
   }
 
-  if (HasNack(codec_settings.codec)) {
-    parameters_.config.rtp.nack.rtp_history_ms = kNackHistoryMs;
-  }
+  parameters_.config.rtp.nack.rtp_history_ms =
+      HasNack(codec_settings.codec) ? kNackHistoryMs : 0;
 
   options.suspend_below_min_bitrate.Get(
       &parameters_.config.suspend_below_min_bitrate);
@@ -2298,10 +2298,15 @@ void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRecvCodecs(
   RecreateWebRtcStream();
 }
 
-void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetRemb(bool enabled) {
-  if (config_.rtp.remb == enabled)
+void WebRtcVideoChannel2::WebRtcVideoReceiveStream::SetNackAndRemb(
+    bool nack_enabled, bool remb_enabled) {
+  int nack_history_ms = nack_enabled ? kNackHistoryMs : 0;
+  if (config_.rtp.nack.rtp_history_ms == nack_history_ms &&
+      config_.rtp.remb == remb_enabled) {
     return;
-  config_.rtp.remb = enabled;
+  }
+  config_.rtp.remb = remb_enabled;
+  config_.rtp.nack.rtp_history_ms = nack_history_ms;
   RecreateWebRtcStream();
 }
 
diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h
index 3bff8c2c4e..8c33069fe8 100644
--- a/talk/media/webrtc/webrtcvideoengine2.h
+++ b/talk/media/webrtc/webrtcvideoengine2.h
@@ -412,7 +412,7 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler,
 
     const std::vector<uint32>& GetSsrcs() const;
 
-    void SetRemb(bool enabled);
+    void SetNackAndRemb(bool nack_enabled, bool remb_enabled);
     void SetRecvCodecs(const std::vector<VideoCodecSettings>& recv_codecs);
     void SetRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions);
 
diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
index 89074ccd1e..4cffada418 100644
--- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc
+++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
@@ -1313,21 +1313,30 @@ TEST_F(WebRtcVideoChannel2Test, NackIsEnabledByDefault) {
             recv_stream->GetConfig().rtp.nack.rtp_history_ms);
 }
 
-TEST_F(WebRtcVideoChannel2Test, NackCanBeDisabled) {
+TEST_F(WebRtcVideoChannel2Test, NackCanBeEnabledAndDisabled) {
+  FakeVideoReceiveStream* recv_stream = AddRecvStream();
+  FakeVideoSendStream* send_stream = AddSendStream();
+
+  EXPECT_GT(recv_stream->GetConfig().rtp.nack.rtp_history_ms, 0);
+  EXPECT_GT(send_stream->GetConfig().rtp.nack.rtp_history_ms, 0);
+
+  // Verify that NACK is turned off when send(!) codecs without NACK are set.
   std::vector<VideoCodec> codecs;
   codecs.push_back(kVp8Codec);
-
-  // Send side.
-  ASSERT_TRUE(channel_->SetSendCodecs(codecs));
-  FakeVideoSendStream* send_stream =
-      AddSendStream(cricket::StreamParams::CreateLegacy(1));
+  EXPECT_TRUE(codecs[0].feedback_params.params().empty());
+  EXPECT_TRUE(channel_->SetSendCodecs(codecs));
+  recv_stream = fake_call_->GetVideoReceiveStreams()[0];
+  EXPECT_EQ(0, recv_stream->GetConfig().rtp.nack.rtp_history_ms);
+  send_stream = fake_call_->GetVideoSendStreams()[0];
   EXPECT_EQ(0, send_stream->GetConfig().rtp.nack.rtp_history_ms);
 
-  // Receiver side.
-  ASSERT_TRUE(channel_->SetRecvCodecs(codecs));
-  FakeVideoReceiveStream* recv_stream =
-      AddRecvStream(cricket::StreamParams::CreateLegacy(1));
-  EXPECT_EQ(0, recv_stream->GetConfig().rtp.nack.rtp_history_ms);
+  // Verify that NACK is turned on when setting default codecs since the
+  // default codecs have NACK enabled.
+  EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs()));
+  recv_stream = fake_call_->GetVideoReceiveStreams()[0];
+  EXPECT_GT(recv_stream->GetConfig().rtp.nack.rtp_history_ms, 0);
+  send_stream = fake_call_->GetVideoSendStreams()[0];
+  EXPECT_GT(send_stream->GetConfig().rtp.nack.rtp_history_ms, 0);
 }
 
 TEST_F(WebRtcVideoChannel2Test, DISABLED_VideoProtectionInterop) {