Refactor MediaOptimization protection methods.

Makes MediaOptimization::EnableProtectionMethod significantly less
confusing. Also removing some dead methods in VideoSender.

BUG=
R=mflodman@webrtc.org
TBR=stefan@webrtc.org

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

Cr-Commit-Position: refs/heads/master@{#8693}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8693 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/video_coding/main/interface/video_coding.h

@@ -528,38 +528,6 @@ public:
 
     // Robustness APIs
 
-    // Set the sender RTX/NACK mode.
-    // Input:
-    //      - mode       : the selected NACK mode.
-    //
-    // Return value      : VCM_OK, on success;
-    //                     < 0, on error.
-    virtual int SetSenderNackMode(SenderNackMode mode) = 0;
-
-    // Set the sender reference picture selection (RPS) mode.
-    // Input:
-    //      - enable     : true or false, for enable and disable, respectively.
-    //
-    // Return value      : VCM_OK, on success;
-    //                     < 0, on error.
-    virtual int SetSenderReferenceSelection(bool enable) = 0;
-
-    // Set the sender forward error correction (FEC) mode.
-    // Input:
-    //      - enable     : true or false, for enable and disable, respectively.
-    //
-    // Return value      : VCM_OK, on success;
-    //                     < 0, on error.
-    virtual int SetSenderFEC(bool enable) = 0;
-
-    // Set the key frame period, or disable periodic key frames (I-frames).
-    // Input:
-    //      - periodMs   : period in ms; <= 0 to disable periodic key frames.
-    //
-    // Return value      : VCM_OK, on success;
-    //                     < 0, on error.
-    virtual int SetSenderKeyFramePeriod(int periodMs) = 0;
-
     // Set the receiver robustness mode. The mode decides how the receiver
     // responds to losses in the stream. The type of counter-measure (soft or
     // hard NACK, dual decoder, RPS, etc.) is selected through the

webrtc/modules/video_coding/main/interface/video_coding_defines.h

@@ -46,6 +46,7 @@ namespace webrtc {
 enum { kDefaultStartBitrateKbps = 300 };
 
 enum VCMVideoProtection {
+  kProtectionNone,
   kProtectionNack,                // Both send-side and receive-side
   kProtectionNackSender,          // Send-side only
   kProtectionNackReceiver,        // Receive-side only
@@ -53,7 +54,6 @@ enum VCMVideoProtection {
   kProtectionNackFEC,
   kProtectionKeyOnLoss,
   kProtectionKeyOnKeyLoss,
-  kProtectionPeriodicKeyFrames
 };
 
 enum VCMTemporalDecimation {

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

@@ -670,61 +670,30 @@ VCMLossProtectionLogic::~VCMLossProtectionLogic()
     Release();
 }
 
-bool
+void VCMLossProtectionLogic::SetMethod(
-VCMLossProtectionLogic::SetMethod(enum VCMProtectionMethodEnum newMethodType)
+    enum VCMProtectionMethodEnum newMethodType) {
-{
+  if (_selectedMethod != nullptr) {
-    if (_selectedMethod != NULL)
-    {
     if (_selectedMethod->Type() == newMethodType)
-        {
+      return;
-            // Nothing to update
+    // Remove old method.
-            return false;
-        }
-        // New method - delete existing one
     delete _selectedMethod;
   }
-    VCMProtectionMethod *newMethod = NULL;
-    switch (newMethodType)
-    {
-        case kNack:
-        {
-            newMethod = new VCMNackMethod();
-            break;
-        }
-        case kFec:
-        {
-            newMethod  = new VCMFecMethod();
-            break;
-        }
-        case kNackFec:
-        {
-            // Default to always having NACK enabled for the hybrid mode.
-            newMethod =  new VCMNackFecMethod(kLowRttNackMs, -1);
-            break;
-        }
-        default:
-        {
-          return false;
-          break;
-        }
 
+  switch(newMethodType) {
+    case kNack:
+      _selectedMethod = new VCMNackMethod();
+      break;
+    case kFec:
+      _selectedMethod = new VCMFecMethod();
+      break;
+    case kNackFec:
+      _selectedMethod = new VCMNackFecMethod(kLowRttNackMs, -1);
+      break;
+    case kNone:
+      _selectedMethod = nullptr;
+      break;
   }
-    _selectedMethod = newMethod;
+  UpdateMethod();
-    return true;
-}
-bool
-VCMLossProtectionLogic::RemoveMethod(enum VCMProtectionMethodEnum method)
-{
-    if (_selectedMethod == NULL)
-    {
-        return false;
-    }
-    else if (_selectedMethod->Type() == method)
-    {
-        delete _selectedMethod;
-        _selectedMethod = NULL;
-    }
-    return true;
 }
 
 float
@@ -924,10 +893,8 @@ VCMLossProtectionLogic::SelectedMethod() const
     return _selectedMethod;
 }
 
-VCMProtectionMethodEnum
+VCMProtectionMethodEnum VCMLossProtectionLogic::SelectedType() const {
-VCMLossProtectionLogic::SelectedType() const
+  return _selectedMethod == nullptr ? kNone : _selectedMethod->Type();
-{
-    return _selectedMethod->Type();
 }
 
 void

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

@@ -245,15 +245,7 @@ public:
     // Input:
     //        - newMethodType    : New requested protection method type. If one
     //                           is already set, it will be deleted and replaced
-    // Return value:             Returns true on update
+    void SetMethod(VCMProtectionMethodEnum newMethodType);
-    bool SetMethod(VCMProtectionMethodEnum newMethodType);
-
-    // Remove requested protection method
-    // Input:
-    //        - method          : method to be removed (if currently selected)
-    //
-    // Return value:             Returns true on update
-    bool RemoveMethod(VCMProtectionMethodEnum method);
 
     // Return required bit rate per selected protectin method
     float RequiredBitRate() const;

webrtc/modules/video_coding/main/source/media_optimization.cc

@@ -321,15 +321,11 @@ uint32_t MediaOptimization::SetTargetRates(
 void MediaOptimization::EnableProtectionMethod(bool enable,
                                                VCMProtectionMethodEnum method) {
   CriticalSectionScoped lock(crit_sect_.get());
-  bool updated = false;
+  if (!enable && loss_prot_logic_->SelectedType() != method)
-  if (enable) {
+    return;
-    updated = loss_prot_logic_->SetMethod(method);
+  if (!enable)
-  } else {
+    method = kNone;
-    loss_prot_logic_->RemoveMethod(method);
+  loss_prot_logic_->SetMethod(method);
-  }
-  if (updated) {
-    loss_prot_logic_->UpdateMethod();
-  }
 }
 
 uint32_t MediaOptimization::InputFrameRate() {

webrtc/modules/video_coding/main/source/video_coding_impl.cc

@@ -173,13 +173,8 @@ class VideoCodingModuleImpl : public VideoCodingModule {
 
   int32_t SetVideoProtection(VCMVideoProtection videoProtection,
                              bool enable) override {
-    int32_t sender_return =
+    sender_->SetVideoProtection(enable, videoProtection);
-        sender_->SetVideoProtection(videoProtection, enable);
+    return receiver_->SetVideoProtection(videoProtection, enable);
-    int32_t receiver_return =
-        receiver_->SetVideoProtection(videoProtection, enable);
-    if (sender_return == VCM_OK)
-      return receiver_return;
-    return sender_return;
   }
 
   int32_t AddVideoFrame(const I420VideoFrame& videoFrame,
@@ -201,22 +196,6 @@ class VideoCodingModuleImpl : public VideoCodingModule {
     return sender_->SentFrameCount(&frameCount);
   }
 
-  int SetSenderNackMode(SenderNackMode mode) override {
-    return sender_->SetSenderNackMode(mode);
-  }
-
-  int SetSenderReferenceSelection(bool enable) override {
-    return sender_->SetSenderReferenceSelection(enable);
-  }
-
-  int SetSenderFEC(bool enable) override {
-    return sender_->SetSenderFEC(enable);
-  }
-
-  int SetSenderKeyFramePeriod(int periodMs) override {
-    return sender_->SetSenderKeyFramePeriod(periodMs);
-  }
-
   int StartDebugRecording(const char* file_name_utf8) override {
     return sender_->StartDebugRecording(file_name_utf8);
   }

webrtc/modules/video_coding/main/source/video_coding_impl.h

@@ -101,7 +101,7 @@ class VideoSender {
   int32_t RegisterSendStatisticsCallback(VCMSendStatisticsCallback* sendStats);
   int32_t RegisterVideoQMCallback(VCMQMSettingsCallback* videoQMSettings);
   int32_t RegisterProtectionCallback(VCMProtectionCallback* protection);
-  int32_t SetVideoProtection(VCMVideoProtection videoProtection, bool enable);
+  void SetVideoProtection(bool enable, VCMVideoProtection videoProtection);
 
   int32_t AddVideoFrame(const I420VideoFrame& videoFrame,
                         const VideoContentMetrics* _contentMetrics,
@@ -110,11 +110,6 @@ class VideoSender {
   int32_t IntraFrameRequest(int stream_index);
   int32_t EnableFrameDropper(bool enable);
 
-  int SetSenderNackMode(SenderNackMode mode);
-  int SetSenderReferenceSelection(bool enable);
-  int SetSenderFEC(bool enable);
-  int SetSenderKeyFramePeriod(int periodMs);
-
   int StartDebugRecording(const char* file_name_utf8);
   void StopDebugRecording();
 

webrtc/modules/video_coding/main/source/video_receiver.cc

@@ -236,9 +236,12 @@ int32_t VideoReceiver::SetVideoProtection(VCMVideoProtection videoProtection,
     }
     case kProtectionNackSender:
     case kProtectionFEC:
-    case kProtectionPeriodicKeyFrames:
       // Ignore encoder modes.
       return VCM_OK;
+    case kProtectionNone:
+      // TODO(pbos): Implement like sender and remove enable parameter. Ignored
+      // for now.
+      break;
   }
   return VCM_OK;
 }

webrtc/modules/video_coding/main/source/video_sender.cc

@@ -330,43 +330,29 @@ int32_t VideoSender::RegisterProtectionCallback(
 }
 
 // Enable or disable a video protection method.
-// Note: This API should be deprecated, as it does not offer a distinction
+void VideoSender::SetVideoProtection(bool enable,
-// between the protection method and decoding with or without errors. If such a
+                                     VCMVideoProtection videoProtection) {
-// behavior is desired, use the following API: SetReceiverRobustnessMode.
-int32_t VideoSender::SetVideoProtection(VCMVideoProtection videoProtection,
-                                        bool enable) {
-  switch (videoProtection) {
-    case kProtectionNack:
-    case kProtectionNackSender: {
   CriticalSectionScoped cs(_sendCritSect);
+  switch (videoProtection) {
+    case kProtectionNone:
+      _mediaOpt.EnableProtectionMethod(enable, media_optimization::kNone);
+      break;
+    case kProtectionNack:
+    case kProtectionNackSender:
       _mediaOpt.EnableProtectionMethod(enable, media_optimization::kNack);
       break;
-    }
+    case kProtectionNackFEC:
-
-    case kProtectionNackFEC: {
-      CriticalSectionScoped cs(_sendCritSect);
       _mediaOpt.EnableProtectionMethod(enable, media_optimization::kNackFec);
       break;
-    }
+    case kProtectionFEC:
-
-    case kProtectionFEC: {
-      CriticalSectionScoped cs(_sendCritSect);
       _mediaOpt.EnableProtectionMethod(enable, media_optimization::kFec);
       break;
-    }
-
-    case kProtectionPeriodicKeyFrames: {
-      CriticalSectionScoped cs(_sendCritSect);
-      return _codecDataBase.SetPeriodicKeyFrames(enable) ? 0 : -1;
-      break;
-    }
     case kProtectionNackReceiver:
     case kProtectionKeyOnLoss:
     case kProtectionKeyOnKeyLoss:
-      // Ignore decoder modes.
+      // Ignore receiver modes.
-      return VCM_OK;
+      return;
   }
-  return VCM_OK;
 }
 // Add one raw video frame to the encoder, blocking.
 int32_t VideoSender::AddVideoFrame(const I420VideoFrame& videoFrame,
@@ -429,33 +415,6 @@ int32_t VideoSender::EnableFrameDropper(bool enable) {
   return VCM_OK;
 }
 
-int VideoSender::SetSenderNackMode(SenderNackMode mode) {
-  switch (mode) {
-    case VideoCodingModule::kNackNone:
-      _mediaOpt.EnableProtectionMethod(false, media_optimization::kNack);
-      break;
-    case VideoCodingModule::kNackAll:
-      _mediaOpt.EnableProtectionMethod(true, media_optimization::kNack);
-      break;
-    case VideoCodingModule::kNackSelective:
-      return VCM_NOT_IMPLEMENTED;
-  }
-  return VCM_OK;
-}
-
-int VideoSender::SetSenderReferenceSelection(bool enable) {
-  return VCM_NOT_IMPLEMENTED;
-}
-
-int VideoSender::SetSenderFEC(bool enable) {
-  _mediaOpt.EnableProtectionMethod(enable, media_optimization::kFec);
-  return VCM_OK;
-}
-
-int VideoSender::SetSenderKeyFramePeriod(int periodMs) {
-  return VCM_NOT_IMPLEMENTED;
-}
-
 int VideoSender::StartDebugRecording(const char* file_name_utf8) {
   return recorder_->Start(file_name_utf8);
 }