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}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index 77246bc..2ca55c7 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -649,6 +649,17 @@
]
}
+ 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 = [
diff --git a/api/test/mock_fec_controller_override.h b/api/test/mock_fec_controller_override.h
new file mode 100644
index 0000000..a7ec836
--- /dev/null
+++ b/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_
diff --git a/api/video_codecs/BUILD.gn b/api/video_codecs/BUILD.gn
index f53ae7b..894d23f 100644
--- a/api/video_codecs/BUILD.gn
+++ b/api/video_codecs/BUILD.gn
@@ -120,6 +120,7 @@
deps = [
":video_codecs_api",
+ "../:fec_controller_api",
"../../modules/video_coding:video_coding_utility",
"../../modules/video_coding:webrtc_vp8_temporal_layers",
"../../rtc_base:checks",
diff --git a/api/video_codecs/vp8_frame_buffer_controller.h b/api/video_codecs/vp8_frame_buffer_controller.h
index 94e08a9..f304413 100644
--- a/api/video_codecs/vp8_frame_buffer_controller.h
+++ b/api/video_codecs/vp8_frame_buffer_controller.h
@@ -106,13 +106,6 @@
// 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 @@
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
diff --git a/api/video_codecs/vp8_temporal_layers.cc b/api/video_codecs/vp8_temporal_layers.cc
index b29cf65..dd75c61 100644
--- a/api/video_codecs/vp8_temporal_layers.cc
+++ b/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 @@
[](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 @@
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();
}
diff --git a/api/video_codecs/vp8_temporal_layers.h b/api/video_codecs/vp8_temporal_layers.h
index efd013e..2ffe6ea 100644
--- a/api/video_codecs/vp8_temporal_layers.h
+++ b/api/video_codecs/vp8_temporal_layers.h
@@ -32,15 +32,13 @@
// 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;
diff --git a/api/video_codecs/vp8_temporal_layers_factory.cc b/api/video_codecs/vp8_temporal_layers_factory.cc
index 1a77323..f7d991c 100644
--- a/api/video_codecs/vp8_temporal_layers_factory.cc
+++ b/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 @@
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 @@
}
}
- return absl::make_unique<Vp8TemporalLayers>(std::move(controllers));
+ return absl::make_unique<Vp8TemporalLayers>(std::move(controllers),
+ fec_controller_override);
}
std::unique_ptr<Vp8FrameBufferControllerFactory>
diff --git a/api/video_codecs/vp8_temporal_layers_factory.h b/api/video_codecs/vp8_temporal_layers_factory.h
index 747580f..082bfe2 100644
--- a/api/video_codecs/vp8_temporal_layers_factory.h
+++ b/api/video_codecs/vp8_temporal_layers_factory.h
@@ -23,9 +23,15 @@
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
diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn
index 1646fe6..9d0c65b 100644
--- a/modules/video_coding/BUILD.gn
+++ b/modules/video_coding/BUILD.gn
@@ -848,6 +848,7 @@
"../../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",
diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers.cc b/modules/video_coding/codecs/vp8/default_temporal_layers.cc
index 07d6c16..84e948e 100644
--- a/modules/video_coding/codecs/vp8/default_temporal_layers.cc
+++ b/modules/video_coding/codecs/vp8/default_temporal_layers.cc
@@ -257,11 +257,6 @@
// Ignore.
}
-void DefaultTemporalLayers::SetFecControllerOverride(
- FecControllerOverride* fec_controller_override) {
- // Ignore.
-}
-
size_t DefaultTemporalLayers::StreamCount() const {
return 1;
}
diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers.h b/modules/video_coding/codecs/vp8/default_temporal_layers.h
index e8a1cee..9f86d40 100644
--- a/modules/video_coding/codecs/vp8/default_temporal_layers.h
+++ b/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 @@
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;
diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc
index 9b984f7..c630e35 100644
--- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc
+++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc
@@ -290,7 +290,8 @@
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::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 @@
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_);
diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h
index 3caf3ab..49cf4cb 100644
--- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h
+++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h
@@ -141,6 +141,8 @@
std::string group_name);
FramerateController framerate_controller_;
int num_steady_state_frames_;
+
+ FecControllerOverride* fec_controller_override_;
};
} // namespace webrtc
diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.cc b/modules/video_coding/codecs/vp8/screenshare_layers.cc
index 84f3d11..b5b963e 100644
--- a/modules/video_coding/codecs/vp8/screenshare_layers.cc
+++ b/modules/video_coding/codecs/vp8/screenshare_layers.cc
@@ -87,11 +87,6 @@
}
}
-void ScreenshareLayers::SetFecControllerOverride(
- FecControllerOverride* fec_controller_override) {
- // Ignore.
-}
-
size_t ScreenshareLayers::StreamCount() const {
return 1;
}
diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.h b/modules/video_coding/codecs/vp8/screenshare_layers.h
index 770ea01..5270ffe 100644
--- a/modules/video_coding/codecs/vp8/screenshare_layers.h
+++ b/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 @@
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;
diff --git a/modules/video_coding/video_codec_initializer_unittest.cc b/modules/video_coding/video_codec_initializer_unittest.cc
index 5cac795..36db33a 100644
--- a/modules/video_coding/video_codec_initializer_unittest.cc
+++ b/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 @@
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 @@
return stream;
}
+ MockFecControllerOverride fec_controller_override_;
+
// Input settings.
VideoEncoderConfig config_;
std::vector<VideoStream> streams_;
diff --git a/test/fake_vp8_encoder.cc b/test/fake_vp8_encoder.cc
index 9d8d510..60bc36c 100644
--- a/test/fake_vp8_encoder.cc
+++ b/test/fake_vp8_encoder.cc
@@ -58,7 +58,8 @@
}
Vp8TemporalLayersFactory factory;
- frame_buffer_controller_ = factory.Create(*config, settings);
+ frame_buffer_controller_ =
+ factory.Create(*config, settings, &fec_controller_override_);
return WEBRTC_VIDEO_CODEC_OK;
}
diff --git a/test/fake_vp8_encoder.h b/test/fake_vp8_encoder.h
index dcfa5ad..a0d8e16 100644
--- a/test/fake_vp8_encoder.h
+++ b/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 @@
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_);
};
diff --git a/video/BUILD.gn b/video/BUILD.gn
index 8ecdd98..a2cf13a 100644
--- a/video/BUILD.gn
+++ b/video/BUILD.gn
@@ -549,6 +549,7 @@
"../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",
diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc
index 44c126b..982ba2d 100644
--- a/video/video_stream_encoder_unittest.cc
+++ b/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 @@
// 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 @@
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 {
