Cast: Fix ExternalVideoEncoder early destruction scenario.

It's possible for ExternalVideoEncoder to get destroyed before it can
receive OnCreateVideoEncodeAccelerator callback. In that case the we
will crash, attempting to post LocalVideoEncodeAcceleratorClient::Destroy
to a still-NULL encoder_task_runner_. VEA::Destroy would still be
called, since it's called from the specialized destructor of VEA and
scopers would take care of this, but it may then be executed on
a wrong thread (i.e. not encoder_task_runner_).

Fix this by moving the VEA creation flow to LocalVideoEncodeAcceleratorClient
instead, which will outlive ExternalVideoEncoder even if it gets destroyed
before VEA finishes being created and properly destroy it in that case.

This also simplifies the creation flow by reducing argument passing between
classes.

Also add a unittest for this scenario.

BUG=
TEST=Cast browser_tests, cast_unittests, cast on CrOS

Review URL: https://codereview.chromium.org/447193003

Cr-Commit-Position: refs/heads/master@{#288763}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288763 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 150 insertions, 71 deletions

diff --git a/media/cast/sender/external_video_encoder.cc b/media/cast/sender/external_video_encoder.cc
index e3abecd..43e7973 100644
--- a/media/cast/sender/external_video_encoder.cc
+++ b/media/cast/sender/external_video_encoder.cc
@@ -34,25 +34,6 @@ void LogFrameEncodedEvent(
       event_time, media::cast::FRAME_ENCODED, media::cast::VIDEO_EVENT,
       rtp_timestamp, frame_id);
 }
-
-// Proxy this call to ExternalVideoEncoder on the cast main thread.
-void ProxyCreateVideoEncodeAccelerator(
-    const scoped_refptr<media::cast::CastEnvironment>& cast_environment,
-    const base::WeakPtr<media::cast::ExternalVideoEncoder>& weak_ptr,
-    const media::cast::CreateVideoEncodeMemoryCallback&
-        create_video_encode_mem_cb,
-    scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner,
-    scoped_ptr<media::VideoEncodeAccelerator> vea) {
-  cast_environment->PostTask(
-      media::cast::CastEnvironment::MAIN,
-      FROM_HERE,
-      base::Bind(
-          &media::cast::ExternalVideoEncoder::OnCreateVideoEncodeAccelerator,
-          weak_ptr,
-          create_video_encode_mem_cb,
-          encoder_task_runner,
-          base::Passed(&vea)));
-}
 }  // namespace
 
 namespace media {
@@ -76,20 +57,28 @@ class LocalVideoEncodeAcceleratorClient
     : public VideoEncodeAccelerator::Client,
       public base::RefCountedThreadSafe<LocalVideoEncodeAcceleratorClient> {
  public:
-  LocalVideoEncodeAcceleratorClient(
+  // Create an instance of this class and post a task to create
+  // video_encode_accelerator_. A ref to |this| will be kept, awaiting reply
+  // via ProxyCreateVideoEncodeAccelerator, which will provide us with the
+  // encoder task runner and vea instance. We cannot be destroyed until we
+  // receive the reply, otherwise the VEA object created may leak.
+  static scoped_refptr<LocalVideoEncodeAcceleratorClient> Create(
       scoped_refptr<CastEnvironment> cast_environment,
-      scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner,
-      scoped_ptr<media::VideoEncodeAccelerator> vea,
+      const CreateVideoEncodeAcceleratorCallback& create_vea_cb,
       const CreateVideoEncodeMemoryCallback& create_video_encode_mem_cb,
-      const base::WeakPtr<ExternalVideoEncoder>& weak_owner)
-      : cast_environment_(cast_environment),
-        encoder_task_runner_(encoder_task_runner),
-        video_encode_accelerator_(vea.Pass()),
-        create_video_encode_memory_cb_(create_video_encode_mem_cb),
-        weak_owner_(weak_owner),
-        last_encoded_frame_id_(kStartFrameId),
-        key_frame_encountered_(false) {
-    DCHECK(encoder_task_runner_);
+      const base::WeakPtr<ExternalVideoEncoder>& weak_owner) {
+    scoped_refptr<LocalVideoEncodeAcceleratorClient> client(
+        new LocalVideoEncodeAcceleratorClient(
+            cast_environment, create_video_encode_mem_cb, weak_owner));
+
+    // This will keep a ref to |client|, if weak_owner is destroyed before
+    // ProxyCreateVideoEncodeAccelerator is called, we will stay alive until
+    // we can properly destroy the VEA.
+    create_vea_cb.Run(base::Bind(
+        &LocalVideoEncodeAcceleratorClient::OnCreateVideoEncodeAcceleratorProxy,
+        client));
+
+    return client;
   }
 
   // Initialize the real HW encoder.
@@ -128,12 +117,22 @@ class LocalVideoEncodeAcceleratorClient
     // initialized.
   }
 
-  // Free the HW.
+  // Destroy the VEA on the correct thread.
   void Destroy() {
     DCHECK(encoder_task_runner_);
-    DCHECK(encoder_task_runner_->RunsTasksOnCurrentThread());
+    if (!video_encode_accelerator_)
+      return;
 
-    video_encode_accelerator_.reset();
+    if (encoder_task_runner_->RunsTasksOnCurrentThread()) {
+      video_encode_accelerator_.reset();
+    } else {
+      // We do this instead of just reposting to encoder_task_runner_, because
+      // we are called from the destructor.
+      encoder_task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&DestroyVideoEncodeAcceleratorOnEncoderThread,
+                     base::Passed(&video_encode_accelerator_)));
+    }
   }
 
   void SetBitRate(uint32 bit_rate) {
@@ -165,7 +164,6 @@ class LocalVideoEncodeAcceleratorClient
     DCHECK(encoder_task_runner_->RunsTasksOnCurrentThread());
     VLOG(1) << "ExternalVideoEncoder NotifyError: " << error;
 
-    video_encode_accelerator_.reset();
     cast_environment_->PostTask(
         CastEnvironment::MAIN,
         FROM_HERE,
@@ -269,6 +267,46 @@ class LocalVideoEncodeAcceleratorClient
   }
 
  private:
+  LocalVideoEncodeAcceleratorClient(
+      scoped_refptr<CastEnvironment> cast_environment,
+      const CreateVideoEncodeMemoryCallback& create_video_encode_mem_cb,
+      const base::WeakPtr<ExternalVideoEncoder>& weak_owner)
+      : cast_environment_(cast_environment),
+        create_video_encode_memory_cb_(create_video_encode_mem_cb),
+        weak_owner_(weak_owner),
+        last_encoded_frame_id_(kStartFrameId),
+        key_frame_encountered_(false) {}
+
+  // Trampoline VEA creation callback to OnCreateVideoEncodeAccelerator()
+  // on encoder_task_runner. Normally we would just repost the same method to
+  // it, and would not need a separate proxy method, but we can't
+  // ThreadTaskRunnerHandle::Get() in unittests just yet.
+  void OnCreateVideoEncodeAcceleratorProxy(
+      scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner,
+      scoped_ptr<media::VideoEncodeAccelerator> vea) {
+    encoder_task_runner->PostTask(
+        FROM_HERE,
+        base::Bind(&media::cast::LocalVideoEncodeAcceleratorClient::
+                       OnCreateVideoEncodeAccelerator,
+                   this,
+                   encoder_task_runner,
+                   base::Passed(&vea)));
+  }
+
+  void OnCreateVideoEncodeAccelerator(
+      scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner,
+      scoped_ptr<media::VideoEncodeAccelerator> vea) {
+    encoder_task_runner_ = encoder_task_runner;
+    video_encode_accelerator_.reset(vea.release());
+
+    cast_environment_->PostTask(
+        CastEnvironment::MAIN,
+        FROM_HERE,
+        base::Bind(&ExternalVideoEncoder::OnCreateVideoEncodeAccelerator,
+                   weak_owner_,
+                   encoder_task_runner_));
+  }
+
   // Note: This method can be called on any thread.
   void OnCreateSharedMemory(scoped_ptr<base::SharedMemory> memory) {
     encoder_task_runner_->PostTask(
@@ -302,9 +340,17 @@ class LocalVideoEncodeAcceleratorClient
         base::Bind(&ExternalVideoEncoder::EncoderInitialized, weak_owner_));
   }
 
+  static void DestroyVideoEncodeAcceleratorOnEncoderThread(
+      scoped_ptr<media::VideoEncodeAccelerator> vea) {
+    // VEA::~VEA specialization takes care of calling Destroy() on the VEA impl.
+  }
+
   friend class base::RefCountedThreadSafe<LocalVideoEncodeAcceleratorClient>;
 
-  virtual ~LocalVideoEncodeAcceleratorClient() {}
+  virtual ~LocalVideoEncodeAcceleratorClient() {
+    Destroy();
+    DCHECK(!video_encode_accelerator_);
+  }
 
   const scoped_refptr<CastEnvironment> cast_environment_;
   scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner_;
@@ -337,17 +383,15 @@ ExternalVideoEncoder::ExternalVideoEncoder(
       weak_factory_(this) {
   DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
 
-  create_vea_cb.Run(base::Bind(&ProxyCreateVideoEncodeAccelerator,
-                               cast_environment,
-                               weak_factory_.GetWeakPtr(),
-                               create_video_encode_mem_cb));
+  video_accelerator_client_ =
+      LocalVideoEncodeAcceleratorClient::Create(cast_environment_,
+                                                create_vea_cb,
+                                                create_video_encode_mem_cb,
+                                                weak_factory_.GetWeakPtr());
+  DCHECK(video_accelerator_client_);
 }
 
 ExternalVideoEncoder::~ExternalVideoEncoder() {
-  encoder_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&LocalVideoEncodeAcceleratorClient::Destroy,
-                 video_accelerator_client_));
 }
 
 void ExternalVideoEncoder::EncoderInitialized() {
@@ -361,18 +405,10 @@ void ExternalVideoEncoder::EncoderError() {
 }
 
 void ExternalVideoEncoder::OnCreateVideoEncodeAccelerator(
-    const CreateVideoEncodeMemoryCallback& create_video_encode_mem_cb,
-    scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner,
-    scoped_ptr<media::VideoEncodeAccelerator> vea) {
+    scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner) {
   DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
   encoder_task_runner_ = encoder_task_runner;
 
-  video_accelerator_client_ =
-      new LocalVideoEncodeAcceleratorClient(cast_environment_,
-                                            encoder_task_runner,
-                                            vea.Pass(),
-                                            create_video_encode_mem_cb,
-                                            weak_factory_.GetWeakPtr());
   encoder_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&LocalVideoEncodeAcceleratorClient::Initialize,
@@ -429,6 +465,5 @@ void ExternalVideoEncoder::GenerateKeyFrame() {
 void ExternalVideoEncoder::LatestFrameIdToReference(uint32 /*frame_id*/) {
   // Do nothing not supported.
 }
-
 }  //  namespace cast
 }  //  namespace media
diff --git a/media/cast/sender/external_video_encoder.h b/media/cast/sender/external_video_encoder.h
index 84de7f0..269fb3e 100644
--- a/media/cast/sender/external_video_encoder.h
+++ b/media/cast/sender/external_video_encoder.h
@@ -50,11 +50,10 @@ class ExternalVideoEncoder : public VideoEncoder {
   virtual void GenerateKeyFrame() OVERRIDE;
   virtual void LatestFrameIdToReference(uint32 frame_id) OVERRIDE;
 
-  // Called when a VEA is created.
+  // Called when video_accelerator_client_ has finished creating the VEA and
+  // is ready for use.
   void OnCreateVideoEncodeAccelerator(
-      const CreateVideoEncodeMemoryCallback& create_video_encode_mem_cb,
-      scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner,
-      scoped_ptr<media::VideoEncodeAccelerator> vea);
+      scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner);
 
  protected:
   void EncoderInitialized();
diff --git a/media/cast/sender/external_video_encoder_unittest.cc b/media/cast/sender/external_video_encoder_unittest.cc
index 9461a11..385b121 100644
--- a/media/cast/sender/external_video_encoder_unittest.cc
+++ b/media/cast/sender/external_video_encoder_unittest.cc
@@ -23,12 +23,26 @@ using testing::_;
 
 namespace {
 
-void CreateVideoEncodeAccelerator(
-    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
-    scoped_ptr<VideoEncodeAccelerator> fake_vea,
-    const ReceiveVideoEncodeAcceleratorCallback& callback) {
-  callback.Run(task_runner, fake_vea.Pass());
-}
+class VEAFactory {
+ public:
+  VEAFactory(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+             scoped_ptr<VideoEncodeAccelerator> vea)
+      : task_runner_(task_runner), vea_(vea.Pass()) {}
+
+  void CreateVideoEncodeAccelerator(
+      const ReceiveVideoEncodeAcceleratorCallback& callback) {
+    create_cb_ = callback;
+  }
+
+  void FinishCreatingVideoEncodeAccelerator() {
+    create_cb_.Run(task_runner_, vea_.Pass());
+  }
+
+ private:
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  scoped_ptr<VideoEncodeAccelerator> vea_;
+  ReceiveVideoEncodeAcceleratorCallback create_cb_;
+};
 
 void CreateSharedMemory(
     size_t size, const ReceiveVideoEncodeMemoryCallback& callback) {
@@ -116,13 +130,14 @@ class ExternalVideoEncoderTest : public ::testing::Test {
     fake_vea_ = new test::FakeVideoEncodeAccelerator(task_runner_,
                                                      &stored_bitrates_);
     scoped_ptr<VideoEncodeAccelerator> fake_vea(fake_vea_);
-    video_encoder_.reset(
-        new ExternalVideoEncoder(cast_environment_,
-                                 video_config_,
-                                 base::Bind(&CreateVideoEncodeAccelerator,
-                                            task_runner_,
-                                            base::Passed(&fake_vea)),
-                                 base::Bind(&CreateSharedMemory)));
+    VEAFactory vea_factory(task_runner_, fake_vea.Pass());
+    video_encoder_.reset(new ExternalVideoEncoder(
+        cast_environment_,
+        video_config_,
+        base::Bind(&VEAFactory::CreateVideoEncodeAccelerator,
+                   base::Unretained(&vea_factory)),
+        base::Bind(&CreateSharedMemory)));
+    vea_factory.FinishCreatingVideoEncodeAccelerator();
   }
 
   virtual ~ExternalVideoEncoderTest() {}
@@ -193,5 +208,35 @@ TEST_F(ExternalVideoEncoderTest, StreamHeader) {
   task_runner_->RunTasks();
 }
 
+// Verify that everything goes well even if ExternalVideoEncoder is destroyed
+// before it has a chance to receive the VEA creation callback.
+TEST(ExternalVideoEncoderEarlyDestroyTest, DestroyBeforeVEACreatedCallback) {
+  VideoSenderConfig video_config;
+  base::SimpleTestTickClock* testing_clock = new base::SimpleTestTickClock();
+  scoped_refptr<test::FakeSingleThreadTaskRunner> task_runner(
+      new test::FakeSingleThreadTaskRunner(testing_clock));
+  scoped_refptr<CastEnvironment> cast_environment(
+      new CastEnvironment(scoped_ptr<base::TickClock>(testing_clock).Pass(),
+                          task_runner,
+                          task_runner,
+                          task_runner));
+
+  std::vector<uint32> stored_bitrates;
+  scoped_ptr<VideoEncodeAccelerator> fake_vea(
+      new test::FakeVideoEncodeAccelerator(task_runner, &stored_bitrates));
+  VEAFactory vea_factory(task_runner, fake_vea.Pass());
+
+  scoped_ptr<ExternalVideoEncoder> video_encoder(new ExternalVideoEncoder(
+      cast_environment,
+      video_config,
+      base::Bind(&VEAFactory::CreateVideoEncodeAccelerator,
+                 base::Unretained(&vea_factory)),
+      base::Bind(&CreateSharedMemory)));
+
+  video_encoder.reset();
+  vea_factory.FinishCreatingVideoEncodeAccelerator();
+  task_runner->RunTasks();
+}
+
 }  // namespace cast
 }  // namespace media