Default to always NACKing residual losses when having both FEC and NACK.

BUG=
TEST=

Review URL: http://webrtc-codereview.appspot.com/296002

git-svn-id: http://webrtc.googlecode.com/svn/trunk@1047 4adac7df-926f-26a2-2b94-8c16560cd09d

src/modules/video_coding/main/source/jitter_buffer.cc

@@ -16,7 +16,6 @@
 #include "jitter_buffer.h"
 #include "jitter_buffer_common.h"
 #include "jitter_estimator.h"
-#include "media_optimization.h" // hybrid NACK/FEC thresholds.
 #include "packet.h"
 
 #include "event.h"
@@ -86,6 +85,8 @@ VCMJitterBuffer::VCMJitterBuffer(WebRtc_Word32 vcmId, WebRtc_Word32 receiverId,
     _jitterEstimate(vcmId, receiverId),
     _rttMs(0),
     _nackMode(kNoNack),
+    _lowRttNackThresholdMs(-1),
+    _highRttNackThresholdMs(-1),
     _NACKSeqNum(),
     _NACKSeqNumLength(0),
     _waitingForKeyFrame(false),
@@ -870,8 +871,10 @@ VCMJitterBuffer::GetEstimatedJitterMsInternal()
     WebRtc_UWord32 estimate = VCMJitterEstimator::OPERATING_SYSTEM_JITTER;
 
     // Compute RTT multiplier for estimation
+    // _lowRttNackThresholdMs == -1 means no FEC.
     double rttMult = 1.0f;
-    if (_nackMode == kNackHybrid && _rttMs > kLowRttNackMs)
+    if (_nackMode == kNackHybrid && (_lowRttNackThresholdMs >= 0 &&
+        static_cast<int>(_rttMs) > _lowRttNackThresholdMs))
     {
         // from here we count on FEC
         rttMult = 0.0f;
@@ -1775,10 +1778,18 @@ VCMJitterBuffer::GetNackMode() const
 
 // Set NACK mode
 void
-VCMJitterBuffer::SetNackMode(VCMNackMode mode)
+VCMJitterBuffer::SetNackMode(VCMNackMode mode,
+                             int lowRttNackThresholdMs,
+                             int highRttNackThresholdMs)
 {
     CriticalSectionScoped cs(_critSect);
     _nackMode = mode;
+    assert(lowRttNackThresholdMs >= -1 && highRttNackThresholdMs >= -1);
+    assert(highRttNackThresholdMs == -1 ||
+           lowRttNackThresholdMs <= highRttNackThresholdMs);
+    assert(lowRttNackThresholdMs > -1 || highRttNackThresholdMs == -1);
+    _lowRttNackThresholdMs = lowRttNackThresholdMs;
+    _highRttNackThresholdMs = highRttNackThresholdMs;
     if (_nackMode == kNoNack)
     {
         _jitterEstimate.ResetNackCount();
@@ -2037,7 +2048,8 @@ VCMJitterBuffer::WaitForNack()
      }
      // else: hybrid mode, evaluate
      // RTT high, don't wait
-     if (_rttMs >= kHighRttNackMs)
+     if (_highRttNackThresholdMs >= 0 &&
+         _rttMs >= static_cast<unsigned int>(_highRttNackThresholdMs))
      {
          return false;
      }
@@ -2045,5 +2057,4 @@ VCMJitterBuffer::WaitForNack()
      return true;
 }
 
-
-}
+}  // namespace webrtc

src/modules/video_coding/main/source/jitter_buffer.h

@@ -121,7 +121,15 @@ public:
     void UpdateRtt(WebRtc_UWord32 rttMs);
 
     // NACK
-    void SetNackMode(VCMNackMode mode); // Enable/disable nack
+    // Set the NACK mode. "highRttNackThreshold" is an RTT threshold in ms above
+    // which NACK will be disabled if the NACK mode is "kNackHybrid",
+    // -1 meaning that NACK is always enabled in the Hybrid mode.
+    // "lowRttNackThreshold" is an RTT threshold in ms below which we expect to
+    // rely on NACK only, and therefore are using larger buffers to have time to
+    // wait for retransmissions.
+    void SetNackMode(VCMNackMode mode,
+                     int lowRttNackThresholdMs,
+                     int highRttNackThresholdMs);
     VCMNackMode GetNackMode() const;    // Get nack mode
     // Get list of missing sequence numbers (size in number of elements)
     WebRtc_UWord16* GetNackList(WebRtc_UWord16& nackSize,
@@ -236,6 +244,8 @@ private:
 
     // NACK
     VCMNackMode             _nackMode;
+    int                     _lowRttNackThresholdMs;
+    int                     _highRttNackThresholdMs;
     // Holds the internal nack list (the missing sequence numbers)
     WebRtc_Word32           _NACKSeqNumInternal[kNackHistoryLength];
     WebRtc_UWord16          _NACKSeqNum[kNackHistoryLength];

src/modules/video_coding/main/source/media_opt_util.cc

@@ -49,10 +49,16 @@ VCMProtectionMethod::UpdateContentMetrics(const
     _qmRobustness->UpdateContent(contentMetrics);
 }
 
-VCMNackFecMethod::VCMNackFecMethod():
-VCMFecMethod()
-{
-    _type = kNackFec;
+VCMNackFecMethod::VCMNackFecMethod(int lowRttNackThresholdMs,
+                                   int highRttNackThresholdMs)
+    : VCMFecMethod(),
+      _lowRttNackMs(lowRttNackThresholdMs),
+      _highRttNackMs(highRttNackThresholdMs) {
+  assert(lowRttNackThresholdMs >= -1 && highRttNackThresholdMs >= -1);
+  assert(highRttNackThresholdMs == -1 ||
+         lowRttNackThresholdMs <= highRttNackThresholdMs);
+  assert(lowRttNackThresholdMs > -1 || highRttNackThresholdMs == -1);
+  _type = kNackFec;
 }
 
 VCMNackFecMethod::~VCMNackFecMethod()
@@ -60,13 +66,13 @@ VCMNackFecMethod::~VCMNackFecMethod()
     //
 }
 bool
-VCMNackFecMethod::ProtectionFactor(const
-                                   VCMProtectionParameters* parameters)
+VCMNackFecMethod::ProtectionFactor(const VCMProtectionParameters* parameters)
 {
     // Hybrid Nack FEC has three operational modes:
     // 1. Low RTT (below kLowRttNackMs) - Nack only: Set FEC rate
-    //    (_protectionFactorD) to zero.
-    // 2. High RTT (above kHighRttNackMs) - FEC Only: Keep FEC factors.
+    //    (_protectionFactorD) to zero. -1 means no FEC.
+    // 2. High RTT (above _highRttNackMs) - FEC Only: Keep FEC factors.
+    //    -1 means always allow NACK.
     // 3. Medium RTT values - Hybrid mode: We will only nack the
     //    residual following the decoding of the FEC (refer to JB logic). FEC
     //    delta protection factor will be adjusted based on the RTT.
@@ -76,7 +82,7 @@ VCMNackFecMethod::ProtectionFactor(const
 
     // Compute the protection factors
     VCMFecMethod::ProtectionFactor(parameters);
-    if (parameters->rtt < kLowRttNackMs)
+    if (_lowRttNackMs == -1 || parameters->rtt < _lowRttNackMs)
     {
         _protectionFactorD = 0;
         VCMFecMethod::UpdateProtectionFactorD(_protectionFactorD);
@@ -84,7 +90,7 @@ VCMNackFecMethod::ProtectionFactor(const
 
     // When in Hybrid mode (RTT range), adjust FEC rates based on the
     // RTT (NACK effectiveness) - adjustment factor is in the range [0,1].
-    else if (parameters->rtt < kHighRttNackMs)
+    else if (_highRttNackMs == -1 || parameters->rtt < _highRttNackMs)
     {
         // TODO(mikhal): Disabling adjustment temporarily.
         // WebRtc_UWord16 rttIndex = (WebRtc_UWord16) parameters->rtt;
@@ -103,8 +109,7 @@ VCMNackFecMethod::ProtectionFactor(const
 }
 
 bool
-VCMNackFecMethod::EffectivePacketLoss(const
-                                      VCMProtectionParameters* parameters)
+VCMNackFecMethod::EffectivePacketLoss(const VCMProtectionParameters* parameters)
 {
     // Set the effective packet loss for encoder (based on FEC code).
     // Compute the effective packet loss and residual packet loss due to FEC.
@@ -125,7 +130,7 @@ VCMNackFecMethod::UpdateParameters(const VCMProtectionParameters* parameters)
     _efficiency = parameters->bitRate * fecRate * _corrFecCost;
 
     // Add NACK cost, when applicable
-    if (parameters->rtt < kHighRttNackMs)
+    if (_highRttNackMs == -1 || parameters->rtt < _highRttNackMs)
     {
         // nackCost  = (bitRate - nackCost) * (lossPr)
         _efficiency += parameters->bitRate * _residualPacketLossFec /
@@ -603,7 +608,8 @@ VCMLossProtectionLogic::SetMethod(enum VCMProtectionMethodEnum newMethodType)
         }
         case kNackFec:
         {
-            newMethod =  new VCMNackFecMethod();
+            // Default to always having NACK enabled for the hybrid mode.
+            newMethod =  new VCMNackFecMethod(kLowRttNackMs, -1);
             break;
         }
         default:

src/modules/video_coding/main/source/media_opt_util.h

@@ -49,7 +49,7 @@ struct VCMProtectionParameters
         residualPacketLossFec(0.0f), codecWidth(0), codecHeight(0)
         {}
 
-    WebRtc_UWord32      rtt;
+    int                 rtt;
     float               lossPr;
     float               bitRate;
     float               packetsPerFrame;
@@ -195,13 +195,18 @@ public:
 class VCMNackFecMethod : public VCMFecMethod
 {
 public:
-    VCMNackFecMethod();
+    VCMNackFecMethod(int lowRttNackThresholdMs,
+                     int highRttNackThresholdMs);
     virtual ~VCMNackFecMethod();
     virtual bool UpdateParameters(const VCMProtectionParameters* parameters);
     // Get the effective packet loss for ER
     bool EffectivePacketLoss(const VCMProtectionParameters* parameters);
     // Get the protection factors
     bool ProtectionFactor(const VCMProtectionParameters* parameters);
+
+private:
+    int _lowRttNackMs;
+    int _highRttNackMs;
 };
 
 class VCMLossProtectionLogic

src/modules/video_coding/main/source/receiver.cc

@@ -8,12 +8,14 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "video_coding.h"
-#include "trace.h"
+#include "receiver.h"
+
 #include "encoded_frame.h"
 #include "internal_defines.h"
-#include "receiver.h"
+#include "media_opt_util.h"
 #include "tick_time.h"
+#include "trace.h"
+#include "video_coding.h"
 
 #include <assert.h>
 
@@ -373,7 +375,8 @@ void
 VCMReceiver::SetNackMode(VCMNackMode nackMode)
 {
     CriticalSectionScoped cs(_critSect);
-    _jitterBuffer.SetNackMode(nackMode);
+    // Default to always having NACK enabled in hybrid mode.
+    _jitterBuffer.SetNackMode(nackMode, kLowRttNackMs, -1);
     if (!_master)
     {
         _state = kPassive; // The dual decoder defaults to passive

src/modules/video_coding/main/test/jitter_buffer_test.cc

@@ -8,20 +8,22 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "common_types.h"
-#include "jitter_buffer.h"
-#include "jitter_estimator.h"
-#include "inter_frame_delay.h"
-#include "packet.h"
-#include "tick_time.h"
-#include "../source/event.h"
-#include "frame_buffer.h"
-#include "jitter_estimate_test.h"
-#include "test_util.h"
-#include "test_macros.h"
 #include <stdio.h>
 #include <math.h>
 
+#include "common_types.h"
+#include "../source/event.h"
+#include "frame_buffer.h"
+#include "inter_frame_delay.h"
+#include "jitter_buffer.h"
+#include "jitter_estimate_test.h"
+#include "jitter_estimator.h"
+#include "media_opt_util.h"
+#include "packet.h"
+#include "test_util.h"
+#include "test_macros.h"
+#include "tick_time.h"
+
 using namespace webrtc;
 
 int CheckOutFrame(VCMEncodedFrame* frameOut, unsigned int size, bool startCode)
@@ -1568,7 +1570,7 @@ int JitterBufferTest(CmdArgs& args)
     //  ---------------------------------------------------------------------------------------------
     // | 3 | 4 | 5 | 6 | 7 | 9 | x | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | x | 21 |.....| 102 |
     //  ---------------------------------------------------------------------------------------------
-    jb.SetNackMode(kNackInfinite);
+    jb.SetNackMode(kNackInfinite, -1, -1);
 
     TEST(jb.GetNackMode() == kNackInfinite);
 
@@ -2039,7 +2041,7 @@ int JitterBufferTest(CmdArgs& args)
 
     // testing that empty packets do not clog the jitter buffer
     // Set hybrid mode
-    jb.SetNackMode(kNackHybrid);
+    jb.SetNackMode(kNackHybrid, kLowRttNackMs, -1);
     TEST(jb.GetNackMode() == kNackHybrid);
 
     int maxSize = 100;
@@ -2067,7 +2069,7 @@ int JitterBufferTest(CmdArgs& args)
     testFrame = jb.GetFrame(packet);
     TEST(frameIn != 0);
 
-    jb.SetNackMode(kNoNack);
+    jb.SetNackMode(kNoNack, -1, -1);
     jb.Flush();
 
     // Testing that 1 empty packet inserted last will not be set for decoding
@@ -2103,7 +2105,7 @@ int JitterBufferTest(CmdArgs& args)
     // Test incomplete NALU frames
 
     jb.Flush();
-    jb.SetNackMode(kNoNack);
+    jb.SetNackMode(kNoNack, -1, -1);
     seqNum ++;
     timeStamp += 33*90;
     int insertedLength=0;