Remove specialization of std::default_delete for media::VideoDecoder

Some VideoDecoder implementations must do non-synchronous work besides
resource reclamation during their destruction process, this CL allows
them to do that more cleanly and safely via an
'AsyncDestroyVideoDecoder' wrapper. All they need to do is implement a
static 'DestroyAsync' method that takes an instance by unique_ptr,
and they can post whatever tasks they need within that function without
worrying about being destroyed when it returns.

Change-Id: I05f258a4cda90b9850e1c4a22c1084bae2dda563
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2191275
Commit-Queue: Will Cassella <cassew@google.com>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782257}

media/base/BUILD.gn

@@ -37,6 +37,7 @@ jumbo_source_set("base") {
     "android_overlay_config.cc",
     "android_overlay_config.h",
     "android_overlay_mojo_factory.h",
+    "async_destroy_video_decoder.h",
     "audio_block_fifo.cc",
     "audio_block_fifo.h",
     "audio_buffer.cc",

media/base/async_destroy_video_decoder.h

@@ -0,0 +1,91 @@
+// Copyright (c) 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_BASE_ASYNC_DESTROY_VIDEO_DECODER_H_
+#define MEDIA_BASE_ASYNC_DESTROY_VIDEO_DECODER_H_
+
+#include <memory>
+#include <type_traits>
+#include "media/base/video_decoder.h"
+
+namespace media {
+
+// Some VideoDecoder implementations must do non-synchronous cleanup before
+// they are destroyed. This wrapper implementation allows a VideoDecoder
+// to schedule its own cleanup tasks before its memory is released.
+// The underlying type must implement a static
+// `DestroyAsync(std::unique_ptr<T>)` function which fires any pending
+// callbacks, stops and destroys the decoder. After this call, external
+// resources (e.g. raw pointers) held by the decoder might be invalidated
+// immediately. So if the decoder is destroyed asynchronously (e.g. DeleteSoon),
+// external resources must be released in this call.
+template <typename T>
+class AsyncDestroyVideoDecoder final : public VideoDecoder {
+ public:
+  explicit AsyncDestroyVideoDecoder(std::unique_ptr<T> wrapped_decoder)
+      : wrapped_decoder_(std::move(wrapped_decoder)) {
+    static_assert(std::is_base_of<VideoDecoder, T>::value,
+                  "T must implement 'media::VideoDecoder'");
+    DCHECK(wrapped_decoder_);
+  }
+
+  ~AsyncDestroyVideoDecoder() override {
+    if (wrapped_decoder_)
+      T::DestroyAsync(std::move(wrapped_decoder_));
+  }
+
+  std::string GetDisplayName() const override {
+    DCHECK(wrapped_decoder_);
+    return wrapped_decoder_->GetDisplayName();
+  }
+
+  bool IsPlatformDecoder() const override {
+    DCHECK(wrapped_decoder_);
+    return wrapped_decoder_->IsPlatformDecoder();
+  }
+
+  void Initialize(const VideoDecoderConfig& config,
+                  bool low_delay,
+                  CdmContext* cdm_context,
+                  InitCB init_cb,
+                  const OutputCB& output_cb,
+                  const WaitingCB& waiting_cb) override {
+    DCHECK(wrapped_decoder_);
+    wrapped_decoder_->Initialize(config, low_delay, cdm_context,
+                                 std::move(init_cb), output_cb, waiting_cb);
+  }
+
+  void Decode(scoped_refptr<DecoderBuffer> buffer,
+              DecodeCB decode_cb) override {
+    DCHECK(wrapped_decoder_);
+    wrapped_decoder_->Decode(std::move(buffer), std::move(decode_cb));
+  }
+
+  void Reset(base::OnceClosure closure) override {
+    DCHECK(wrapped_decoder_);
+    wrapped_decoder_->Reset(std::move(closure));
+  }
+
+  bool NeedsBitstreamConversion() const override {
+    DCHECK(wrapped_decoder_);
+    return wrapped_decoder_->NeedsBitstreamConversion();
+  }
+
+  bool CanReadWithoutStalling() const override {
+    DCHECK(wrapped_decoder_);
+    return wrapped_decoder_->CanReadWithoutStalling();
+  }
+
+  int GetMaxDecodeRequests() const override {
+    DCHECK(wrapped_decoder_);
+    return wrapped_decoder_->GetMaxDecodeRequests();
+  }
+
+ private:
+  std::unique_ptr<T> wrapped_decoder_;
+};
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_ASYNC_DESTROY_VIDEO_DECODER_H_

media/base/video_decoder.cc

@@ -15,10 +15,6 @@ namespace media {
 
 VideoDecoder::VideoDecoder() = default;
 
-void VideoDecoder::Destroy() {
-  delete this;
-}
-
 VideoDecoder::~VideoDecoder() = default;
 
 bool VideoDecoder::IsPlatformDecoder() const {
@@ -66,12 +62,3 @@ int VideoDecoder::GetRecommendedThreadCount(int desired_threads) {
 }
 
 }  // namespace media
-
-namespace std {
-
-void default_delete<media::VideoDecoder>::operator()(
-    media::VideoDecoder* ptr) const {
-  ptr->Destroy();
-}
-
-}  // namespace std

media/base/video_decoder.h

@@ -39,6 +39,7 @@ class MEDIA_EXPORT VideoDecoder {
   using DecodeCB = base::OnceCallback<void(DecodeStatus)>;
 
   VideoDecoder();
+  virtual ~VideoDecoder();
 
   // Returns the name of the decoder for logging and decoder selection purposes.
   // This name should be available immediately after construction (e.g. before
@@ -134,39 +135,9 @@ class MEDIA_EXPORT VideoDecoder {
   // [|limits::kMinVideoDecodeThreads|, |limits::kMaxVideoDecodeThreads|].
   static int GetRecommendedThreadCount(int desired_threads);
 
- protected:
-  // Deletion is only allowed via Destroy().
-  virtual ~VideoDecoder();
-
- private:
-  friend struct std::default_delete<VideoDecoder>;
-
-  // Fires any pending callbacks, stops and destroys the decoder. After this
-  // call, external resources (e.g. raw pointers) |this| holds might be
-  // invalidated immediately. So if the decoder is destroyed asynchronously
-  // (e.g. DeleteSoon), external resources must be released in this call.
-  virtual void Destroy();
-
   DISALLOW_COPY_AND_ASSIGN(VideoDecoder);
 };
 
 }  // namespace media
 
-namespace std {
-
-// Specialize std::default_delete to call Destroy().
-template <>
-struct MEDIA_EXPORT default_delete<media::VideoDecoder> {
-  constexpr default_delete() = default;
-
-  template <typename U,
-            typename = typename std::enable_if<
-                std::is_convertible<U*, media::VideoDecoder*>::value>::type>
-  default_delete(const default_delete<U>& d) {}
-
-  void operator()(media::VideoDecoder* ptr) const;
-};
-
-}  // namespace std
-
 #endif  // MEDIA_BASE_VIDEO_DECODER_H_

media/gpu/android/media_codec_video_decoder.cc

@@ -19,6 +19,7 @@
 #include "base/trace_event/trace_event.h"
 #include "media/base/android/media_codec_bridge_impl.h"
 #include "media/base/android/media_codec_util.h"
+#include "media/base/async_destroy_video_decoder.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/cdm_context.h"
 #include "media/base/decoder_buffer.h"
@@ -238,44 +239,69 @@ MediaCodecVideoDecoder::MediaCodecVideoDecoder(
                  weak_factory_.GetWeakPtr(), nullptr));
 }
 
+std::unique_ptr<VideoDecoder> MediaCodecVideoDecoder::Create(
+    const gpu::GpuPreferences& gpu_preferences,
+    const gpu::GpuFeatureInfo& gpu_feature_info,
+    std::unique_ptr<MediaLog> media_log,
+    DeviceInfo* device_info,
+    CodecAllocator* codec_allocator,
+    std::unique_ptr<AndroidVideoSurfaceChooser> surface_chooser,
+    AndroidOverlayMojoFactoryCB overlay_factory_cb,
+    RequestOverlayInfoCB request_overlay_info_cb,
+    std::unique_ptr<VideoFrameFactory> video_frame_factory) {
+  auto* decoder = new MediaCodecVideoDecoder(
+      gpu_preferences, gpu_feature_info, std::move(media_log), device_info,
+      codec_allocator, std::move(surface_chooser),
+      std::move(overlay_factory_cb), std::move(request_overlay_info_cb),
+      std::move(video_frame_factory));
+  return std::make_unique<AsyncDestroyVideoDecoder<MediaCodecVideoDecoder>>(
+      base::WrapUnique(decoder));
+}
+
 MediaCodecVideoDecoder::~MediaCodecVideoDecoder() {
   DVLOG(2) << __func__;
   TRACE_EVENT0("media", "MediaCodecVideoDecoder::~MediaCodecVideoDecoder");
   ReleaseCodec();
 }
 
-void MediaCodecVideoDecoder::Destroy() {
+void MediaCodecVideoDecoder::DestroyAsync(
+    std::unique_ptr<MediaCodecVideoDecoder> decoder) {
   DVLOG(1) << __func__;
   TRACE_EVENT0("media", "MediaCodecVideoDecoder::Destroy");
+  DCHECK(decoder);
+
+  // This will be destroyed by a call to |DeleteSoon|
+  // in |OnCodecDrained|.
+  auto* self = decoder.release();
 
   // Cancel pending callbacks.
   //
   // WARNING: This will lose the callback we've given to MediaCodecBridge for
   // asynchronous notifications; so we must not leave this function with any
   // work necessary from StartTimerOrPumpCodec().
-  weak_factory_.InvalidateWeakPtrs();
+  self->weak_factory_.InvalidateWeakPtrs();
 
-  if (media_crypto_context_) {
+  if (self->media_crypto_context_) {
     // Cancel previously registered callback (if any).
-    media_crypto_context_->SetMediaCryptoReadyCB(base::NullCallback());
-    if (cdm_registration_id_)
-      media_crypto_context_->UnregisterPlayer(cdm_registration_id_);
-    media_crypto_context_ = nullptr;
-    cdm_registration_id_ = 0;
+    self->media_crypto_context_->SetMediaCryptoReadyCB(base::NullCallback());
+    if (self->cdm_registration_id_)
+      self->media_crypto_context_->UnregisterPlayer(self->cdm_registration_id_);
+    self->media_crypto_context_ = nullptr;
+    self->cdm_registration_id_ = 0;
   }
 
   // Mojo callbacks require that they're run before destruction.
-  if (reset_cb_)
-    std::move(reset_cb_).Run();
+  if (self->reset_cb_)
+    std::move(self->reset_cb_).Run();
 
   // Cancel callbacks we no longer want.
-  codec_allocator_weak_factory_.InvalidateWeakPtrs();
-  CancelPendingDecodes(DecodeStatus::ABORTED);
-  StartDrainingCodec(DrainType::kForDestroy);
+  self->codec_allocator_weak_factory_.InvalidateWeakPtrs();
+  self->CancelPendingDecodes(DecodeStatus::ABORTED);
+  self->StartDrainingCodec(DrainType::kForDestroy);
 
   // Per the WARNING above. Validate that no draining work remains.
-  if (using_async_api_)
-    DCHECK(!drain_type_.has_value());
+  if (self->using_async_api_)
+    DCHECK(!self->drain_type_.has_value());
 }
 
 void MediaCodecVideoDecoder::Initialize(const VideoDecoderConfig& config,

media/gpu/android/media_codec_video_decoder.h

@@ -58,11 +58,14 @@ struct PendingDecode {
 // playbacks that need them.
 // TODO: Lazy initialization should be handled at a higher layer of the media
 // stack for both simplicity and cross platform support.
-class MEDIA_GPU_EXPORT MediaCodecVideoDecoder : public VideoDecoder {
+class MEDIA_GPU_EXPORT MediaCodecVideoDecoder final : public VideoDecoder {
  public:
   static std::vector<SupportedVideoDecoderConfig> GetSupportedConfigs();
 
-  MediaCodecVideoDecoder(
+  ~MediaCodecVideoDecoder() override;
+  static void DestroyAsync(std::unique_ptr<MediaCodecVideoDecoder>);
+
+  static std::unique_ptr<VideoDecoder> Create(
       const gpu::GpuPreferences& gpu_preferences,
       const gpu::GpuFeatureInfo& gpu_feature_info,
       std::unique_ptr<MediaLog> media_log,
@@ -87,9 +90,20 @@ class MEDIA_GPU_EXPORT MediaCodecVideoDecoder : public VideoDecoder {
   bool CanReadWithoutStalling() const override;
   int GetMaxDecodeRequests() const override;
 
- protected:
-  // Protected for testing.
-  ~MediaCodecVideoDecoder() override;
+ private:
+  // The test has access for PumpCodec() and the constructor.
+  friend class MediaCodecVideoDecoderTest;
+
+  MediaCodecVideoDecoder(
+      const gpu::GpuPreferences& gpu_preferences,
+      const gpu::GpuFeatureInfo& gpu_feature_info,
+      std::unique_ptr<MediaLog> media_log,
+      DeviceInfo* device_info,
+      CodecAllocator* codec_allocator,
+      std::unique_ptr<AndroidVideoSurfaceChooser> surface_chooser,
+      AndroidOverlayMojoFactoryCB overlay_factory_cb,
+      RequestOverlayInfoCB request_overlay_info_cb,
+      std::unique_ptr<VideoFrameFactory> video_frame_factory);
 
   // Set up |cdm_context| as part of initialization.  Guarantees that |init_cb|
   // will be called depending on the outcome, though not necessarily before this
@@ -102,11 +116,6 @@ class MEDIA_GPU_EXPORT MediaCodecVideoDecoder : public VideoDecoder {
                           JavaObjectPtr media_crypto,
                           bool requires_secure_video_codec);
 
- private:
-  // The test has access for PumpCodec().
-  friend class MediaCodecVideoDecoderTest;
-  friend class base::DeleteHelper<MediaCodecVideoDecoder>;
-
   enum class State {
     // Initializing resources required to create a codec.
     kInitializing,
@@ -124,9 +133,6 @@ class MEDIA_GPU_EXPORT MediaCodecVideoDecoder : public VideoDecoder {
 
   enum class DrainType { kForReset, kForDestroy };
 
-  // Starts teardown.
-  void Destroy() override;
-
   // Finishes initialization.
   void StartLazyInit();
   void OnVideoFrameFactoryInitialized(