From f52d34d682a049028ef5f2d126721dce03a50d8b Mon Sep 17 00:00:00 2001 From: magjed Date: Tue, 29 Aug 2017 00:58:52 -0700 Subject: [PATCH] Let VideoEncoderSoftwareFallbackWrapper own the wrapped encoder Currently, ownership of the wrapped hardware encoder is handled outside VideoEncoderSoftwareFallbackWrapper. It's easier if VideoEncoderSoftwareFallbackWrapper owns and relases it instead. BUG=webrtc:7925 Review-Url: https://codereview.webrtc.org/3007683002 Cr-Commit-Position: refs/heads/master@{#19572} --- .../videoencodersoftwarefallbackwrapper.cc | 4 +- .../videoencodersoftwarefallbackwrapper.h | 7 +- ...encodersoftwarefallbackwrapper_unittest.cc | 69 ++++++++++--------- webrtc/media/engine/webrtcvideoengine.cc | 11 ++- webrtc/media/engine/webrtcvideoengine.h | 10 +-- 5 files changed, 50 insertions(+), 51 deletions(-) diff --git a/webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc b/webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc index 946b91e158..7e2a678a56 100644 --- a/webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc +++ b/webrtc/media/engine/videoencodersoftwarefallbackwrapper.cc @@ -69,7 +69,7 @@ void GetForcedFallbackParamsFromFieldTrialGroup(uint32_t* param_low_kbps, VideoEncoderSoftwareFallbackWrapper::VideoEncoderSoftwareFallbackWrapper( const cricket::VideoCodec& codec, - webrtc::VideoEncoder* encoder) + std::unique_ptr encoder) : number_of_cores_(0), max_payload_size_(0), rates_set_(false), @@ -78,7 +78,7 @@ VideoEncoderSoftwareFallbackWrapper::VideoEncoderSoftwareFallbackWrapper( packet_loss_(0), rtt_(0), codec_(codec), - encoder_(encoder), + encoder_(std::move(encoder)), callback_(nullptr), forced_fallback_possible_(EnableForcedFallback(codec)) { if (forced_fallback_possible_) { diff --git a/webrtc/media/engine/videoencodersoftwarefallbackwrapper.h b/webrtc/media/engine/videoencodersoftwarefallbackwrapper.h index 8061c008c6..8dec2f5448 100644 --- a/webrtc/media/engine/videoencodersoftwarefallbackwrapper.h +++ b/webrtc/media/engine/videoencodersoftwarefallbackwrapper.h @@ -25,8 +25,9 @@ namespace webrtc { // hardware restrictions, such as max resolution. class VideoEncoderSoftwareFallbackWrapper : public VideoEncoder { public: - VideoEncoderSoftwareFallbackWrapper(const cricket::VideoCodec& codec, - webrtc::VideoEncoder* encoder); + VideoEncoderSoftwareFallbackWrapper( + const cricket::VideoCodec& codec, + std::unique_ptr encoder); int32_t InitEncode(const VideoCodec* codec_settings, int32_t number_of_cores, @@ -96,7 +97,7 @@ class VideoEncoderSoftwareFallbackWrapper : public VideoEncoder { int64_t rtt_; cricket::VideoCodec codec_; - webrtc::VideoEncoder* const encoder_; + std::unique_ptr encoder_; std::unique_ptr fallback_encoder_; std::string fallback_implementation_name_; diff --git a/webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc b/webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc index ecf719948f..f8521ab888 100644 --- a/webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc +++ b/webrtc/media/engine/videoencodersoftwarefallbackwrapper_unittest.cc @@ -39,7 +39,9 @@ class VideoEncoderSoftwareFallbackWrapperTest : public ::testing::Test { explicit VideoEncoderSoftwareFallbackWrapperTest( const std::string& field_trials) : override_field_trials_(field_trials), - fallback_wrapper_(cricket::VideoCodec("VP8"), &fake_encoder_) {} + fake_encoder_(new CountingFakeEncoder()), + fallback_wrapper_(cricket::VideoCodec("VP8"), + std::unique_ptr(fake_encoder_)) {} class CountingFakeEncoder : public VideoEncoder { public: @@ -134,7 +136,8 @@ class VideoEncoderSoftwareFallbackWrapperTest : public ::testing::Test { test::ScopedFieldTrials override_field_trials_; FakeEncodedImageCallback callback_; - CountingFakeEncoder fake_encoder_; + // |fake_encoder_| is owned and released by |fallback_wrapper_|. + CountingFakeEncoder* fake_encoder_; VideoEncoderSoftwareFallbackWrapper fallback_wrapper_; VideoCodec codec_ = {}; std::unique_ptr frame_; @@ -157,7 +160,7 @@ void VideoEncoderSoftwareFallbackWrapperTest::EncodeFrame(int expected_ret) { void VideoEncoderSoftwareFallbackWrapperTest::UtilizeFallbackEncoder() { fallback_wrapper_.RegisterEncodeCompleteCallback(&callback_); - EXPECT_EQ(&callback_, fake_encoder_.encode_complete_callback_); + EXPECT_EQ(&callback_, fake_encoder_->encode_complete_callback_); // Register with failing fake encoder. Should succeed with VP8 fallback. codec_.codecType = kVideoCodecVP8; @@ -171,7 +174,7 @@ void VideoEncoderSoftwareFallbackWrapperTest::UtilizeFallbackEncoder() { rate_allocator_.reset( new SimulcastRateAllocator(codec_, std::move(tl_factory))); - fake_encoder_.init_encode_return_code_ = WEBRTC_VIDEO_CODEC_ERROR; + fake_encoder_->init_encode_return_code_ = WEBRTC_VIDEO_CODEC_ERROR; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); EXPECT_EQ( @@ -180,9 +183,9 @@ void VideoEncoderSoftwareFallbackWrapperTest::UtilizeFallbackEncoder() { rate_allocator_->GetAllocation(300000, kFramerate), kFramerate)); int callback_count = callback_.callback_count_; - int encode_count = fake_encoder_.encode_count_; + int encode_count = fake_encoder_->encode_count_; EncodeFrame(); - EXPECT_EQ(encode_count, fake_encoder_.encode_count_); + EXPECT_EQ(encode_count, fake_encoder_->encode_count_); EXPECT_EQ(callback_count + 1, callback_.callback_count_); } @@ -203,30 +206,30 @@ void VideoEncoderSoftwareFallbackWrapperTest::FallbackFromEncodeRequest() { WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.SetRateAllocation( rate_allocator_->GetAllocation(300000, kFramerate), kFramerate)); - EXPECT_EQ(1, fake_encoder_.init_encode_count_); + EXPECT_EQ(1, fake_encoder_->init_encode_count_); // Have the non-fallback encoder request a software fallback. - fake_encoder_.encode_return_code_ = WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; + fake_encoder_->encode_return_code_ = WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE; int callback_count = callback_.callback_count_; - int encode_count = fake_encoder_.encode_count_; + int encode_count = fake_encoder_->encode_count_; EncodeFrame(); // Single encode request, which returned failure. - EXPECT_EQ(encode_count + 1, fake_encoder_.encode_count_); + EXPECT_EQ(encode_count + 1, fake_encoder_->encode_count_); EXPECT_EQ(callback_count + 1, callback_.callback_count_); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, InitializesEncoder) { VideoCodec codec = {}; fallback_wrapper_.InitEncode(&codec, 2, kMaxPayloadSize); - EXPECT_EQ(1, fake_encoder_.init_encode_count_); + EXPECT_EQ(1, fake_encoder_->init_encode_count_); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, EncodeRequestsFallback) { FallbackFromEncodeRequest(); // After fallback, further encodes shouldn't hit the fake encoder. - int encode_count = fake_encoder_.encode_count_; + int encode_count = fake_encoder_->encode_count_; EncodeFrame(); - EXPECT_EQ(encode_count, fake_encoder_.encode_count_); + EXPECT_EQ(encode_count, fake_encoder_->encode_count_); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, CanUtilizeFallbackEncoder) { @@ -236,20 +239,20 @@ TEST_F(VideoEncoderSoftwareFallbackWrapperTest, CanUtilizeFallbackEncoder) { TEST_F(VideoEncoderSoftwareFallbackWrapperTest, InternalEncoderReleasedDuringFallback) { - EXPECT_EQ(0, fake_encoder_.release_count_); + EXPECT_EQ(0, fake_encoder_->release_count_); UtilizeFallbackEncoder(); - EXPECT_EQ(1, fake_encoder_.release_count_); + EXPECT_EQ(1, fake_encoder_->release_count_); EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.Release()); // No extra release when the fallback is released. - EXPECT_EQ(1, fake_encoder_.release_count_); + EXPECT_EQ(1, fake_encoder_->release_count_); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, InternalEncoderNotEncodingDuringFallback) { UtilizeFallbackEncoder(); - int encode_count = fake_encoder_.encode_count_; + int encode_count = fake_encoder_->encode_count_; EncodeFrame(); - EXPECT_EQ(encode_count, fake_encoder_.encode_count_); + EXPECT_EQ(encode_count, fake_encoder_->encode_count_); EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.Release()); } @@ -261,7 +264,7 @@ TEST_F(VideoEncoderSoftwareFallbackWrapperTest, // encoder is being used. FakeEncodedImageCallback callback2; fallback_wrapper_.RegisterEncodeCompleteCallback(&callback2); - EXPECT_EQ(&callback2, fake_encoder_.encode_complete_callback_); + EXPECT_EQ(&callback2, fake_encoder_->encode_complete_callback_); // Encoding a frame using the fallback should arrive at the new callback. std::vector types(1, kVideoFrameKey); @@ -275,32 +278,32 @@ TEST_F(VideoEncoderSoftwareFallbackWrapperTest, TEST_F(VideoEncoderSoftwareFallbackWrapperTest, SetChannelParametersForwardedDuringFallback) { UtilizeFallbackEncoder(); - EXPECT_EQ(0, fake_encoder_.set_channel_parameters_count_); + EXPECT_EQ(0, fake_encoder_->set_channel_parameters_count_); fallback_wrapper_.SetChannelParameters(1, 1); - EXPECT_EQ(1, fake_encoder_.set_channel_parameters_count_); + EXPECT_EQ(1, fake_encoder_->set_channel_parameters_count_); EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.Release()); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, SetRatesForwardedDuringFallback) { UtilizeFallbackEncoder(); - EXPECT_EQ(1, fake_encoder_.set_rates_count_); + EXPECT_EQ(1, fake_encoder_->set_rates_count_); fallback_wrapper_.SetRateAllocation(BitrateAllocation(), 1); - EXPECT_EQ(2, fake_encoder_.set_rates_count_); + EXPECT_EQ(2, fake_encoder_->set_rates_count_); EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.Release()); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, SupportsNativeHandleForwardedWithoutFallback) { fallback_wrapper_.SupportsNativeHandle(); - EXPECT_EQ(1, fake_encoder_.supports_native_handle_count_); + EXPECT_EQ(1, fake_encoder_->supports_native_handle_count_); } TEST_F(VideoEncoderSoftwareFallbackWrapperTest, SupportsNativeHandleNotForwardedDuringFallback) { UtilizeFallbackEncoder(); fallback_wrapper_.SupportsNativeHandle(); - EXPECT_EQ(0, fake_encoder_.supports_native_handle_count_); + EXPECT_EQ(0, fake_encoder_->supports_native_handle_count_); EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.Release()); } @@ -343,7 +346,7 @@ class ForcedFallbackTest : public VideoEncoderSoftwareFallbackWrapperTest { ConfigureVp8Codec(); EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode( &codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(1, fake_encoder_.init_encode_count_); + EXPECT_EQ(1, fake_encoder_->init_encode_count_); } void TearDown() override { @@ -509,7 +512,7 @@ TEST_F(ForcedFallbackTestEnabled, MultipleStartEndFallback) { } TEST_F(ForcedFallbackTestEnabled, DropsFirstNonNativeFrameAfterFallbackEnds) { - fake_encoder_.supports_native_handle_ = true; + fake_encoder_->supports_native_handle_ = true; // Bitrate at low threshold. SetRateAllocation(kLowKbps); @@ -536,7 +539,7 @@ TEST_F(ForcedFallbackTestEnabled, FallbackIsKeptWhenInitEncodeIsCalled) { // Re-initialize encoder, still expect fallback. EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(1, fake_encoder_.init_encode_count_); // No change. + EXPECT_EQ(1, fake_encoder_->init_encode_count_); // No change. SetRateAllocation(kLowKbps); EncodeFrameAndVerifyLastName("libvpx"); } @@ -553,7 +556,7 @@ TEST_F(ForcedFallbackTestEnabled, FallbackIsEndedWhenResolutionIsTooLarge) { codec_.width += 1; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(2, fake_encoder_.init_encode_count_); + EXPECT_EQ(2, fake_encoder_->init_encode_count_); SetRateAllocation(kLowKbps); EncodeFrameAndVerifyLastName("fake-encoder"); } @@ -570,7 +573,7 @@ TEST_F(ForcedFallbackTestEnabled, FallbackIsEndedForNonValidSettings) { codec_.VP8()->numberOfTemporalLayers = 2; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(2, fake_encoder_.init_encode_count_); + EXPECT_EQ(2, fake_encoder_->init_encode_count_); SetRateAllocation(kLowKbps); EncodeFrameAndVerifyLastName("fake-encoder"); @@ -578,7 +581,7 @@ TEST_F(ForcedFallbackTestEnabled, FallbackIsEndedForNonValidSettings) { codec_.VP8()->numberOfTemporalLayers = 1; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(3, fake_encoder_.init_encode_count_); + EXPECT_EQ(3, fake_encoder_->init_encode_count_); // Bitrate at low threshold. SetRateAllocation(kLowKbps); EncodeFrameAndVerifyLastName("fake-encoder"); @@ -622,7 +625,7 @@ TEST_F(ForcedFallbackTestEnabled, FallbackIsKeptIfResolutionIsTooSmall) { codec_.height = kMinPixelsStop / codec_.width - 1; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(1, fake_encoder_.init_encode_count_); // No change + EXPECT_EQ(1, fake_encoder_->init_encode_count_); // No change SetRateAllocation(kHighKbps - 1); EncodeFrameAndVerifyLastName("libvpx"); // Bitrate at high threshold but resolution too small for fallback to end. @@ -633,7 +636,7 @@ TEST_F(ForcedFallbackTestEnabled, FallbackIsKeptIfResolutionIsTooSmall) { codec_.height++; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, fallback_wrapper_.InitEncode(&codec_, kNumCores, kMaxPayloadSize)); - EXPECT_EQ(1, fake_encoder_.init_encode_count_); // No change + EXPECT_EQ(1, fake_encoder_->init_encode_count_); // No change SetRateAllocation(kHighKbps - 1); EncodeFrameAndVerifyLastName("libvpx"); // Bitrate at high threshold and resolution large enough for fallback to end. diff --git a/webrtc/media/engine/webrtcvideoengine.cc b/webrtc/media/engine/webrtcvideoengine.cc index 9c11bb9469..274b801625 100644 --- a/webrtc/media/engine/webrtcvideoengine.cc +++ b/webrtc/media/engine/webrtcvideoengine.cc @@ -1415,16 +1415,15 @@ WebRtcVideoChannel::WebRtcVideoSendStream::VideoSendStreamParameters:: WebRtcVideoChannel::WebRtcVideoSendStream::AllocatedEncoder::AllocatedEncoder( std::unique_ptr encoder, - std::unique_ptr external_encoder, + bool is_external, const cricket::VideoCodec& codec, bool has_internal_source) : encoder_(std::move(encoder)), - external_encoder_(std::move(external_encoder)), + is_external_(is_external), codec_(codec), has_internal_source_(has_internal_source) {} void WebRtcVideoChannel::WebRtcVideoSendStream::AllocatedEncoder::Reset() { - external_encoder_.reset(); encoder_.reset(); codec_ = cricket::VideoCodec(); } @@ -1602,13 +1601,13 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoder( if (external_encoder) { std::unique_ptr internal_encoder( new webrtc::VideoEncoderSoftwareFallbackWrapper( - codec, external_encoder.get())); + codec, std::move(external_encoder))); const webrtc::VideoCodecType codec_type = webrtc::PayloadStringToCodecType(codec.name); const bool has_internal_source = external_encoder_factory_->EncoderTypeHasInternalSource(codec_type); return AllocatedEncoder(std::move(internal_encoder), - std::move(external_encoder), codec, + true /* is_external */, codec, has_internal_source); } } @@ -1629,7 +1628,7 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoder( internal_encoder_factory_->CreateVideoEncoder(codec)); } return AllocatedEncoder(std::move(internal_encoder), - nullptr /* external_encoder */, codec, + false /* is_external */, codec, false /* has_internal_source */); } diff --git a/webrtc/media/engine/webrtcvideoengine.h b/webrtc/media/engine/webrtcvideoengine.h index caca11c1ff..deb13b1d2c 100644 --- a/webrtc/media/engine/webrtcvideoengine.h +++ b/webrtc/media/engine/webrtcvideoengine.h @@ -317,7 +317,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, public webrtc::Transport { AllocatedEncoder& operator=(AllocatedEncoder&&) = default; AllocatedEncoder(std::unique_ptr encoder, - std::unique_ptr external_encoder, + bool is_external, const cricket::VideoCodec& codec, bool has_internal_source); @@ -326,7 +326,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, public webrtc::Transport { webrtc::VideoEncoder* encoder() { return encoder_.get(); } // Returns true if the encoder is external. - bool IsExternal() { return static_cast(external_encoder_); } + bool IsExternal() { return is_external_; } cricket::VideoCodec codec() { return codec_; } @@ -337,11 +337,7 @@ class WebRtcVideoChannel : public VideoMediaChannel, public webrtc::Transport { private: std::unique_ptr encoder_; - // TODO(magjed): |external_encoder_| is not used except for managing - // the lifetime when we use VideoEncoderSoftwareFallbackWrapper. Let - // VideoEncoderSoftwareFallbackWrapper own the external encoder instead - // and remove this member variable. - std::unique_ptr external_encoder_; + bool is_external_; cricket::VideoCodec codec_; bool has_internal_source_; };