From 45befc5f1fcffd095fe947ee5de82cc72b927aef Mon Sep 17 00:00:00 2001 From: Elad Alon Date: Tue, 2 Jul 2019 11:20:09 +0200 Subject: [PATCH] Pass FecControllerOverride to Vp8FrameBufferControllerFactory::Create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Elad Alon Cr-Commit-Position: refs/heads/master@{#28443} --- api/BUILD.gn | 11 ++++++++ api/test/mock_fec_controller_override.h | 28 +++++++++++++++++++ api/video_codecs/BUILD.gn | 1 + .../vp8_frame_buffer_controller.h | 24 ++++++++++------ api/video_codecs/vp8_temporal_layers.cc | 11 ++++---- api/video_codecs/vp8_temporal_layers.h | 8 ++---- .../vp8_temporal_layers_factory.cc | 11 +++++++- .../vp8_temporal_layers_factory.h | 6 ++++ modules/video_coding/BUILD.gn | 1 + .../codecs/vp8/default_temporal_layers.cc | 5 ---- .../codecs/vp8/default_temporal_layers.h | 4 --- .../codecs/vp8/libvpx_vp8_encoder.cc | 17 +++++++---- .../codecs/vp8/libvpx_vp8_encoder.h | 2 ++ .../codecs/vp8/screenshare_layers.cc | 5 ---- .../codecs/vp8/screenshare_layers.h | 4 --- .../video_codec_initializer_unittest.cc | 7 ++++- test/fake_vp8_encoder.cc | 3 +- test/fake_vp8_encoder.h | 11 ++++++++ video/BUILD.gn | 1 + video/video_stream_encoder_unittest.cc | 5 +++- 20 files changed, 118 insertions(+), 47 deletions(-) create mode 100644 api/test/mock_fec_controller_override.h diff --git a/api/BUILD.gn b/api/BUILD.gn index 77246bcbce..2ca55c7fd0 100644 --- a/api/BUILD.gn +++ b/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 = [ diff --git a/api/test/mock_fec_controller_override.h b/api/test/mock_fec_controller_override.h new file mode 100644 index 0000000000..a7ec8360ab --- /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 f53ae7bc7c..894d23f430 100644 --- a/api/video_codecs/BUILD.gn +++ b/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", diff --git a/api/video_codecs/vp8_frame_buffer_controller.h b/api/video_codecs/vp8_frame_buffer_controller.h index 94e08a9aea..f3044138ca 100644 --- a/api/video_codecs/vp8_frame_buffer_controller.h +++ b/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 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 Create( const VideoCodec& codec, - const VideoEncoder::Settings& settings) = 0; + const VideoEncoder::Settings& settings) { + return Create(codec, settings, nullptr); + } + virtual std::unique_ptr 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 b29cf65474..dd75c616d8 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>&& controllers) + std::vector>&& 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& 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(); } diff --git a/api/video_codecs/vp8_temporal_layers.h b/api/video_codecs/vp8_temporal_layers.h index efd013e4cd..2ffe6eacdf 100644 --- a/api/video_codecs/vp8_temporal_layers.h +++ b/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>&& controllers); + Vp8TemporalLayers( + std::vector>&& 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 1a773237a1..f7d991c89f 100644 --- a/api/video_codecs/vp8_temporal_layers_factory.cc +++ b/api/video_codecs/vp8_temporal_layers_factory.cc @@ -15,6 +15,7 @@ #include #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 Vp8TemporalLayersFactory::Create( const VideoCodec& codec, const VideoEncoder::Settings& settings) { + return Create(codec, settings, nullptr); +} + +std::unique_ptr Vp8TemporalLayersFactory::Create( + const VideoCodec& codec, + const VideoEncoder::Settings& settings, + FecControllerOverride* fec_controller_override) { std::vector> controllers; const int num_streams = SimulcastUtility::NumberOfSimulcastStreams(codec); RTC_DCHECK_GE(num_streams, 1); @@ -44,7 +52,8 @@ std::unique_ptr Vp8TemporalLayersFactory::Create( } } - return absl::make_unique(std::move(controllers)); + return absl::make_unique(std::move(controllers), + fec_controller_override); } std::unique_ptr diff --git a/api/video_codecs/vp8_temporal_layers_factory.h b/api/video_codecs/vp8_temporal_layers_factory.h index 747580ffdb..082bfe28dc 100644 --- a/api/video_codecs/vp8_temporal_layers_factory.h +++ b/api/video_codecs/vp8_temporal_layers_factory.h @@ -23,9 +23,15 @@ class Vp8TemporalLayersFactory : public Vp8FrameBufferControllerFactory { std::unique_ptr Clone() const override; + // TODO(bugs.webrtc.org/10769): Update downstream projects, then remove. std::unique_ptr Create( const VideoCodec& codec, const VideoEncoder::Settings& settings) override; + + std::unique_ptr 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 1646fe6053..9d0c65b3d1 100644 --- a/modules/video_coding/BUILD.gn +++ b/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", diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers.cc b/modules/video_coding/codecs/vp8/default_temporal_layers.cc index 07d6c16054..84e948e8e2 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 @@ void DefaultTemporalLayers::SetQpLimits(size_t stream_index, // 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 e8a1cee9bb..9f86d408ad 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 #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; diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc index 9b984f715c..c630e35402 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 @@ 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_); diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h index 3caf3abe02..49cf4cb233 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 @@ class LibvpxVp8Encoder : public VideoEncoder { 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 84f3d1178b..b5b963e2a9 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::SetQpLimits(size_t stream_index, } } -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 770ea01e99..5270ffe81c 100644 --- a/modules/video_coding/codecs/vp8/screenshare_layers.h +++ b/modules/video_coding/codecs/vp8/screenshare_layers.h @@ -14,7 +14,6 @@ #include #include -#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; diff --git a/modules/video_coding/video_codec_initializer_unittest.cc b/modules/video_coding/video_codec_initializer_unittest.cc index 5cac795c06..36db33ac4e 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 @@ 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 streams_; diff --git a/test/fake_vp8_encoder.cc b/test/fake_vp8_encoder.cc index 9d8d510ea3..60bc36c570 100644 --- a/test/fake_vp8_encoder.cc +++ b/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; } diff --git a/test/fake_vp8_encoder.h b/test/fake_vp8_encoder.h index dcfa5ad491..a0d8e167c7 100644 --- a/test/fake_vp8_encoder.h +++ b/test/fake_vp8_encoder.h @@ -16,6 +16,7 @@ #include +#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 frame_buffer_controller_ RTC_GUARDED_BY(sequence_checker_); }; diff --git a/video/BUILD.gn b/video/BUILD.gn index 8ecdd984e1..a2cf13a3f9 100644 --- a/video/BUILD.gn +++ b/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", diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 44c126b3d0..982ba2d497 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 @@ 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 {