Move destruction of VEAEncoder to encoding task runner

content/renderer/media_recorder/video_track_recorder.cc

 // Copyright 2015 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.
 
 #include "content/renderer/media_recorder/video_track_recorder.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 431 matching lines @@
 
   void UseOutputBitstreamBufferId(int32_t bitstream_buffer_id);
   void FrameFinished(std::unique_ptr<base::SharedMemory> shm);
 
   // VideoTrackRecorder::Encoder implementation.
   ~VEAEncoder() override;
   void EncodeOnEncodingTaskRunner(scoped_refptr<VideoFrame> frame,
                                   base::TimeTicks capture_timestamp) override;
   void ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) override;
 
+  void DestroyOnEncodingTaskRunner(base::WaitableEvent* async_waiter);
+
   media::GpuVideoAcceleratorFactories* const gpu_factories_;
 
   const media::VideoCodecProfile codec_;
 
   // The underlying VEA to perform encoding on.
   std::unique_ptr<media::VideoEncodeAccelerator> video_encoder_;
 
   // Shared memory buffers for output with the VEA.
   std::vector<std::unique_ptr<base::SharedMemory>> output_buffers_;
 
@@ skipping 116 matching lines @@
   DCHECK(gpu_factories_);
   DCHECK_GE(size.width(), kVEAEncoderMinResolutionWidth);
   DCHECK_GE(size.height(), kVEAEncoderMinResolutionHeight);
 
   encoding_task_runner_->PostTask(
       FROM_HERE, base::Bind(&VEAEncoder::ConfigureEncoderOnEncodingTaskRunner,
                             this, size));
 }
 
 VEAEncoder::~VEAEncoder() {
+  DVLOG(3) << __func__;

[mcasas, 2017/03/15 01:14:17]

nit: remove the DVLOG()s

[emircan, 2017/03/15 01:36:18]

Done.

+
+  base::WaitableEvent release_waiter(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  // base::Unretained is safe because the class will be alive until
+  // |release_waiter| is signaled.

[mcasas, 2017/03/15 01:14:17]

I (still) think we need a comment here saying that
it's unsafe to just do a
  encoding_task_runner_->DeleteSoon(FROM_HERE, video_encoder_.release());
because |video_encoder_| holds a naked pointer to
us in |encoding_task_runner_|.

TODO+BUG point this situation out or not, I'll leave it
to you :-)
If VEA is staying alive in strange ways because of IPC,
mojo and its clear lifetime semantics can be of help
#justsayin'

[emircan, 2017/03/15 01:36:18]

Done.

   encoding_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&media::VideoEncodeAccelerator::Destroy,
-                            base::Unretained(video_encoder_.release())));
+      FROM_HERE, base::Bind(&VEAEncoder::DestroyOnEncodingTaskRunner,
+                            base::Unretained(this), &release_waiter));
+  release_waiter.Wait();
 }
 
 void VEAEncoder::RequireBitstreamBuffers(unsigned int /*input_count*/,
                                          const gfx::Size& input_coded_size,
                                          size_t output_buffer_size) {
   DVLOG(3) << __func__;
   DCHECK(encoding_task_runner_->BelongsToCurrentThread());
 
   vea_requested_input_size_ = input_coded_size;
   output_buffers_.clear();
@@ skipping 136 matching lines @@
                      video_frame->stride(media::VideoFrame::kYPlane),
                      video_frame->visible_data(media::VideoFrame::kUPlane),
                      video_frame->stride(media::VideoFrame::kUPlane),
                      video_frame->visible_data(media::VideoFrame::kVPlane),
                      video_frame->stride(media::VideoFrame::kVPlane),
                      input_size_.width(), input_size_.height());
   }
   frames_in_encode_.push(std::make_pair(
       media::WebmMuxer::VideoParameters(frame), capture_timestamp));
 
-  encoding_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&media::VideoEncodeAccelerator::Encode,
-                 base::Unretained(video_encoder_.get()), video_frame, false));
+  video_encoder_->Encode(video_frame, false);
 }
 
 void VEAEncoder::ConfigureEncoderOnEncodingTaskRunner(const gfx::Size& size) {
   DVLOG(3) << __func__;
   DCHECK(encoding_task_runner_->BelongsToCurrentThread());
   DCHECK(gpu_factories_->GetTaskRunner()->BelongsToCurrentThread());
   DCHECK_GT(bits_per_second_, 0);
 
   input_size_ = size;
   video_encoder_ = gpu_factories_->CreateVideoEncodeAccelerator();
   if (!video_encoder_ ||
       !video_encoder_->Initialize(media::PIXEL_FORMAT_I420, input_size_, codec_,
                                   bits_per_second_, this)) {
     NotifyError(media::VideoEncodeAccelerator::kPlatformFailureError);
   }
 }
 
+void VEAEncoder::DestroyOnEncodingTaskRunner(
+    base::WaitableEvent* async_waiter) {
+  DVLOG(3) << __func__;
+  DCHECK(encoding_task_runner_->BelongsToCurrentThread());

[mcasas, 2017/03/15 01:14:17]

This one as well.

[emircan, 2017/03/15 01:36:18]

Done.

+  video_encoder_.reset();
+  async_waiter->Signal();
+}
+
 // static
 void VpxEncoder::ShutdownEncoder(std::unique_ptr<base::Thread> encoding_thread,
                                  ScopedVpxCodecCtxPtr encoder) {
   DCHECK(encoding_thread->IsRunning());
   encoding_thread->Stop();
   // Both |encoding_thread| and |encoder| will be destroyed at end-of-scope.
 }
 
 VpxEncoder::VpxEncoder(
     bool use_vp9,
@@ skipping 452 matching lines @@
     encoder_->SetPaused(paused_before_init_);
 
   // StartFrameEncode() will be called on Render IO thread.
   MediaStreamVideoSink::ConnectToTrack(
       track_,
       base::Bind(&VideoTrackRecorder::Encoder::StartFrameEncode, encoder_),
       false);
 }
 
 }  // namespace content