From 5b7fc8ce424490ccf3229c855facdef78e6cb615 Mon Sep 17 00:00:00 2001 From: tommi Date: Wed, 5 Jul 2017 16:45:57 -0700 Subject: [PATCH] A few simplifications to CodecDatabase and VCMGenericDecoder. * Remove the ReleaseDecoder and Release methods that were used in combination with deleting the decoder object. Now simply deleting the object does the right thing. * Remove 'friend' relationship between the two classes since they don't need to touch each other's state directly anymore. * Use std::unique_ptr for holding pointers and transferring ownership. These changes were previously reviewed here: https://codereview.webrtc.org/2764573002/ BUG=webrtc:7361, 695438 Review-Url: https://codereview.webrtc.org/2966823002 Cr-Commit-Position: refs/heads/master@{#18908} --- webrtc/modules/video_coding/codec_database.cc | 89 +++++++++---------- webrtc/modules/video_coding/codec_database.h | 16 ++-- .../modules/video_coding/generic_decoder.cc | 35 ++++---- webrtc/modules/video_coding/generic_decoder.h | 17 ++-- 4 files changed, 71 insertions(+), 86 deletions(-) diff --git a/webrtc/modules/video_coding/codec_database.cc b/webrtc/modules/video_coding/codec_database.cc index 92ae5d23b5..2f5e7fbc38 100644 --- a/webrtc/modules/video_coding/codec_database.cc +++ b/webrtc/modules/video_coding/codec_database.cc @@ -71,6 +71,31 @@ VideoCodecH264 VideoEncoder::GetDefaultH264Settings() { return h264_settings; } +// Create an internal Decoder given a codec type +static std::unique_ptr CreateDecoder(VideoCodecType type) { + switch (type) { + case kVideoCodecVP8: + return std::unique_ptr( + new VCMGenericDecoder(VP8Decoder::Create())); + case kVideoCodecVP9: + return std::unique_ptr( + new VCMGenericDecoder(VP9Decoder::Create())); + case kVideoCodecI420: + return std::unique_ptr( + new VCMGenericDecoder(new I420Decoder())); + case kVideoCodecH264: + if (H264Decoder::IsSupported()) { + return std::unique_ptr( + new VCMGenericDecoder(H264Decoder::Create())); + } + break; + default: + break; + } + LOG(LS_WARNING) << "No internal decoder of this type exists."; + return std::unique_ptr(); +} + VCMDecoderMapItem::VCMDecoderMapItem(VideoCodec* settings, int number_of_cores, bool require_key_frame) @@ -98,13 +123,12 @@ VCMCodecDataBase::VCMCodecDataBase( external_encoder_(nullptr), internal_source_(false), encoded_frame_callback_(encoded_frame_callback), - ptr_decoder_(nullptr), dec_map_(), dec_external_map_() {} VCMCodecDataBase::~VCMCodecDataBase() { DeleteEncoder(); - ReleaseDecoder(ptr_decoder_); + ptr_decoder_.reset(); for (auto& kv : dec_map_) delete kv.second; for (auto& kv : dec_external_map_) @@ -400,11 +424,10 @@ bool VCMCodecDataBase::DeregisterExternalDecoder(uint8_t payload_type) { // We can't use payload_type to check if the decoder is currently in use, // because payload type may be out of date (e.g. before we decode the first // frame after RegisterReceiveCodec) - if (ptr_decoder_ != nullptr && - ptr_decoder_->_decoder == (*it).second->external_decoder_instance) { + if (ptr_decoder_ && + ptr_decoder_->IsSameDecoder((*it).second->external_decoder_instance)) { // Release it if it was registered and in use. - ReleaseDecoder(ptr_decoder_); - ptr_decoder_ = nullptr; + ptr_decoder_.reset(); } DeregisterReceiveCodec(payload_type); delete it->second; @@ -464,12 +487,11 @@ VCMGenericDecoder* VCMCodecDataBase::GetDecoder( RTC_DCHECK(decoded_frame_callback->UserReceiveCallback()); uint8_t payload_type = frame.PayloadType(); if (payload_type == receive_codec_.plType || payload_type == 0) { - return ptr_decoder_; + return ptr_decoder_.get(); } // Check for exisitng decoder, if exists - delete. if (ptr_decoder_) { - ReleaseDecoder(ptr_decoder_); - ptr_decoder_ = nullptr; + ptr_decoder_.reset(); memset(&receive_codec_, 0, sizeof(VideoCodec)); } ptr_decoder_ = CreateAndInitDecoder(frame, &receive_codec_); @@ -480,36 +502,26 @@ VCMGenericDecoder* VCMCodecDataBase::GetDecoder( callback->OnIncomingPayloadType(receive_codec_.plType); if (ptr_decoder_->RegisterDecodeCompleteCallback(decoded_frame_callback) < 0) { - ReleaseDecoder(ptr_decoder_); - ptr_decoder_ = nullptr; + ptr_decoder_.reset(); memset(&receive_codec_, 0, sizeof(VideoCodec)); return nullptr; } - return ptr_decoder_; + return ptr_decoder_.get(); } -void VCMCodecDataBase::ReleaseDecoder(VCMGenericDecoder* decoder) const { - if (decoder) { - RTC_DCHECK(decoder->_decoder); - decoder->Release(); - if (!decoder->External()) { - delete decoder->_decoder; - } - delete decoder; - } +VCMGenericDecoder* VCMCodecDataBase::GetCurrentDecoder() { + return ptr_decoder_.get(); } bool VCMCodecDataBase::PrefersLateDecoding() const { - if (!ptr_decoder_) - return true; - return ptr_decoder_->PrefersLateDecoding(); + return ptr_decoder_ ? ptr_decoder_->PrefersLateDecoding() : true; } bool VCMCodecDataBase::MatchesCurrentResolution(int width, int height) const { return send_codec_.width == width && send_codec_.height == height; } -VCMGenericDecoder* VCMCodecDataBase::CreateAndInitDecoder( +std::unique_ptr VCMCodecDataBase::CreateAndInitDecoder( const VCMEncodedFrame& frame, VideoCodec* new_codec) const { uint8_t payload_type = frame.PayloadType(); @@ -522,13 +534,13 @@ VCMGenericDecoder* VCMCodecDataBase::CreateAndInitDecoder( << static_cast(payload_type); return nullptr; } - VCMGenericDecoder* ptr_decoder = nullptr; + std::unique_ptr ptr_decoder; const VCMExtDecoderMapItem* external_dec_item = FindExternalDecoderItem(payload_type); if (external_dec_item) { // External codec. - ptr_decoder = new VCMGenericDecoder( - external_dec_item->external_decoder_instance, true); + ptr_decoder.reset(new VCMGenericDecoder( + external_dec_item->external_decoder_instance, true)); } else { // Create decoder. ptr_decoder = CreateDecoder(decoder_item->settings->codecType); @@ -547,7 +559,6 @@ VCMGenericDecoder* VCMCodecDataBase::CreateAndInitDecoder( } if (ptr_decoder->InitDecode(decoder_item->settings.get(), decoder_item->number_of_cores) < 0) { - ReleaseDecoder(ptr_decoder); return nullptr; } memcpy(new_codec, decoder_item->settings.get(), sizeof(VideoCodec)); @@ -561,26 +572,6 @@ void VCMCodecDataBase::DeleteEncoder() { ptr_encoder_.reset(); } -VCMGenericDecoder* VCMCodecDataBase::CreateDecoder(VideoCodecType type) const { - switch (type) { - case kVideoCodecVP8: - return new VCMGenericDecoder(VP8Decoder::Create()); - case kVideoCodecVP9: - return new VCMGenericDecoder(VP9Decoder::Create()); - case kVideoCodecI420: - return new VCMGenericDecoder(new I420Decoder()); - case kVideoCodecH264: - if (H264Decoder::IsSupported()) { - return new VCMGenericDecoder(H264Decoder::Create()); - } - break; - default: - break; - } - LOG(LS_WARNING) << "No internal decoder of this type exists."; - return nullptr; -} - const VCMDecoderMapItem* VCMCodecDataBase::FindDecoderItem( uint8_t payload_type) const { DecoderMap::const_iterator it = dec_map_.find(payload_type); diff --git a/webrtc/modules/video_coding/codec_database.h b/webrtc/modules/video_coding/codec_database.h index 1317e2262c..5f8656df8d 100644 --- a/webrtc/modules/video_coding/codec_database.h +++ b/webrtc/modules/video_coding/codec_database.h @@ -107,9 +107,9 @@ class VCMCodecDataBase { const VCMEncodedFrame& frame, VCMDecodedFrameCallback* decoded_frame_callback); - // Deletes the memory of the decoder instance |decoder|. Used to delete - // deep copies returned by CreateDecoderCopy(). - void ReleaseDecoder(VCMGenericDecoder* decoder) const; + // Returns the current decoder (i.e. the same value as was last returned from + // GetDecoder(); + VCMGenericDecoder* GetCurrentDecoder(); // Returns true if the currently active decoder prefer to decode frames late. // That means that frames must be decoded near the render times stamp. @@ -121,8 +121,9 @@ class VCMCodecDataBase { typedef std::map DecoderMap; typedef std::map ExternalDecoderMap; - VCMGenericDecoder* CreateAndInitDecoder(const VCMEncodedFrame& frame, - VideoCodec* new_codec) const; + std::unique_ptr CreateAndInitDecoder( + const VCMEncodedFrame& frame, + VideoCodec* new_codec) const; // Determines whether a new codec has to be created or not. // Checks every setting apart from maxFramerate and startBitrate. @@ -130,9 +131,6 @@ class VCMCodecDataBase { void DeleteEncoder(); - // Create an internal Decoder given a codec type - VCMGenericDecoder* CreateDecoder(VideoCodecType type) const; - const VCMDecoderMapItem* FindDecoderItem(uint8_t payload_type) const; const VCMExtDecoderMapItem* FindExternalDecoderItem( @@ -149,7 +147,7 @@ class VCMCodecDataBase { bool internal_source_; VCMEncodedFrameCallback* const encoded_frame_callback_; std::unique_ptr ptr_encoder_; - VCMGenericDecoder* ptr_decoder_; + std::unique_ptr ptr_decoder_; DecoderMap dec_map_; ExternalDecoderMap dec_external_map_; }; // VCMCodecDataBase diff --git a/webrtc/modules/video_coding/generic_decoder.cc b/webrtc/modules/video_coding/generic_decoder.cc index 722b0b4154..42ee8b600c 100644 --- a/webrtc/modules/video_coding/generic_decoder.cc +++ b/webrtc/modules/video_coding/generic_decoder.cc @@ -8,12 +8,13 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include "webrtc/modules/video_coding/generic_decoder.h" + #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" #include "webrtc/base/timeutils.h" #include "webrtc/base/trace_event.h" #include "webrtc/modules/video_coding/include/video_coding.h" -#include "webrtc/modules/video_coding/generic_decoder.h" #include "webrtc/modules/video_coding/internal_defines.h" #include "webrtc/system_wrappers/include/clock.h" @@ -156,20 +157,26 @@ VCMGenericDecoder::VCMGenericDecoder(VideoDecoder* decoder, bool isExternal) : _callback(NULL), _frameInfos(), _nextFrameInfoIdx(0), - _decoder(decoder), + decoder_(decoder), _codecType(kVideoCodecUnknown), _isExternal(isExternal), - _keyFrameDecoded(false), - _last_keyframe_content_type(VideoContentType::UNSPECIFIED) {} + _last_keyframe_content_type(VideoContentType::UNSPECIFIED) { + RTC_DCHECK(decoder_); +} -VCMGenericDecoder::~VCMGenericDecoder() {} +VCMGenericDecoder::~VCMGenericDecoder() { + decoder_->Release(); + if (_isExternal) + decoder_.release(); + RTC_DCHECK(_isExternal || !decoder_); +} int32_t VCMGenericDecoder::InitDecode(const VideoCodec* settings, int32_t numberOfCores) { TRACE_EVENT0("webrtc", "VCMGenericDecoder::InitDecode"); _codecType = settings->codecType; - return _decoder->InitDecode(settings, numberOfCores); + return decoder_->InitDecode(settings, numberOfCores); } int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, int64_t nowMs) { @@ -192,11 +199,11 @@ int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, int64_t nowMs) { _nextFrameInfoIdx = (_nextFrameInfoIdx + 1) % kDecoderFrameMemoryLength; const RTPFragmentationHeader dummy_header; - int32_t ret = _decoder->Decode(frame.EncodedImage(), frame.MissingFrame(), + int32_t ret = decoder_->Decode(frame.EncodedImage(), frame.MissingFrame(), &dummy_header, frame.CodecSpecific(), frame.RenderTimeMs()); - _callback->OnDecoderImplementationName(_decoder->ImplementationName()); + _callback->OnDecoderImplementationName(decoder_->ImplementationName()); if (ret < WEBRTC_VIDEO_CODEC_OK) { LOG(LS_WARNING) << "Failed to decode frame with timestamp " << frame.TimeStamp() << ", error code: " << ret; @@ -210,22 +217,14 @@ int32_t VCMGenericDecoder::Decode(const VCMEncodedFrame& frame, int64_t nowMs) { return ret; } -int32_t VCMGenericDecoder::Release() { - return _decoder->Release(); -} - int32_t VCMGenericDecoder::RegisterDecodeCompleteCallback( VCMDecodedFrameCallback* callback) { _callback = callback; - return _decoder->RegisterDecodeCompleteCallback(callback); -} - -bool VCMGenericDecoder::External() const { - return _isExternal; + return decoder_->RegisterDecodeCompleteCallback(callback); } bool VCMGenericDecoder::PrefersLateDecoding() const { - return _decoder->PrefersLateDecoding(); + return decoder_->PrefersLateDecoding(); } } // namespace webrtc diff --git a/webrtc/modules/video_coding/generic_decoder.h b/webrtc/modules/video_coding/generic_decoder.h index 8e26a8b1af..f7196f04c5 100644 --- a/webrtc/modules/video_coding/generic_decoder.h +++ b/webrtc/modules/video_coding/generic_decoder.h @@ -11,6 +11,8 @@ #ifndef WEBRTC_MODULES_VIDEO_CODING_GENERIC_DECODER_H_ #define WEBRTC_MODULES_VIDEO_CODING_GENERIC_DECODER_H_ +#include + #include "webrtc/base/criticalsection.h" #include "webrtc/base/thread_checker.h" #include "webrtc/modules/include/module_common_types.h" @@ -73,8 +75,6 @@ class VCMDecodedFrameCallback : public DecodedImageCallback { }; class VCMGenericDecoder { - friend class VCMCodecDataBase; - public: explicit VCMGenericDecoder(VideoDecoder* decoder, bool isExternal = false); ~VCMGenericDecoder(); @@ -91,11 +91,6 @@ class VCMGenericDecoder { */ int32_t Decode(const VCMEncodedFrame& inputFrame, int64_t nowMs); - /** - * Free the decoder memory - */ - int32_t Release(); - /** * Set decode callback. Deregistering while decoding is illegal. */ @@ -103,15 +98,17 @@ class VCMGenericDecoder { bool External() const; bool PrefersLateDecoding() const; + bool IsSameDecoder(VideoDecoder* decoder) const { + return decoder_.get() == decoder; + } private: VCMDecodedFrameCallback* _callback; VCMFrameInformation _frameInfos[kDecoderFrameMemoryLength]; uint32_t _nextFrameInfoIdx; - VideoDecoder* const _decoder; + std::unique_ptr decoder_; VideoCodecType _codecType; - bool _isExternal; - bool _keyFrameDecoded; + const bool _isExternal; VideoContentType _last_keyframe_content_type; };