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

@@ -71,6 +71,31 @@ VideoCodecH264 VideoEncoder::GetDefaultH264Settings() {
   return h264_settings;
 }
 
+// Create an internal Decoder given a codec type
+static std::unique_ptr<VCMGenericDecoder> CreateDecoder(VideoCodecType type) {
+  switch (type) {
+    case kVideoCodecVP8:
+      return std::unique_ptr<VCMGenericDecoder>(
+          new VCMGenericDecoder(VP8Decoder::Create()));
+    case kVideoCodecVP9:
+      return std::unique_ptr<VCMGenericDecoder>(
+          new VCMGenericDecoder(VP9Decoder::Create()));
+    case kVideoCodecI420:
+      return std::unique_ptr<VCMGenericDecoder>(
+          new VCMGenericDecoder(new I420Decoder()));
+    case kVideoCodecH264:
+      if (H264Decoder::IsSupported()) {
+        return std::unique_ptr<VCMGenericDecoder>(
+            new VCMGenericDecoder(H264Decoder::Create()));
+      }
+      break;
+    default:
+      break;
+  }
+  LOG(LS_WARNING) << "No internal decoder of this type exists.";
+  return std::unique_ptr<VCMGenericDecoder>();
+}
+
 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 &&
+  if (ptr_decoder_ &&
-      ptr_decoder_->_decoder == (*it).second->external_decoder_instance) {
+      ptr_decoder_->IsSameDecoder((*it).second->external_decoder_instance)) {
     // Release it if it was registered and in use.
-    ReleaseDecoder(ptr_decoder_);
+    ptr_decoder_.reset();
-    ptr_decoder_ = nullptr;
   }
   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_.reset();
-    ptr_decoder_ = nullptr;
     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_.reset();
-    ptr_decoder_ = nullptr;
     memset(&receive_codec_, 0, sizeof(VideoCodec));
     return nullptr;
   }
-  return ptr_decoder_;
+  return ptr_decoder_.get();
 }
 
-void VCMCodecDataBase::ReleaseDecoder(VCMGenericDecoder* decoder) const {
+VCMGenericDecoder* VCMCodecDataBase::GetCurrentDecoder() {
-  if (decoder) {
+  return ptr_decoder_.get();
-    RTC_DCHECK(decoder->_decoder);
-    decoder->Release();
-    if (!decoder->External()) {
-      delete decoder->_decoder;
-    }
-    delete decoder;
-  }
 }
 
 bool VCMCodecDataBase::PrefersLateDecoding() const {
-  if (!ptr_decoder_)
+  return ptr_decoder_ ? ptr_decoder_->PrefersLateDecoding() : true;
-    return true;
-  return ptr_decoder_->PrefersLateDecoding();
 }
 
 bool VCMCodecDataBase::MatchesCurrentResolution(int width, int height) const {
   return send_codec_.width == width && send_codec_.height == height;
 }
 
-VCMGenericDecoder* VCMCodecDataBase::CreateAndInitDecoder(
+std::unique_ptr<VCMGenericDecoder> VCMCodecDataBase::CreateAndInitDecoder(
     const VCMEncodedFrame& frame,
     VideoCodec* new_codec) const {
   uint8_t payload_type = frame.PayloadType();
@@ -522,13 +534,13 @@ VCMGenericDecoder* VCMCodecDataBase::CreateAndInitDecoder(
                   << static_cast<int>(payload_type);
     return nullptr;
   }
-  VCMGenericDecoder* ptr_decoder = nullptr;
+  std::unique_ptr<VCMGenericDecoder> ptr_decoder;
   const VCMExtDecoderMapItem* external_dec_item =
       FindExternalDecoderItem(payload_type);
   if (external_dec_item) {
     // External codec.
-    ptr_decoder = new VCMGenericDecoder(
+    ptr_decoder.reset(new VCMGenericDecoder(
-        external_dec_item->external_decoder_instance, true);
+        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);

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
+  // Returns the current decoder (i.e. the same value as was last returned from
-  // deep copies returned by CreateDecoderCopy().
+  // GetDecoder();
-  void ReleaseDecoder(VCMGenericDecoder* decoder) const;
+  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,7 +121,8 @@ class VCMCodecDataBase {
   typedef std::map<uint8_t, VCMDecoderMapItem*> DecoderMap;
   typedef std::map<uint8_t, VCMExtDecoderMapItem*> ExternalDecoderMap;
 
-  VCMGenericDecoder* CreateAndInitDecoder(const VCMEncodedFrame& frame,
+  std::unique_ptr<VCMGenericDecoder> CreateAndInitDecoder(
+      const VCMEncodedFrame& frame,
       VideoCodec* new_codec) const;
 
   // Determines whether a new codec has to be created or not.
@@ -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<VCMGenericEncoder> ptr_encoder_;
-  VCMGenericDecoder* ptr_decoder_;
+  std::unique_ptr<VCMGenericDecoder> ptr_decoder_;
   DecoderMap dec_map_;
   ExternalDecoderMap dec_external_map_;
 };  // VCMCodecDataBase

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);
+  return decoder_->RegisterDecodeCompleteCallback(callback);
-}
-
-bool VCMGenericDecoder::External() const {
-  return _isExternal;
 }
 
 bool VCMGenericDecoder::PrefersLateDecoding() const {
-  return _decoder->PrefersLateDecoding();
+  return decoder_->PrefersLateDecoding();
 }
 
 }  // namespace webrtc

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 <memory>
+
 #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<VideoDecoder> decoder_;
   VideoCodecType _codecType;
-  bool _isExternal;
+  const bool _isExternal;
-  bool _keyFrameDecoded;
   VideoContentType _last_keyframe_content_type;
 };