Delete nack_enabled flag in encoder configuration.

This is a followup to cl https://webrtc-review.googlesource.com/71380,
which reworked the way encoder resilience is done, and made the
nack_enabled flag unused.

Bug: webrtc:8830
Change-Id: I3de2508c97bc71e01c8f2232d16cd1f33e57fe4a
Reviewed-on: https://webrtc-review.googlesource.com/69986
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23080}

modules/video_coding/include/video_codec_initializer.h

@@ -34,10 +34,20 @@ class VideoCodecInitializer {
   static bool SetupCodec(
       const VideoEncoderConfig& config,
       const std::vector<VideoStream>& streams,
-      bool nack_enabled,
       VideoCodec* codec,
       std::unique_ptr<VideoBitrateAllocator>* bitrate_allocator);
 
+  // TODO(nisse): Deprecated, delete as soon as downstream projects are
+  // updated.
+  static bool SetupCodec(
+      const VideoEncoderConfig& config,
+      const std::vector<VideoStream>& streams,
+      bool /* nack_enabled */,
+      VideoCodec* codec,
+      std::unique_ptr<VideoBitrateAllocator>* bitrate_allocator) {
+    return SetupCodec(config, streams, codec, bitrate_allocator);
+  }
+
   // Create a bitrate allocator for the specified codec.
   static std::unique_ptr<VideoBitrateAllocator> CreateBitrateAllocator(
       const VideoCodec& codec);
@@ -45,8 +55,7 @@ class VideoCodecInitializer {
  private:
   static VideoCodec VideoEncoderConfigToVideoCodec(
       const VideoEncoderConfig& config,
-      const std::vector<VideoStream>& streams,
+      const std::vector<VideoStream>& streams);
-      bool nack_enabled);
 };
 
 }  // namespace webrtc

modules/video_coding/video_codec_initializer.cc

@@ -29,13 +29,12 @@ namespace webrtc {
 bool VideoCodecInitializer::SetupCodec(
     const VideoEncoderConfig& config,
     const std::vector<VideoStream>& streams,
-    bool nack_enabled,
     VideoCodec* codec,
     std::unique_ptr<VideoBitrateAllocator>* bitrate_allocator) {
   if (config.codec_type == kVideoCodecMultiplex) {
     VideoEncoderConfig associated_config = config.Copy();
     associated_config.codec_type = kVideoCodecVP9;
-    if (!SetupCodec(associated_config, streams, nack_enabled, codec,
+    if (!SetupCodec(associated_config, streams, codec,
                     bitrate_allocator)) {
       RTC_LOG(LS_ERROR) << "Failed to create stereo encoder configuration.";
       return false;
@@ -45,7 +44,7 @@ bool VideoCodecInitializer::SetupCodec(
   }
 
   *codec =
-      VideoEncoderConfigToVideoCodec(config, streams, nack_enabled);
+      VideoEncoderConfigToVideoCodec(config, streams);
   *bitrate_allocator = CreateBitrateAllocator(*codec);
 
   return true;
@@ -73,8 +72,7 @@ VideoCodecInitializer::CreateBitrateAllocator(const VideoCodec& codec) {
 // TODO(sprang): Split this up and separate the codec specific parts.
 VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec(
     const VideoEncoderConfig& config,
-    const std::vector<VideoStream>& streams,
+    const std::vector<VideoStream>& streams) {
-    bool nack_enabled) {
   static const int kEncoderMinBitrateKbps = 30;
   RTC_DCHECK(!streams.empty());
   RTC_DCHECK_GE(config.min_transmit_bitrate_bps, 0);

modules/video_coding/video_codec_initializer_unittest.cc

@@ -39,7 +39,7 @@ static const uint32_t kHighScreenshareTl1Bps = 1200000;
 // TODO(sprang): Extend coverage to handle the rest of the codec initializer.
 class VideoCodecInitializerTest : public ::testing::Test {
  public:
-  VideoCodecInitializerTest() : nack_enabled_(false) {}
+  VideoCodecInitializerTest() {}
   virtual ~VideoCodecInitializerTest() {}
 
  protected:
@@ -76,7 +76,7 @@ class VideoCodecInitializerTest : public ::testing::Test {
     codec_out_ = VideoCodec();
     bitrate_allocator_out_.reset();
     temporal_layers_.clear();
-    if (!VideoCodecInitializer::SetupCodec(config_, streams_, nack_enabled_,
+    if (!VideoCodecInitializer::SetupCodec(config_, streams_,
                                            &codec_out_,
                                            &bitrate_allocator_out_)) {
       return false;
@@ -122,7 +122,6 @@ class VideoCodecInitializerTest : public ::testing::Test {
   // Input settings.
   VideoEncoderConfig config_;
   std::vector<VideoStream> streams_;
-  bool nack_enabled_;
 
   // Output.
   VideoCodec codec_out_;

video/test/mock_video_stream_encoder.h

@@ -28,14 +28,13 @@ class MockVideoStreamEncoder : public VideoStreamEncoderInterface {
   MOCK_METHOD1(SetBitrateObserver, void(VideoBitrateAllocationObserver*));
   MOCK_METHOD0(Stop, void());
 
-  MOCK_METHOD3(MockedConfigureEncoder,
+  MOCK_METHOD2(MockedConfigureEncoder,
-               void(const VideoEncoderConfig&, size_t, bool));
+               void(const VideoEncoderConfig&, size_t));
   // gtest generates implicit copy which is not allowed on VideoEncoderConfig,
   // so we can't mock ConfigureEncoder directly.
   void ConfigureEncoder(VideoEncoderConfig config,
-                        size_t max_data_payload_length,
+                        size_t max_data_payload_length) {
-                        bool nack_enabled) {
+    MockedConfigureEncoder(config, max_data_payload_length);
-    MockedConfigureEncoder(config, max_data_payload_length, nack_enabled);
   }
 };
 

video/video_send_stream.cc

@@ -169,8 +169,7 @@ void VideoSendStream::ReconfigureVideoEncoder(VideoEncoderConfig config) {
   RTC_DCHECK(content_type_ == config.content_type);
   video_stream_encoder_->ConfigureEncoder(
       std::move(config),
-      config_.rtp.max_packet_size - CalculateMaxHeaderSize(config_.rtp),
+      config_.rtp.max_packet_size - CalculateMaxHeaderSize(config_.rtp));
-      config_.rtp.nack.rtp_history_ms > 0);
 }
 
 VideoSendStream::Stats VideoSendStream::GetStats() {

video/video_stream_encoder.cc

@@ -344,7 +344,6 @@ VideoStreamEncoder::VideoStreamEncoder(
       pending_encoder_creation_(false),
       encoder_start_bitrate_bps_(0),
       max_data_payload_length_(0),
-      nack_enabled_(false),
       last_observed_bitrate_bps_(0),
       encoder_paused_and_dropped_frame_(false),
       clock_(Clock::GetRealTimeClock()),
@@ -448,34 +447,30 @@ void VideoStreamEncoder::SetStartBitrate(int start_bitrate_bps) {
 }
 
 void VideoStreamEncoder::ConfigureEncoder(VideoEncoderConfig config,
-                                          size_t max_data_payload_length,
+                                          size_t max_data_payload_length) {
-                                          bool nack_enabled) {
   // TODO(srte): This struct should be replaced by a lambda with move capture
   // when C++14 lambda is allowed.
   struct ConfigureEncoderTask {
     void operator()() {
       encoder->ConfigureEncoderOnTaskQueue(
-          std::move(config), max_data_payload_length, nack_enabled);
+          std::move(config), max_data_payload_length);
     }
     VideoStreamEncoder* encoder;
     VideoEncoderConfig config;
     size_t max_data_payload_length;
-    bool nack_enabled;
   };
   encoder_queue_.PostTask(ConfigureEncoderTask{
-      this, std::move(config), max_data_payload_length, nack_enabled});
+      this, std::move(config), max_data_payload_length});
 }
 
 void VideoStreamEncoder::ConfigureEncoderOnTaskQueue(
     VideoEncoderConfig config,
-    size_t max_data_payload_length,
+    size_t max_data_payload_length) {
-    bool nack_enabled) {
   RTC_DCHECK_RUN_ON(&encoder_queue_);
   RTC_DCHECK(sink_);
   RTC_LOG(LS_INFO) << "ConfigureEncoder requested.";
 
   max_data_payload_length_ = max_data_payload_length;
-  nack_enabled_ = nack_enabled;
   pending_encoder_creation_ =
       (!encoder_ || encoder_config_.video_format != config.video_format);
   encoder_config_ = std::move(config);
@@ -520,7 +515,7 @@ void VideoStreamEncoder::ReconfigureEncoder() {
 
   VideoCodec codec;
   if (!VideoCodecInitializer::SetupCodec(
-          encoder_config_, streams, nack_enabled_, &codec, &rate_allocator_)) {
+          encoder_config_, streams, &codec, &rate_allocator_)) {
     RTC_LOG(LS_ERROR) << "Failed to create encoder configuration.";
   }
 

video/video_stream_encoder.h

@@ -63,8 +63,7 @@ class VideoStreamEncoderInterface : public rtc::VideoSinkInterface<VideoFrame> {
   virtual void SetBitrateObserver(
       VideoBitrateAllocationObserver* bitrate_observer) = 0;
   virtual void ConfigureEncoder(VideoEncoderConfig config,
-                                size_t max_data_payload_length,
+                                size_t max_data_payload_length) = 0;
-                                bool nack_enabled) = 0;
   virtual void Stop() = 0;
 
  protected:
@@ -115,8 +114,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface,
       VideoBitrateAllocationObserver* bitrate_observer) override;
 
   void ConfigureEncoder(VideoEncoderConfig config,
-                        size_t max_data_payload_length,
+                        size_t max_data_payload_length) override;
-                        bool nack_enabled) override;
 
   // Permanently stop encoding. After this method has returned, it is
   // guaranteed that no encoded frames will be delivered to the sink.
@@ -156,8 +154,7 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface,
   };
 
   void ConfigureEncoderOnTaskQueue(VideoEncoderConfig config,
-                                   size_t max_data_payload_length,
+                                   size_t max_data_payload_length);
-                                   bool nack_enabled);
   void ReconfigureEncoder() RTC_RUN_ON(&encoder_queue_);
 
   void ConfigureQualityScaler();
@@ -276,7 +273,6 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface,
   int crop_height_ RTC_GUARDED_BY(&encoder_queue_);
   uint32_t encoder_start_bitrate_bps_ RTC_GUARDED_BY(&encoder_queue_);
   size_t max_data_payload_length_ RTC_GUARDED_BY(&encoder_queue_);
-  bool nack_enabled_ RTC_GUARDED_BY(&encoder_queue_);
   uint32_t last_observed_bitrate_bps_ RTC_GUARDED_BY(&encoder_queue_);
   bool encoder_paused_and_dropped_frame_ RTC_GUARDED_BY(&encoder_queue_);
   Clock* const clock_;

video/video_stream_encoder_unittest.cc

@@ -301,11 +301,10 @@ class VideoStreamEncoderTest : public ::testing::Test {
     max_framerate_ = streams[0].max_framerate;
     fake_clock_.SetTimeMicros(1234);
 
-    ConfigureEncoder(std::move(video_encoder_config), true /* nack_enabled */);
+    ConfigureEncoder(std::move(video_encoder_config));
   }
 
-  void ConfigureEncoder(VideoEncoderConfig video_encoder_config,
+  void ConfigureEncoder(VideoEncoderConfig video_encoder_config) {
-                        bool nack_enabled) {
     if (video_stream_encoder_)
       video_stream_encoder_->Stop();
     video_stream_encoder_.reset(new VideoStreamEncoderUnderTest(
@@ -316,7 +315,7 @@ class VideoStreamEncoderTest : public ::testing::Test {
         VideoSendStream::DegradationPreference::kMaintainFramerate);
     video_stream_encoder_->SetStartBitrate(kTargetBitrateBps);
     video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                            kMaxPayloadLength, nack_enabled);
+                                            kMaxPayloadLength);
     video_stream_encoder_->WaitUntilTaskQueueIsIdle();
   }
 
@@ -324,7 +323,6 @@ class VideoStreamEncoderTest : public ::testing::Test {
                     size_t num_streams,
                     size_t num_temporal_layers,
                     unsigned char num_spatial_layers,
-                    bool nack_enabled,
                     bool screenshare) {
     video_send_config_.rtp.payload_name = payload_name;
 
@@ -345,7 +343,7 @@ class VideoStreamEncoderTest : public ::testing::Test {
           new rtc::RefCountedObject<
               VideoEncoderConfig::Vp9EncoderSpecificSettings>(vp9_settings);
     }
-    ConfigureEncoder(std::move(video_encoder_config), nack_enabled);
+    ConfigureEncoder(std::move(video_encoder_config));
   }
 
   VideoFrame CreateFrame(int64_t ntp_time_ms,
@@ -800,8 +798,7 @@ TEST_F(VideoStreamEncoderTest,
   test::FillEncoderConfiguration(kVideoCodecVP8, 1, &video_encoder_config);
   video_encoder_config.min_transmit_bitrate_bps = 9999;
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength,
+                                          kMaxPayloadLength);
-                                          true /* nack_enabled */);
 
   // Capture a frame and wait for it to synchronize with the encoder thread.
   video_source_.IncomingCapturedFrame(CreateFrame(2, nullptr));
@@ -1278,8 +1275,7 @@ TEST_F(VideoStreamEncoderTest,
   // Make format different, to force recreation of encoder.
   video_encoder_config.video_format.parameters["foo"] = "foo";
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength,
+                                          kMaxPayloadLength);
-                                          true /* nack_enabled */);
 
   video_source_.IncomingCapturedFrame(CreateFrame(4, kWidth, kHeight));
   WaitForEncodedFrame(4);
@@ -2136,7 +2132,7 @@ TEST_F(VideoStreamEncoderTest, OveruseDetectorUpdatedOnReconfigureAndAdaption) {
   video_encoder_config.video_stream_factory =
       new rtc::RefCountedObject<VideoStreamFactory>(1, kFramerate);
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength, false);
+                                          kMaxPayloadLength);
   video_stream_encoder_->WaitUntilTaskQueueIsIdle();
 
   // Detector should be updated with fps limit from codec config.
@@ -2189,7 +2185,7 @@ TEST_F(VideoStreamEncoderTest,
       new rtc::RefCountedObject<VideoStreamFactory>(1, kLowFramerate);
   source.IncomingCapturedFrame(CreateFrame(1, kFrameWidth, kFrameHeight));
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength, false);
+                                          kMaxPayloadLength);
   video_stream_encoder_->WaitUntilTaskQueueIsIdle();
 
   EXPECT_EQ(
@@ -2212,7 +2208,7 @@ TEST_F(VideoStreamEncoderTest,
       new rtc::RefCountedObject<VideoStreamFactory>(1, kHighFramerate);
   source.IncomingCapturedFrame(CreateFrame(1, kFrameWidth, kFrameHeight));
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength, false);
+                                          kMaxPayloadLength);
   video_stream_encoder_->WaitUntilTaskQueueIsIdle();
 
   EXPECT_EQ(
@@ -2252,7 +2248,7 @@ TEST_F(VideoStreamEncoderTest,
       new rtc::RefCountedObject<VideoStreamFactory>(1, kFramerate);
   source.IncomingCapturedFrame(CreateFrame(1, kFrameWidth, kFrameHeight));
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength, false);
+                                          kMaxPayloadLength);
   video_stream_encoder_->WaitUntilTaskQueueIsIdle();
 
   EXPECT_EQ(
@@ -2360,8 +2356,7 @@ TEST_F(VideoStreamEncoderTest, InitialFrameDropOffWhenEncoderDisabledScaling) {
   // Make format different, to force recreation of encoder.
   video_encoder_config.video_format.parameters["foo"] = "foo";
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength,
+                                          kMaxPayloadLength);
-                                          true /* nack_enabled */);
   video_stream_encoder_->OnBitrateUpdated(kLowTargetBitrateBps, 0, 0);
 
   // Force quality scaler reconfiguration by resetting the source.
@@ -2441,7 +2436,7 @@ TEST_F(VideoStreamEncoderTest,
 TEST_F(VideoStreamEncoderTest, FailingInitEncodeDoesntCauseCrash) {
   fake_encoder_.ForceInitEncodeFailure(true);
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
-  ResetEncoder("VP8", 2, 1, 1, true, false);
+  ResetEncoder("VP8", 2, 1, 1, false);
   const int kFrameWidth = 1280;
   const int kFrameHeight = 720;
   video_source_.IncomingCapturedFrame(
@@ -2587,7 +2582,7 @@ TEST_F(VideoStreamEncoderTest, DoesntAdaptDownPastMinFramerate) {
 
   // Reconfigure encoder with two temporal layers and screensharing, which will
   // disable frame dropping and make testing easier.
-  ResetEncoder("VP8", 1, 2, 1, true, true);
+  ResetEncoder("VP8", 1, 2, 1, true);
 
   video_stream_encoder_->OnBitrateUpdated(kTargetBitrateBps, 0, 0);
   video_stream_encoder_->SetSource(
@@ -3053,7 +3048,7 @@ TEST_F(VideoStreamEncoderTest, AcceptsFullHdAdaptedDownSimulcastFrames) {
   video_encoder_config.video_stream_factory =
       new rtc::RefCountedObject<CroppingVideoStreamFactory>(1, kFramerate);
   video_stream_encoder_->ConfigureEncoder(std::move(video_encoder_config),
-                                          kMaxPayloadLength, false);
+                                          kMaxPayloadLength);
   video_stream_encoder_->WaitUntilTaskQueueIsIdle();
 
   video_source_.set_adaptation_enabled(true);