media/gpu/V4L2VEA: Post task to encoder_thread_task_runner with WeakPtr of this

At V4L2VEA::Destroy(), encoder thread process all the pending tasks. If
PumpBitstreamBuffers() is in the pending tasks, Enqueue() is posted and
processed after DestroyTask(). That leads |input_queue_| and |output_queue_| are
nullptr and causes SIGSEGV.

This fixes the issue by posting tasks to encoder_thread_.task_runner() with
WeakPtr of this. This makes V4L2VEA not execute any tasks posted to the task
runner after DestroyTask() because the WeakPtr is invalidated in DestroyTask().

Bug: 1019437
Test: VEA unittest on guado with dcheck_always_on=true
Change-Id: Ie2e43d1dd9c74114cbd0d2b51e260213b60136e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1890462
Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Chih-Yu Huang <akahuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711148}

media/gpu/v4l2/v4l2_video_encode_accelerator.cc

@@ -216,9 +216,8 @@ bool V4L2VideoEncodeAccelerator::Initialize(const Config& config,
   bool result = false;
   base::WaitableEvent done;
   encoder_thread_.task_runner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&V4L2VideoEncodeAccelerator::InitializeTask,
-                     base::Unretained(this), config, &result, &done));
+      FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::InitializeTask,
+                                weak_this_, config, &result, &done));
   done.Wait();
   return result;
 }
@@ -317,11 +316,8 @@ bool V4L2VideoEncodeAccelerator::CreateImageProcessor(
       {ImageProcessor::OutputMode::ALLOCATE,
        ImageProcessor::OutputMode::IMPORT},
       kImageProcBufferCount,
-      // Unretained(this) is safe here, because image_processor is destroyed
-      // before video_encoder_thread stops.
-      BindToCurrentLoop(
-          base::BindRepeating(&V4L2VideoEncodeAccelerator::ImageProcessorError,
-                              base::Unretained(this))));
+      BindToCurrentLoop(base::BindRepeating(
+          &V4L2VideoEncodeAccelerator::ImageProcessorError, weak_this_)));
   if (!image_processor_) {
     VLOGF(1) << "Failed initializing image processor";
     return false;
@@ -421,9 +417,8 @@ void V4L2VideoEncodeAccelerator::Encode(scoped_refptr<VideoFrame> frame,
   DCHECK(child_task_runner_->BelongsToCurrentThread());
 
   encoder_thread_.task_runner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&V4L2VideoEncodeAccelerator::EncodeTask,
-                     base::Unretained(this), std::move(frame), force_keyframe));
+      FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::EncodeTask,
+                                weak_this_, std::move(frame), force_keyframe));
 }
 
 void V4L2VideoEncodeAccelerator::UseOutputBitstreamBuffer(
@@ -434,7 +429,7 @@ void V4L2VideoEncodeAccelerator::UseOutputBitstreamBuffer(
   encoder_thread_.task_runner()->PostTask(
       FROM_HERE,
       base::BindOnce(&V4L2VideoEncodeAccelerator::UseOutputBitstreamBufferTask,
-                     base::Unretained(this), std::move(buffer)));
+                     weak_this_, std::move(buffer)));
 }
 
 void V4L2VideoEncodeAccelerator::RequestEncodingParametersChange(
@@ -447,7 +442,7 @@ void V4L2VideoEncodeAccelerator::RequestEncodingParametersChange(
       FROM_HERE,
       base::BindOnce(
           &V4L2VideoEncodeAccelerator::RequestEncodingParametersChangeTask,
-          base::Unretained(this), bitrate, framerate));
+          weak_this_, bitrate, framerate));
 }
 
 void V4L2VideoEncodeAccelerator::Destroy() {
@@ -456,13 +451,12 @@ void V4L2VideoEncodeAccelerator::Destroy() {
 
   // We're destroying; cancel all callbacks.
   client_ptr_factory_.reset();
-  weak_this_ptr_factory_.InvalidateWeakPtrs();
 
   // If the encoder thread is running, destroy using posted task.
   if (encoder_thread_.IsRunning()) {
     encoder_thread_.task_runner()->PostTask(
-        FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::DestroyTask,
-                                  base::Unretained(this)));
+        FROM_HERE,
+        base::BindOnce(&V4L2VideoEncodeAccelerator::DestroyTask, weak_this_));
     // DestroyTask() will put the encoder into kError state and cause all tasks
     // to no-op.
     encoder_thread_.Stop();
@@ -486,9 +480,8 @@ void V4L2VideoEncodeAccelerator::Flush(FlushCallback flush_callback) {
   DCHECK(child_task_runner_->BelongsToCurrentThread());
 
   encoder_thread_.task_runner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(&V4L2VideoEncodeAccelerator::FlushTask,
-                     base::Unretained(this), std::move(flush_callback)));
+      FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::FlushTask,
+                                weak_this_, std::move(flush_callback)));
 }
 
 void V4L2VideoEncodeAccelerator::FlushTask(FlushCallback flush_callback) {
@@ -533,8 +526,8 @@ void V4L2VideoEncodeAccelerator::FrameProcessed(
   encoder_input_queue_.emplace(std::move(frame), force_keyframe,
                                output_buffer_index);
   encoder_thread_.task_runner()->PostTask(
-      FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::Enqueue,
-                                base::Unretained(this)));
+      FROM_HERE,
+      base::BindOnce(&V4L2VideoEncodeAccelerator::Enqueue, weak_this_));
 }
 
 void V4L2VideoEncodeAccelerator::ReuseImageProcessorOutputBuffer(
@@ -723,6 +716,7 @@ bool V4L2VideoEncodeAccelerator::ReconfigureFormatIfNeeded(
 }
 
 void V4L2VideoEncodeAccelerator::InputImageProcessorTask() {
+  DCHECK(encoder_thread_.task_runner()->BelongsToCurrentThread());
   if (free_image_processor_output_buffer_indices_.empty())
     return;
   if (image_processor_input_queue_.empty())
@@ -741,22 +735,18 @@ void V4L2VideoEncodeAccelerator::InputImageProcessorTask() {
     auto output_frame = VideoFrame::WrapVideoFrame(
         buf, buf->format(), buf->visible_rect(), buf->natural_size());
 
-    // Unretained(this) is safe here, because image_processor is destroyed
-    // before video_encoder_thread stops.
     if (!image_processor_->Process(
             std::move(frame), std::move(output_frame),
-            BindToCurrentLoop(
-                base::BindOnce(&V4L2VideoEncodeAccelerator::FrameProcessed,
-                               base::Unretained(this), force_keyframe,
-                               timestamp, output_buffer_index)))) {
+            BindToCurrentLoop(base::BindOnce(
+                &V4L2VideoEncodeAccelerator::FrameProcessed, weak_this_,
+                force_keyframe, timestamp, output_buffer_index)))) {
       NOTIFY_ERROR(kPlatformFailureError);
     }
   } else {
     if (!image_processor_->Process(
-            std::move(frame),
-            BindToCurrentLoop(base::BindOnce(
-                &V4L2VideoEncodeAccelerator::FrameProcessed,
-                base::Unretained(this), force_keyframe, timestamp)))) {
+            std::move(frame), BindToCurrentLoop(base::BindOnce(
+                                  &V4L2VideoEncodeAccelerator::FrameProcessed,
+                                  weak_this_, force_keyframe, timestamp)))) {
       NOTIFY_ERROR(kPlatformFailureError);
     }
   }
@@ -792,6 +782,10 @@ void V4L2VideoEncodeAccelerator::UseOutputBitstreamBufferTask(
 void V4L2VideoEncodeAccelerator::DestroyTask() {
   VLOGF(2);
 
+  // Make tasks posted to encoder_thread_.task_runner() after DestroyTask() not
+  // be executed.
+  weak_this_ptr_factory_.InvalidateWeakPtrs();
+
   // DestroyTask() should run regardless of encoder_state_.
 
   // Stop streaming and the device_poll_thread_.
@@ -841,6 +835,8 @@ void V4L2VideoEncodeAccelerator::ServiceDeviceTask() {
   //   already.
   DCHECK(device_poll_thread_.task_runner());
   // Queue the DevicePollTask() now.
+  // base::Unretained(this) is safe, because device_poll_thread_ is owned by
+  // *this and stops before *this destruction.
   device_poll_thread_.task_runner()->PostTask(
       FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::DevicePollTask,
                                 base::Unretained(this), poll_device));
@@ -857,11 +853,10 @@ void V4L2VideoEncodeAccelerator::ServiceDeviceTask() {
 
 void V4L2VideoEncodeAccelerator::Enqueue() {
   DCHECK(encoder_thread_.task_runner()->BelongsToCurrentThread());
+  DCHECK(input_queue_ && output_queue_);
   TRACE_EVENT0("media,gpu", "V4L2VEA::Enqueue");
-
   DVLOGF(4) << "free_input_buffers: " << input_queue_->FreeBuffersCount()
             << "input_queue: " << encoder_input_queue_.size();
-
   bool do_streamon = false;
   // Enqueue all the inputs we can.
   const size_t old_inputs_queued = input_queue_->QueuedBuffersCount();
@@ -1048,8 +1043,8 @@ void V4L2VideoEncodeAccelerator::PumpBitstreamBuffers() {
   // We may free some V4L2 output buffers above. Enqueue them if needed.
   if (output_queue_->FreeBuffersCount() > 0) {
     encoder_thread_.task_runner()->PostTask(
-        FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::Enqueue,
-                                  base::Unretained(this)));
+        FROM_HERE,
+        base::BindOnce(&V4L2VideoEncodeAccelerator::Enqueue, weak_this_));
   }
 }
 
@@ -1206,6 +1201,8 @@ bool V4L2VideoEncodeAccelerator::StartDevicePoll() {
   }
   // Enqueue a poll task with no devices to poll on -- it will wait only on the
   // interrupt fd.
+  // base::Unretained(this) is safe, because device_poll_thread_ is owned by
+  // *this and stops before *this destruction.
   device_poll_thread_.task_runner()->PostTask(
       FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::DevicePollTask,
                                 base::Unretained(this), false));
@@ -1261,23 +1258,25 @@ void V4L2VideoEncodeAccelerator::DevicePollTask(bool poll_device) {
   // touch encoder state from this thread.
   encoder_thread_.task_runner()->PostTask(
       FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::ServiceDeviceTask,
-                                base::Unretained(this)));
+                                weak_this_));
 }
 
 void V4L2VideoEncodeAccelerator::NotifyError(Error error) {
+  // Note that NotifyError() must be called from SetErrorState() only, so that
+  // NotifyError() will not be called twice.
   VLOGF(1) << "error=" << error;
-
-  if (!child_task_runner_->BelongsToCurrentThread()) {
-    child_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::NotifyError,
-                                  weak_this_, error));
+  DCHECK(child_task_runner_);
+  if (child_task_runner_->BelongsToCurrentThread()) {
+    if (client_) {
+      client_->NotifyError(error);
+      client_ptr_factory_.reset();
+    }
     return;
   }
 
-  if (client_) {
-    client_->NotifyError(error);
-    client_ptr_factory_.reset();
-  }
+  // Called on encoder_thread_.task_runner().
+  child_task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&Client::NotifyError, client_, error));
 }
 
 void V4L2VideoEncodeAccelerator::SetErrorState(Error error) {
@@ -1288,7 +1287,7 @@ void V4L2VideoEncodeAccelerator::SetErrorState(Error error) {
   if (task_runner && !task_runner->BelongsToCurrentThread()) {
     task_runner->PostTask(
         FROM_HERE, base::BindOnce(&V4L2VideoEncodeAccelerator::SetErrorState,
-                                  base::Unretained(this), error));
+                                  weak_this_, error));
     return;
   }
 

media/gpu/v4l2/v4l2_video_encode_accelerator.h

@@ -347,10 +347,9 @@ class MEDIA_GPU_EXPORT V4L2VideoEncodeAccelerator
   base::WeakPtr<Client> client_;
   std::unique_ptr<base::WeakPtrFactory<Client>> client_ptr_factory_;
 
-  // WeakPtr<> pointing to |this| for use in posting tasks from the
-  // image_processor_ back to the child thread.
-  // Tasks posted onto encoder and poll threads can use base::Unretained(this),
-  // as both threads will not outlive this object.
+  // WeakPtr<> pointing to |this| for use in posting tasks to
+  // |encoder_thread_.task_runner()|. It guarantees no task will be executed
+  // after DestroyTask().
   base::WeakPtr<V4L2VideoEncodeAccelerator> weak_this_;
   base::WeakPtrFactory<V4L2VideoEncodeAccelerator> weak_this_ptr_factory_;