From 1d9061e8ae00a7150949810b006f8c84d13cbadf Mon Sep 17 00:00:00 2001 From: "henrik.lundin" Date: Tue, 26 Apr 2016 12:19:34 -0700 Subject: [PATCH] NetEq: Dependency injection through one struct instead of many params With this change, the NetEqImpl constructor takes a struct (NetEqImpl::Dependencies) as input instead of a collection of individual dependencies. The NetEqImpl unit test fixture is modified to make better used of unique_ptrs. Review URL: https://codereview.webrtc.org/1921243002 Cr-Commit-Position: refs/heads/master@{#12514} --- webrtc/modules/audio_coding/neteq/neteq.cc | 36 +-- .../modules/audio_coding/neteq/neteq_impl.cc | 60 ++--- .../modules/audio_coding/neteq/neteq_impl.h | 41 ++-- .../audio_coding/neteq/neteq_impl_unittest.cc | 220 ++++++++---------- 4 files changed, 156 insertions(+), 201 deletions(-) diff --git a/webrtc/modules/audio_coding/neteq/neteq.cc b/webrtc/modules/audio_coding/neteq/neteq.cc index 1af7966e41..c2a0cb66d1 100644 --- a/webrtc/modules/audio_coding/neteq/neteq.cc +++ b/webrtc/modules/audio_coding/neteq/neteq.cc @@ -12,20 +12,7 @@ #include -#include "webrtc/modules/audio_coding/neteq/accelerate.h" -#include "webrtc/modules/audio_coding/neteq/buffer_level_filter.h" -#include "webrtc/modules/audio_coding/neteq/decoder_database.h" -#include "webrtc/modules/audio_coding/neteq/delay_manager.h" -#include "webrtc/modules/audio_coding/neteq/delay_peak_detector.h" -#include "webrtc/modules/audio_coding/neteq/dtmf_buffer.h" -#include "webrtc/modules/audio_coding/neteq/dtmf_tone_generator.h" -#include "webrtc/modules/audio_coding/neteq/expand.h" #include "webrtc/modules/audio_coding/neteq/neteq_impl.h" -#include "webrtc/modules/audio_coding/neteq/packet_buffer.h" -#include "webrtc/modules/audio_coding/neteq/payload_splitter.h" -#include "webrtc/modules/audio_coding/neteq/preemptive_expand.h" -#include "webrtc/modules/audio_coding/neteq/tick_timer.h" -#include "webrtc/modules/audio_coding/neteq/timestamp_scaler.h" namespace webrtc { @@ -45,28 +32,7 @@ std::string NetEq::Config::ToString() const { // Creates all classes needed and inject them into a new NetEqImpl object. // Return the new object. NetEq* NetEq::Create(const NetEq::Config& config) { - std::unique_ptr tick_timer(new TickTimer); - BufferLevelFilter* buffer_level_filter = new BufferLevelFilter; - DecoderDatabase* decoder_database = new DecoderDatabase; - DelayPeakDetector* delay_peak_detector = new DelayPeakDetector; - DelayManager* delay_manager = - new DelayManager(config.max_packets_in_buffer, delay_peak_detector); - delay_manager->SetMaximumDelay(config.max_delay_ms); - DtmfBuffer* dtmf_buffer = new DtmfBuffer(config.sample_rate_hz); - DtmfToneGenerator* dtmf_tone_generator = new DtmfToneGenerator; - PacketBuffer* packet_buffer = - new PacketBuffer(config.max_packets_in_buffer, tick_timer.get()); - PayloadSplitter* payload_splitter = new PayloadSplitter; - TimestampScaler* timestamp_scaler = new TimestampScaler(*decoder_database); - AccelerateFactory* accelerate_factory = new AccelerateFactory; - ExpandFactory* expand_factory = new ExpandFactory; - PreemptiveExpandFactory* preemptive_expand_factory = - new PreemptiveExpandFactory; - return new NetEqImpl(config, std::move(tick_timer), buffer_level_filter, - decoder_database, delay_manager, delay_peak_detector, - dtmf_buffer, dtmf_tone_generator, packet_buffer, - payload_splitter, timestamp_scaler, accelerate_factory, - expand_factory, preemptive_expand_factory); + return new NetEqImpl(config, NetEqImpl::Dependencies(config)); } } // namespace webrtc diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.cc b/webrtc/modules/audio_coding/neteq/neteq_impl.cc index cca1c4c9e9..01325e33cd 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.cc @@ -54,35 +54,42 @@ namespace webrtc { +NetEqImpl::Dependencies::Dependencies(const NetEq::Config& config) + : tick_timer(new TickTimer), + buffer_level_filter(new BufferLevelFilter), + decoder_database(new DecoderDatabase), + delay_peak_detector(new DelayPeakDetector), + delay_manager(new DelayManager(config.max_packets_in_buffer, + delay_peak_detector.get())), + dtmf_buffer(new DtmfBuffer(config.sample_rate_hz)), + dtmf_tone_generator(new DtmfToneGenerator), + packet_buffer( + new PacketBuffer(config.max_packets_in_buffer, tick_timer.get())), + payload_splitter(new PayloadSplitter), + timestamp_scaler(new TimestampScaler(*decoder_database)), + accelerate_factory(new AccelerateFactory), + expand_factory(new ExpandFactory), + preemptive_expand_factory(new PreemptiveExpandFactory) {} + +NetEqImpl::Dependencies::~Dependencies() = default; + NetEqImpl::NetEqImpl(const NetEq::Config& config, - std::unique_ptr tick_timer, - BufferLevelFilter* buffer_level_filter, - DecoderDatabase* decoder_database, - DelayManager* delay_manager, - DelayPeakDetector* delay_peak_detector, - DtmfBuffer* dtmf_buffer, - DtmfToneGenerator* dtmf_tone_generator, - PacketBuffer* packet_buffer, - PayloadSplitter* payload_splitter, - TimestampScaler* timestamp_scaler, - AccelerateFactory* accelerate_factory, - ExpandFactory* expand_factory, - PreemptiveExpandFactory* preemptive_expand_factory, + Dependencies&& deps, bool create_components) - : tick_timer_(std::move(tick_timer)), - buffer_level_filter_(buffer_level_filter), - decoder_database_(decoder_database), - delay_manager_(delay_manager), - delay_peak_detector_(delay_peak_detector), - dtmf_buffer_(dtmf_buffer), - dtmf_tone_generator_(dtmf_tone_generator), - packet_buffer_(packet_buffer), - payload_splitter_(payload_splitter), - timestamp_scaler_(timestamp_scaler), + : tick_timer_(std::move(deps.tick_timer)), + buffer_level_filter_(std::move(deps.buffer_level_filter)), + decoder_database_(std::move(deps.decoder_database)), + delay_manager_(std::move(deps.delay_manager)), + delay_peak_detector_(std::move(deps.delay_peak_detector)), + dtmf_buffer_(std::move(deps.dtmf_buffer)), + dtmf_tone_generator_(std::move(deps.dtmf_tone_generator)), + packet_buffer_(std::move(deps.packet_buffer)), + payload_splitter_(std::move(deps.payload_splitter)), + timestamp_scaler_(std::move(deps.timestamp_scaler)), vad_(new PostDecodeVad()), - expand_factory_(expand_factory), - accelerate_factory_(accelerate_factory), - preemptive_expand_factory_(preemptive_expand_factory), + expand_factory_(std::move(deps.expand_factory)), + accelerate_factory_(std::move(deps.accelerate_factory)), + preemptive_expand_factory_(std::move(deps.preemptive_expand_factory)), last_mode_(kModeNormal), decoded_buffer_length_(kMaxFrameSize), decoded_buffer_(new int16_t[decoded_buffer_length_]), @@ -107,6 +114,7 @@ NetEqImpl::NetEqImpl(const NetEq::Config& config, "Changing to 8000 Hz."; fs = 8000; } + delay_manager_->SetMaximumDelay(config.max_delay_ms); fs_hz_ = fs; fs_mult_ = fs / 8000; last_output_sample_rate_hz_ = fs; diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl.h b/webrtc/modules/audio_coding/neteq/neteq_impl.h index 9d4a9ff269..707fbebca7 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl.h +++ b/webrtc/modules/audio_coding/neteq/neteq_impl.h @@ -66,22 +66,33 @@ class NetEqImpl : public webrtc::NetEq { kVadPassive }; - // Creates a new NetEqImpl object. The object will assume ownership of all - // injected dependencies, and will delete them when done. + struct Dependencies { + // The constructor populates the Dependencies struct with the default + // implementations of the objects. They can all be replaced by the user + // before sending the struct to the NetEqImpl constructor. However, there + // are dependencies between some of the classes inside the struct, so + // swapping out one may make it necessary to re-create another one. + explicit Dependencies(const NetEq::Config& config); + ~Dependencies(); + + std::unique_ptr tick_timer; + std::unique_ptr buffer_level_filter; + std::unique_ptr decoder_database; + std::unique_ptr delay_peak_detector; + std::unique_ptr delay_manager; + std::unique_ptr dtmf_buffer; + std::unique_ptr dtmf_tone_generator; + std::unique_ptr packet_buffer; + std::unique_ptr payload_splitter; + std::unique_ptr timestamp_scaler; + std::unique_ptr accelerate_factory; + std::unique_ptr expand_factory; + std::unique_ptr preemptive_expand_factory; + }; + + // Creates a new NetEqImpl object. NetEqImpl(const NetEq::Config& config, - std::unique_ptr tick_timer, - BufferLevelFilter* buffer_level_filter, - DecoderDatabase* decoder_database, - DelayManager* delay_manager, - DelayPeakDetector* delay_peak_detector, - DtmfBuffer* dtmf_buffer, - DtmfToneGenerator* dtmf_tone_generator, - PacketBuffer* packet_buffer, - PayloadSplitter* payload_splitter, - TimestampScaler* timestamp_scaler, - AccelerateFactory* accelerate_factory, - ExpandFactory* expand_factory, - PreemptiveExpandFactory* preemptive_expand_factory, + Dependencies&& deps, bool create_components = true); ~NetEqImpl() override; diff --git a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc index 4433d2067b..3e10507511 100644 --- a/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc @@ -54,107 +54,84 @@ int DeletePacketsAndReturnOk(PacketList* packet_list) { class NetEqImplTest : public ::testing::Test { protected: - NetEqImplTest() - : neteq_(NULL), - config_(), - tick_timer_(new TickTimer), - mock_buffer_level_filter_(NULL), - buffer_level_filter_(NULL), - use_mock_buffer_level_filter_(true), - mock_decoder_database_(NULL), - decoder_database_(NULL), - use_mock_decoder_database_(true), - mock_delay_peak_detector_(NULL), - delay_peak_detector_(NULL), - use_mock_delay_peak_detector_(true), - mock_delay_manager_(NULL), - delay_manager_(NULL), - use_mock_delay_manager_(true), - mock_dtmf_buffer_(NULL), - dtmf_buffer_(NULL), - use_mock_dtmf_buffer_(true), - mock_dtmf_tone_generator_(NULL), - dtmf_tone_generator_(NULL), - use_mock_dtmf_tone_generator_(true), - mock_packet_buffer_(NULL), - packet_buffer_(NULL), - use_mock_packet_buffer_(true), - mock_payload_splitter_(NULL), - payload_splitter_(NULL), - use_mock_payload_splitter_(true), - timestamp_scaler_(NULL) { - config_.sample_rate_hz = 8000; - } + NetEqImplTest() { config_.sample_rate_hz = 8000; } void CreateInstance() { + NetEqImpl::Dependencies deps(config_); + + // Get a local pointer to NetEq's TickTimer object. + tick_timer_ = deps.tick_timer.get(); + if (use_mock_buffer_level_filter_) { - mock_buffer_level_filter_ = new MockBufferLevelFilter; - buffer_level_filter_ = mock_buffer_level_filter_; - } else { - buffer_level_filter_ = new BufferLevelFilter; + std::unique_ptr mock(new MockBufferLevelFilter); + mock_buffer_level_filter_ = mock.get(); + deps.buffer_level_filter = std::move(mock); } + buffer_level_filter_ = deps.buffer_level_filter.get(); + if (use_mock_decoder_database_) { - mock_decoder_database_ = new MockDecoderDatabase; + std::unique_ptr mock(new MockDecoderDatabase); + mock_decoder_database_ = mock.get(); EXPECT_CALL(*mock_decoder_database_, GetActiveCngDecoder()) .WillOnce(ReturnNull()); - decoder_database_ = mock_decoder_database_; - } else { - decoder_database_ = new DecoderDatabase; + deps.decoder_database = std::move(mock); } - if (use_mock_delay_peak_detector_) { - mock_delay_peak_detector_ = new MockDelayPeakDetector; - EXPECT_CALL(*mock_delay_peak_detector_, Reset()).Times(1); - delay_peak_detector_ = mock_delay_peak_detector_; - } else { - delay_peak_detector_ = new DelayPeakDetector; - } - if (use_mock_delay_manager_) { - mock_delay_manager_ = new MockDelayManager(config_.max_packets_in_buffer, - delay_peak_detector_); - EXPECT_CALL(*mock_delay_manager_, set_streaming_mode(false)).Times(1); - delay_manager_ = mock_delay_manager_; - } else { - delay_manager_ = - new DelayManager(config_.max_packets_in_buffer, delay_peak_detector_); - } - if (use_mock_dtmf_buffer_) { - mock_dtmf_buffer_ = new MockDtmfBuffer(config_.sample_rate_hz); - dtmf_buffer_ = mock_dtmf_buffer_; - } else { - dtmf_buffer_ = new DtmfBuffer(config_.sample_rate_hz); - } - if (use_mock_dtmf_tone_generator_) { - mock_dtmf_tone_generator_ = new MockDtmfToneGenerator; - dtmf_tone_generator_ = mock_dtmf_tone_generator_; - } else { - dtmf_tone_generator_ = new DtmfToneGenerator; - } - if (use_mock_packet_buffer_) { - mock_packet_buffer_ = - new MockPacketBuffer(config_.max_packets_in_buffer, tick_timer_); - packet_buffer_ = mock_packet_buffer_; - } else { - packet_buffer_ = - new PacketBuffer(config_.max_packets_in_buffer, tick_timer_); - } - if (use_mock_payload_splitter_) { - mock_payload_splitter_ = new MockPayloadSplitter; - payload_splitter_ = mock_payload_splitter_; - } else { - payload_splitter_ = new PayloadSplitter; - } - timestamp_scaler_ = new TimestampScaler(*decoder_database_); - AccelerateFactory* accelerate_factory = new AccelerateFactory; - ExpandFactory* expand_factory = new ExpandFactory; - PreemptiveExpandFactory* preemptive_expand_factory = - new PreemptiveExpandFactory; + decoder_database_ = deps.decoder_database.get(); - neteq_ = new NetEqImpl( - config_, std::unique_ptr(tick_timer_), buffer_level_filter_, - decoder_database_, delay_manager_, delay_peak_detector_, dtmf_buffer_, - dtmf_tone_generator_, packet_buffer_, payload_splitter_, - timestamp_scaler_, accelerate_factory, expand_factory, - preemptive_expand_factory); + if (use_mock_delay_peak_detector_) { + std::unique_ptr mock(new MockDelayPeakDetector); + mock_delay_peak_detector_ = mock.get(); + EXPECT_CALL(*mock_delay_peak_detector_, Reset()).Times(1); + deps.delay_peak_detector = std::move(mock); + } + delay_peak_detector_ = deps.delay_peak_detector.get(); + + if (use_mock_delay_manager_) { + std::unique_ptr mock(new MockDelayManager( + config_.max_packets_in_buffer, delay_peak_detector_)); + mock_delay_manager_ = mock.get(); + EXPECT_CALL(*mock_delay_manager_, set_streaming_mode(false)).Times(1); + deps.delay_manager = std::move(mock); + } + delay_manager_ = deps.delay_manager.get(); + + if (use_mock_dtmf_buffer_) { + std::unique_ptr mock( + new MockDtmfBuffer(config_.sample_rate_hz)); + mock_dtmf_buffer_ = mock.get(); + deps.dtmf_buffer = std::move(mock); + } + dtmf_buffer_ = deps.dtmf_buffer.get(); + + if (use_mock_dtmf_tone_generator_) { + std::unique_ptr mock(new MockDtmfToneGenerator); + mock_dtmf_tone_generator_ = mock.get(); + deps.dtmf_tone_generator = std::move(mock); + } + dtmf_tone_generator_ = deps.dtmf_tone_generator.get(); + + if (use_mock_packet_buffer_) { + std::unique_ptr mock( + new MockPacketBuffer(config_.max_packets_in_buffer, tick_timer_)); + mock_packet_buffer_ = mock.get(); + deps.packet_buffer = std::move(mock); + } else { + deps.packet_buffer.reset( + new PacketBuffer(config_.max_packets_in_buffer, tick_timer_)); + } + packet_buffer_ = deps.packet_buffer.get(); + + if (use_mock_payload_splitter_) { + std::unique_ptr mock(new MockPayloadSplitter); + mock_payload_splitter_ = mock.get(); + deps.payload_splitter = std::move(mock); + } + payload_splitter_ = deps.payload_splitter.get(); + + deps.timestamp_scaler = std::unique_ptr( + new TimestampScaler(*deps.decoder_database.get())); + + neteq_.reset(new NetEqImpl(config_, std::move(deps))); ASSERT_TRUE(neteq_ != NULL); } @@ -192,37 +169,35 @@ class NetEqImplTest : public ::testing::Test { if (use_mock_packet_buffer_) { EXPECT_CALL(*mock_packet_buffer_, Die()).Times(1); } - delete neteq_; } - NetEqImpl* neteq_; + std::unique_ptr neteq_; NetEq::Config config_; - TickTimer* tick_timer_; - MockBufferLevelFilter* mock_buffer_level_filter_; - BufferLevelFilter* buffer_level_filter_; - bool use_mock_buffer_level_filter_; - MockDecoderDatabase* mock_decoder_database_; - DecoderDatabase* decoder_database_; - bool use_mock_decoder_database_; - MockDelayPeakDetector* mock_delay_peak_detector_; - DelayPeakDetector* delay_peak_detector_; - bool use_mock_delay_peak_detector_; - MockDelayManager* mock_delay_manager_; - DelayManager* delay_manager_; - bool use_mock_delay_manager_; - MockDtmfBuffer* mock_dtmf_buffer_; - DtmfBuffer* dtmf_buffer_; - bool use_mock_dtmf_buffer_; - MockDtmfToneGenerator* mock_dtmf_tone_generator_; - DtmfToneGenerator* dtmf_tone_generator_; - bool use_mock_dtmf_tone_generator_; - MockPacketBuffer* mock_packet_buffer_; - PacketBuffer* packet_buffer_; - bool use_mock_packet_buffer_; - MockPayloadSplitter* mock_payload_splitter_; - PayloadSplitter* payload_splitter_; - bool use_mock_payload_splitter_; - TimestampScaler* timestamp_scaler_; + TickTimer* tick_timer_ = nullptr; + MockBufferLevelFilter* mock_buffer_level_filter_ = nullptr; + BufferLevelFilter* buffer_level_filter_ = nullptr; + bool use_mock_buffer_level_filter_ = true; + MockDecoderDatabase* mock_decoder_database_ = nullptr; + DecoderDatabase* decoder_database_ = nullptr; + bool use_mock_decoder_database_ = true; + MockDelayPeakDetector* mock_delay_peak_detector_ = nullptr; + DelayPeakDetector* delay_peak_detector_ = nullptr; + bool use_mock_delay_peak_detector_ = true; + MockDelayManager* mock_delay_manager_ = nullptr; + DelayManager* delay_manager_ = nullptr; + bool use_mock_delay_manager_ = true; + MockDtmfBuffer* mock_dtmf_buffer_ = nullptr; + DtmfBuffer* dtmf_buffer_ = nullptr; + bool use_mock_dtmf_buffer_ = true; + MockDtmfToneGenerator* mock_dtmf_tone_generator_ = nullptr; + DtmfToneGenerator* dtmf_tone_generator_ = nullptr; + bool use_mock_dtmf_tone_generator_ = true; + MockPacketBuffer* mock_packet_buffer_ = nullptr; + PacketBuffer* packet_buffer_ = nullptr; + bool use_mock_packet_buffer_ = true; + MockPayloadSplitter* mock_payload_splitter_ = nullptr; + PayloadSplitter* payload_splitter_ = nullptr; + bool use_mock_payload_splitter_ = true; }; @@ -353,9 +328,6 @@ TEST_F(NetEqImplTest, InsertPacket) { } // Expectations for payload splitter. - EXPECT_CALL(*mock_payload_splitter_, SplitFec(_, _)) - .Times(2) - .WillRepeatedly(Return(PayloadSplitter::kOK)); EXPECT_CALL(*mock_payload_splitter_, SplitAudio(_, _)) .Times(2) .WillRepeatedly(Return(PayloadSplitter::kOK)); @@ -521,8 +493,6 @@ TEST_F(NetEqImplTest, ReorderedPacket) { EXPECT_CALL(mock_decoder, Channels()).WillRepeatedly(Return(1)); EXPECT_CALL(mock_decoder, IncomingPacket(_, kPayloadLengthBytes, _, _, _)) .WillRepeatedly(Return(0)); - EXPECT_CALL(mock_decoder, PacketDuration(_, kPayloadLengthBytes)) - .WillRepeatedly(Return(kPayloadLengthSamples)); int16_t dummy_output[kPayloadLengthSamples] = {0}; // The below expectation will make the mock decoder write // |kPayloadLengthSamples| zeros to the output array, and mark it as speech.