Pass FecControllerOverride to Vp8FrameBufferControllerFactory::Create

Previously, FecControllerOverride was passed to
Vp8FrameBufferController::SetFecControllerOverride. Passing to
the factory is a more elegant way, since it's only used when
the controller is constructed.

TBR=kwiberg@webrtc.org

Bug: webrtc:10769
Change-Id: Iae599889e7ca9003e3200c2911239cbb763ee65a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/144380
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Elad Alon <eladalon@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28443}

api/BUILD.gn

@@ -649,6 +649,17 @@ if (rtc_include_tests) {
     ]
   }
 
+  rtc_source_set("mock_fec_controller_override") {
+    testonly = true
+    sources = [
+      "test/mock_fec_controller_override.h",
+    ]
+    deps = [
+      ":fec_controller_api",
+      "../test:test_support",
+    ]
+  }
+
   rtc_source_set("mock_frame_encryptor") {
     testonly = true
     sources = [

api/test/mock_fec_controller_override.h

@@ -0,0 +1,28 @@
+/*
+ *  Copyright 2019 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef API_TEST_MOCK_FEC_CONTROLLER_OVERRIDE_H_
+#define API_TEST_MOCK_FEC_CONTROLLER_OVERRIDE_H_
+
+#include "api/fec_controller_override.h"
+#include "test/gmock.h"
+
+namespace webrtc {
+
+class MockFecControllerOverride : public FecControllerOverride {
+ public:
+  ~MockFecControllerOverride() override = default;
+
+  MOCK_METHOD1(SetFecAllowed, void(bool fec_allowed));
+};
+
+}  // namespace webrtc
+
+#endif  // API_TEST_MOCK_FEC_CONTROLLER_OVERRIDE_H_

api/video_codecs/BUILD.gn

@@ -120,6 +120,7 @@ rtc_static_library("vp8_temporal_layers_factory") {
 
   deps = [
     ":video_codecs_api",
+    "../:fec_controller_api",
     "../../modules/video_coding:video_coding_utility",
     "../../modules/video_coding:webrtc_vp8_temporal_layers",
     "../../rtc_base:checks",

api/video_codecs/vp8_frame_buffer_controller.h

@@ -106,13 +106,6 @@ class Vp8FrameBufferController {
   // The limits are suggestion-only; the controller is allowed to exceed them.
   virtual void SetQpLimits(size_t stream_index, int min_qp, int max_qp) = 0;
 
-  // Set a FecControllerOverride, through which the bandwidth allocation
-  // decisions made by FecController may be overridden.
-  // TODO(bugs.webrtc.org/10769): Update downstream projects, then make
-  // this pure-virtual.
-  virtual void SetFecControllerOverride(
-      FecControllerOverride* fec_controller_override) {}
-
   // Number of streamed controlled by |this|.
   virtual size_t StreamCount() const = 0;
 
@@ -188,9 +181,24 @@ class Vp8FrameBufferControllerFactory {
   virtual std::unique_ptr<Vp8FrameBufferControllerFactory> Clone() const = 0;
 
   // Create a Vp8FrameBufferController instance.
+  // TODO(bugs.webrtc.org/10769): Update downstream projects, then remove
+  // version without |fec_controller_override| and make the other version
+  // pure-virtual.
+  // (In theory, if neither version is overridden, stack overflow would occur.
+  // In practice, all subclasses override at least one version, and following
+  // the update of downstream projects, only one pure-virtual version will
+  // remain.)
   virtual std::unique_ptr<Vp8FrameBufferController> Create(
       const VideoCodec& codec,
-      const VideoEncoder::Settings& settings) = 0;
+      const VideoEncoder::Settings& settings) {
+    return Create(codec, settings, nullptr);
+  }
+  virtual std::unique_ptr<Vp8FrameBufferController> Create(
+      const VideoCodec& codec,
+      const VideoEncoder::Settings& settings,
+      FecControllerOverride* fec_controller_override) {
+    return Create(codec, settings);
+  }
 };
 
 }  // namespace webrtc

api/video_codecs/vp8_temporal_layers.cc

@@ -18,7 +18,8 @@
 namespace webrtc {
 
 Vp8TemporalLayers::Vp8TemporalLayers(
-    std::vector<std::unique_ptr<Vp8FrameBufferController>>&& controllers)
+    std::vector<std::unique_ptr<Vp8FrameBufferController>>&& controllers,
+    FecControllerOverride* fec_controller_override)
     : controllers_(std::move(controllers)) {
   RTC_DCHECK(!controllers_.empty());
   RTC_DCHECK(absl::c_none_of(
@@ -26,6 +27,9 @@ Vp8TemporalLayers::Vp8TemporalLayers(
       [](const std::unique_ptr<Vp8FrameBufferController>& controller) {
         return controller.get() == nullptr;
       }));
+  if (fec_controller_override) {
+    fec_controller_override->SetFecAllowed(true);
+  }
 }
 
 void Vp8TemporalLayers::SetQpLimits(size_t stream_index,
@@ -35,11 +39,6 @@ void Vp8TemporalLayers::SetQpLimits(size_t stream_index,
   return controllers_[stream_index]->SetQpLimits(0, min_qp, max_qp);
 }
 
-void Vp8TemporalLayers::SetFecControllerOverride(
-    FecControllerOverride* fec_controller_override) {
-  // Ignore.
-}
-
 size_t Vp8TemporalLayers::StreamCount() const {
   return controllers_.size();
 }

api/video_codecs/vp8_temporal_layers.h

@@ -32,15 +32,13 @@ enum class Vp8TemporalLayersType { kFixedPattern, kBitrateDynamic };
 // realize a temporal layer structure.
 class Vp8TemporalLayers final : public Vp8FrameBufferController {
  public:
-  explicit Vp8TemporalLayers(
-      std::vector<std::unique_ptr<Vp8FrameBufferController>>&& controllers);
+  Vp8TemporalLayers(
+      std::vector<std::unique_ptr<Vp8FrameBufferController>>&& controllers,
+      FecControllerOverride* fec_controller_override);
   ~Vp8TemporalLayers() override = default;
 
   void SetQpLimits(size_t stream_index, int min_qp, int max_qp) override;
 
-  void SetFecControllerOverride(
-      FecControllerOverride* fec_controller_override) override;
-
   size_t StreamCount() const override;
 
   bool SupportsEncoderFrameDropping(size_t stream_index) const override;

api/video_codecs/vp8_temporal_layers_factory.cc

@@ -15,6 +15,7 @@
 #include <vector>
 
 #include "absl/memory/memory.h"
+#include "api/fec_controller_override.h"
 #include "modules/video_coding/codecs/vp8/default_temporal_layers.h"
 #include "modules/video_coding/codecs/vp8/screenshare_layers.h"
 #include "modules/video_coding/utility/simulcast_utility.h"
@@ -25,6 +26,13 @@ namespace webrtc {
 std::unique_ptr<Vp8FrameBufferController> Vp8TemporalLayersFactory::Create(
     const VideoCodec& codec,
     const VideoEncoder::Settings& settings) {
+  return Create(codec, settings, nullptr);
+}
+
+std::unique_ptr<Vp8FrameBufferController> Vp8TemporalLayersFactory::Create(
+    const VideoCodec& codec,
+    const VideoEncoder::Settings& settings,
+    FecControllerOverride* fec_controller_override) {
   std::vector<std::unique_ptr<Vp8FrameBufferController>> controllers;
   const int num_streams = SimulcastUtility::NumberOfSimulcastStreams(codec);
   RTC_DCHECK_GE(num_streams, 1);
@@ -44,7 +52,8 @@ std::unique_ptr<Vp8FrameBufferController> Vp8TemporalLayersFactory::Create(
     }
   }
 
-  return absl::make_unique<Vp8TemporalLayers>(std::move(controllers));
+  return absl::make_unique<Vp8TemporalLayers>(std::move(controllers),
+                                              fec_controller_override);
 }
 
 std::unique_ptr<Vp8FrameBufferControllerFactory>

api/video_codecs/vp8_temporal_layers_factory.h

@@ -23,9 +23,15 @@ class Vp8TemporalLayersFactory : public Vp8FrameBufferControllerFactory {
 
   std::unique_ptr<Vp8FrameBufferControllerFactory> Clone() const override;
 
+  // TODO(bugs.webrtc.org/10769): Update downstream projects, then remove.
   std::unique_ptr<Vp8FrameBufferController> Create(
       const VideoCodec& codec,
       const VideoEncoder::Settings& settings) override;
+
+  std::unique_ptr<Vp8FrameBufferController> Create(
+      const VideoCodec& codec,
+      const VideoEncoder::Settings& settings,
+      FecControllerOverride* fec_controller_override) override;
 };
 
 }  // namespace webrtc

modules/video_coding/BUILD.gn

@@ -848,6 +848,7 @@ if (rtc_include_tests) {
       "../../api:array_view",
       "../../api:create_simulcast_test_fixture_api",
       "../../api:fec_controller_api",
+      "../../api:mock_fec_controller_override",
       "../../api:mock_video_decoder",
       "../../api:mock_video_encoder",
       "../../api:scoped_refptr",

modules/video_coding/codecs/vp8/default_temporal_layers.cc

@@ -257,11 +257,6 @@ void DefaultTemporalLayers::SetQpLimits(size_t stream_index,
   // Ignore.
 }
 
-void DefaultTemporalLayers::SetFecControllerOverride(
-    FecControllerOverride* fec_controller_override) {
-  // Ignore.
-}
-
 size_t DefaultTemporalLayers::StreamCount() const {
   return 1;
 }

modules/video_coding/codecs/vp8/default_temporal_layers.h

@@ -22,7 +22,6 @@
 #include <vector>
 
 #include "absl/types/optional.h"
-#include "api/fec_controller_override.h"
 #include "api/video_codecs/vp8_frame_config.h"
 #include "api/video_codecs/vp8_temporal_layers.h"
 #include "modules/video_coding/codecs/vp8/include/temporal_layers_checker.h"
@@ -37,9 +36,6 @@ class DefaultTemporalLayers final : public Vp8FrameBufferController {
 
   void SetQpLimits(size_t stream_index, int min_qp, int max_qp) override;
 
-  void SetFecControllerOverride(
-      FecControllerOverride* fec_controller_override) override;
-
   size_t StreamCount() const override;
 
   bool SupportsEncoderFrameDropping(size_t stream_index) const override;

modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc

@@ -290,7 +290,8 @@ LibvpxVp8Encoder::LibvpxVp8Encoder(
       variable_framerate_experiment_(ParseVariableFramerateConfig(
           "WebRTC-VP8VariableFramerateScreenshare")),
       framerate_controller_(variable_framerate_experiment_.framerate_limit),
-      num_steady_state_frames_(0) {
+      num_steady_state_frames_(0),
+      fec_controller_override_(nullptr) {
   // TODO(eladalon/ilnik): These reservations might be wasting memory.
   // InitEncode() is resizing to the actual size, which might be smaller.
   raw_images_.reserve(kMaxSimulcastStreams);
@@ -452,8 +453,11 @@ void LibvpxVp8Encoder::SetStreamState(bool send_stream, int stream_idx) {
 
 void LibvpxVp8Encoder::SetFecControllerOverride(
     FecControllerOverride* fec_controller_override) {
-  RTC_DCHECK(fec_controller_override);
-  // TODO(bugs.webrtc.og/10769): Pass on to the frame buffer controller.
+  // TODO(bugs.webrtc.org/10769): Update downstream and remove ability to
+  // pass nullptr.
+  // RTC_DCHECK(fec_controller_override);
+  RTC_DCHECK(!fec_controller_override_);
+  fec_controller_override_ = fec_controller_override;
 }
 
 // TODO(eladalon): s/inst/codec_settings/g.
@@ -491,11 +495,12 @@ int LibvpxVp8Encoder::InitEncode(const VideoCodec* inst,
 
   RTC_DCHECK(!frame_buffer_controller_);
   if (frame_buffer_controller_factory_) {
-    frame_buffer_controller_ =
-        frame_buffer_controller_factory_->Create(*inst, settings);
+    frame_buffer_controller_ = frame_buffer_controller_factory_->Create(
+        *inst, settings, fec_controller_override_);
   } else {
     Vp8TemporalLayersFactory factory;
-    frame_buffer_controller_ = factory.Create(*inst, settings);
+    frame_buffer_controller_ =
+        factory.Create(*inst, settings, fec_controller_override_);
   }
   RTC_DCHECK(frame_buffer_controller_);
 

modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h

@@ -141,6 +141,8 @@ class LibvpxVp8Encoder : public VideoEncoder {
       std::string group_name);
   FramerateController framerate_controller_;
   int num_steady_state_frames_;
+
+  FecControllerOverride* fec_controller_override_;
 };
 
 }  // namespace webrtc

modules/video_coding/codecs/vp8/screenshare_layers.cc

@@ -87,11 +87,6 @@ void ScreenshareLayers::SetQpLimits(size_t stream_index,
   }
 }
 
-void ScreenshareLayers::SetFecControllerOverride(
-    FecControllerOverride* fec_controller_override) {
-  // Ignore.
-}
-
 size_t ScreenshareLayers::StreamCount() const {
   return 1;
 }

modules/video_coding/codecs/vp8/screenshare_layers.h

@@ -14,7 +14,6 @@
 #include <utility>
 #include <vector>
 
-#include "api/fec_controller_override.h"
 #include "api/video_codecs/vp8_frame_config.h"
 #include "api/video_codecs/vp8_temporal_layers.h"
 #include "modules/video_coding/codecs/vp8/include/temporal_layers_checker.h"
@@ -39,9 +38,6 @@ class ScreenshareLayers final : public Vp8FrameBufferController {
 
   void SetQpLimits(size_t stream_index, int min_qp, int max_qp) override;
 
-  void SetFecControllerOverride(
-      FecControllerOverride* fec_controller_override) override;
-
   size_t StreamCount() const override;
 
   bool SupportsEncoderFrameDropping(size_t stream_index) const override;

modules/video_coding/video_codec_initializer_unittest.cc

@@ -16,6 +16,7 @@
 
 #include "absl/types/optional.h"
 #include "api/scoped_refptr.h"
+#include "api/test/mock_fec_controller_override.h"
 #include "api/video/builtin_video_bitrate_allocator_factory.h"
 #include "api/video/video_bitrate_allocation.h"
 #include "api/video/video_bitrate_allocator.h"
@@ -26,6 +27,7 @@
 #include "modules/video_coding/codecs/vp9/include/vp9_globals.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/ref_counted_object.h"
+#include "test/gmock.h"
 #include "test/gtest.h"
 
 namespace webrtc {
@@ -100,7 +102,8 @@ class VideoCodecInitializerTest : public ::testing::Test {
       Vp8TemporalLayersFactory factory;
       const VideoEncoder::Settings settings(VideoEncoder::Capabilities(false),
                                             1, 1000);
-      frame_buffer_controller_ = factory.Create(codec_out_, settings);
+      frame_buffer_controller_ =
+          factory.Create(codec_out_, settings, &fec_controller_override_);
     }
     return true;
   }
@@ -130,6 +133,8 @@ class VideoCodecInitializerTest : public ::testing::Test {
     return stream;
   }
 
+  MockFecControllerOverride fec_controller_override_;
+
   // Input settings.
   VideoEncoderConfig config_;
   std::vector<VideoStream> streams_;

test/fake_vp8_encoder.cc

@@ -58,7 +58,8 @@ int32_t FakeVP8Encoder::InitEncode(const VideoCodec* config,
   }
 
   Vp8TemporalLayersFactory factory;
-  frame_buffer_controller_ = factory.Create(*config, settings);
+  frame_buffer_controller_ =
+      factory.Create(*config, settings, &fec_controller_override_);
 
   return WEBRTC_VIDEO_CODEC_OK;
 }

test/fake_vp8_encoder.h

@@ -16,6 +16,7 @@
 
 #include <memory>
 
+#include "api/fec_controller_override.h"
 #include "api/video/encoded_image.h"
 #include "api/video_codecs/video_codec.h"
 #include "api/video_codecs/video_encoder.h"
@@ -56,6 +57,16 @@ class FakeVP8Encoder : public FakeEncoder {
 
   SequenceChecker sequence_checker_;
 
+  class FakeFecControllerOverride : public FecControllerOverride {
+   public:
+    ~FakeFecControllerOverride() override = default;
+
+    void SetFecAllowed(bool fec_allowed) override {}
+  };
+
+  FakeFecControllerOverride fec_controller_override_
+      RTC_GUARDED_BY(sequence_checker_);
+
   std::unique_ptr<Vp8FrameBufferController> frame_buffer_controller_
       RTC_GUARDED_BY(sequence_checker_);
 };

video/BUILD.gn

@@ -549,6 +549,7 @@ if (rtc_include_tests) {
       "../api:fake_frame_decryptor",
       "../api:fake_frame_encryptor",
       "../api:libjingle_peerconnection_api",
+      "../api:mock_fec_controller_override",
       "../api:mock_frame_decryptor",
       "../api:rtp_headers",
       "../api:scoped_refptr",

video/video_stream_encoder_unittest.cc

@@ -17,6 +17,7 @@
 
 #include "absl/memory/memory.h"
 #include "api/task_queue/default_task_queue_factory.h"
+#include "api/test/mock_fec_controller_override.h"
 #include "api/video/builtin_video_bitrate_allocator_factory.h"
 #include "api/video/i420_buffer.h"
 #include "api/video/video_bitrate_allocation.h"
@@ -801,7 +802,8 @@ class VideoStreamEncoderTest : public ::testing::Test {
         // Simulate setting up temporal layers, in order to validate the life
         // cycle of these objects.
         Vp8TemporalLayersFactory factory;
-        frame_buffer_controller_ = factory.Create(*config, settings);
+        frame_buffer_controller_ =
+            factory.Create(*config, settings, &fec_controller_override_);
       }
       if (force_init_encode_failed_) {
         initialized_ = EncoderState::kInitializationFailed;
@@ -869,6 +871,7 @@ class VideoStreamEncoderTest : public ::testing::Test {
     bool expect_null_frame_ = false;
     EncodedImageCallback* encoded_image_callback_
         RTC_GUARDED_BY(local_crit_sect_) = nullptr;
+    MockFecControllerOverride fec_controller_override_;
   };
 
   class TestSink : public VideoStreamEncoder::EncoderSink {