From f99c2f2dbcaab24b45295cb9e06c3c52ad349d81 Mon Sep 17 00:00:00 2001
From: "pbos@webrtc.org" <pbos@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Fri, 13 Jun 2014 12:27:38 +0000
Subject: [PATCH] Add NACK feedback parameter to WebRtcVideoEngine2.

Also fixing enabling/disabling of NACK. Previous implementation was made
under the assumption that NACK should always be enabled which caused
both missing NACK settings in SDP as well as broken interop between old
and new WebRtcVideoEngines.

BUG=1788
R=pthatcher@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6431 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 talk/media/webrtc/webrtcvideoengine2.cc       | 25 ++++++++++++--
 .../webrtc/webrtcvideoengine2_unittest.cc     | 33 ++++++++++++++++++-
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc
index bf731c4ca..d6d1354bf 100644
--- a/talk/media/webrtc/webrtcvideoengine2.cc
+++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -161,6 +161,22 @@ static bool FindBestVideoFormat(int max_width,
   return false;
 }
 
+static void AddDefaultFeedbackParams(VideoCodec* codec) {
+  const FeedbackParam kFir(kRtcpFbParamCcm, kRtcpFbCcmParamFir);
+  codec->AddFeedbackParam(kFir);
+  const FeedbackParam kNack(kRtcpFbParamNack, kParamValueEmpty);
+  codec->AddFeedbackParam(kNack);
+  const FeedbackParam kPli(kRtcpFbParamNack, kRtcpFbNackParamPli);
+  codec->AddFeedbackParam(kPli);
+  const FeedbackParam kRemb(kRtcpFbParamRemb, kParamValueEmpty);
+  codec->AddFeedbackParam(kRemb);
+}
+
+static bool IsNackEnabled(const VideoCodec& codec) {
+  return codec.HasFeedbackParam(
+      FeedbackParam(kRtcpFbParamNack, kParamValueEmpty));
+}
+
 static VideoCodec DefaultVideoCodec() {
   VideoCodec default_codec(kDefaultVideoCodecPref.payload_type,
                            kDefaultVideoCodecPref.name,
@@ -168,6 +184,7 @@ static VideoCodec DefaultVideoCodec() {
                            kDefaultVideoFormat.height,
                            kDefaultFramerate,
                            0);
+  AddDefaultFeedbackParams(&default_codec);
   return default_codec;
 }
 
@@ -950,7 +967,9 @@ bool WebRtcVideoChannel2::AddSendStream(const StreamParams& sp) {
     config.rtp.rtx.payload_type = codec_settings.rtx_payload_type;
   }
 
-  config.rtp.nack.rtp_history_ms = kNackHistoryMs;
+  if (IsNackEnabled(codec_settings.codec)) {
+    config.rtp.nack.rtp_history_ms = kNackHistoryMs;
+  }
   config.rtp.max_packet_size = kVideoMtu;
 
   WebRtcVideoSendStream* stream =
@@ -1024,7 +1043,9 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) {
   config.rtp.remote_ssrc = ssrc;
   config.rtp.local_ssrc = rtcp_receiver_report_ssrc_;
 
-  config.rtp.nack.rtp_history_ms = kNackHistoryMs;
+  if (IsNackEnabled(recv_codecs_.begin()->codec)) {
+    config.rtp.nack.rtp_history_ms = kNackHistoryMs;
+  }
   config.rtp.remb = true;
   // 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
diff --git a/talk/media/webrtc/webrtcvideoengine2_unittest.cc b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
index 4dd27bc79..c9ff18290 100644
--- a/talk/media/webrtc/webrtcvideoengine2_unittest.cc
+++ b/talk/media/webrtc/webrtcvideoengine2_unittest.cc
@@ -48,6 +48,18 @@ static const cricket::VideoCodec kUlpfecCodec(117, "ulpfec", 0, 0, 0, 0);
 
 static const uint32 kSsrcs1[] = {1};
 static const uint32 kRtxSsrcs1[] = {4};
+
+void VerifyCodecHasDefaultFeedbackParams(const cricket::VideoCodec& codec) {
+  EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
+      cricket::kRtcpFbParamNack, cricket::kParamValueEmpty)));
+  EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
+      cricket::kRtcpFbParamNack, cricket::kRtcpFbNackParamPli)));
+  EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
+      cricket::kRtcpFbParamRemb, cricket::kParamValueEmpty)));
+  EXPECT_TRUE(codec.HasFeedbackParam(cricket::FeedbackParam(
+      cricket::kRtcpFbParamCcm, cricket::kRtcpFbCcmParamFir)));
+}
+
 }  // namespace
 
 namespace cricket {
@@ -744,7 +756,9 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_RembOnOff) {
   FAIL() << "Not implemented.";  // TODO(pbos): Implement.
 }
 
-TEST_F(WebRtcVideoChannel2Test, NackIsEnabled) {
+TEST_F(WebRtcVideoChannel2Test, NackIsEnabledByDefault) {
+  VerifyCodecHasDefaultFeedbackParams(default_codec_);
+
   EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs()));
   EXPECT_TRUE(channel_->SetSend(true));
 
@@ -763,6 +777,23 @@ TEST_F(WebRtcVideoChannel2Test, NackIsEnabled) {
             recv_stream->GetConfig().rtp.nack.rtp_history_ms);
 }
 
+TEST_F(WebRtcVideoChannel2Test, NackCanBeDisabled) {
+  std::vector<VideoCodec> codecs;
+  codecs.push_back(kVp8Codec);
+
+  // Send side.
+  ASSERT_TRUE(channel_->SetSendCodecs(codecs));
+  FakeVideoSendStream* send_stream =
+      AddSendStream(cricket::StreamParams::CreateLegacy(1));
+  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);
+}
+
 TEST_F(WebRtcVideoChannel2Test, DISABLED_VideoProtectionInterop) {
   FAIL() << "Not implemented.";  // TODO(pbos): Implement.
 }