media/gpu/v4l2: Fix V4L2ImageProcessor Reset().

Currently, in Reset() we only call TryCancelAll() on the client
sequence to cancel tasks on device sequence. However, TryCancelAll()
cancels tasks in a best-effort, meaning a few tasks on device sequence
can still run. The tasks posts new tasks to the client sequence.
Therefore, V4L2ImageProcessor might still return previous frames after
Reset() is called.

In this CL, we have these changes:
1. Post additional task ResetTask() to device sequence after
   cancelling previous tasks.
2. Clear |running_jobs_| at ResetTask(), and check the dequeued buffer
   id at Dequeue(). Only return the buffers at |running_jobs_|.
3. Invalidate WeakPtr of the client sequence to cancel pending tasks
   on the client sequence.

With 1. and 2., we could guarantee that after ResetTask(), we don't
post task to |client_task_runner_| for returning frames. With 3., we
could guarantee the tasks on |client_task_runner_| posted by the tasks
on |device_task_runner_| prior to ResetTask() are cancelled.

Bug: 1004727
Test: Run video_decode_accelerator_tests on Kukui and Hana
Test: Run video_encode_accelerator_unittest on Kukui and Hana

Change-Id: Ic2f8084f7e7658488dac9e7d39b0f22be347795e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906858
Commit-Queue: Chih-Yu Huang <akahuang@chromium.org>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715612}

media/gpu/v4l2/v4l2_image_processor.cc

@@ -9,6 +9,8 @@
 #include <sys/eventfd.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
+
+#include <limits>
 #include <tuple>
 #include <utility>
 
@@ -51,7 +53,8 @@
 
 namespace media {
 
-V4L2ImageProcessor::JobRecord::JobRecord() = default;
+V4L2ImageProcessor::JobRecord::JobRecord()
+    : output_buffer_id(std::numeric_limits<size_t>::max()) {}
 
 V4L2ImageProcessor::JobRecord::~JobRecord() = default;
 
@@ -513,13 +516,34 @@ void V4L2ImageProcessor::ProcessJobsTask() {
 }
 
 bool V4L2ImageProcessor::Reset() {
-  VLOGF(2);
+  DVLOGF(3);
   DCHECK_CALLED_ON_VALID_SEQUENCE(client_sequence_checker_);
 
   process_task_tracker_.TryCancelAll();
+  base::WaitableEvent event;
+  device_task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&V4L2ImageProcessor::ResetTask,
+                                device_weak_this_, base::Unretained(&event)));
+  event.Wait();
+
+  // Then cancel pending tasks on |client_task_runner_| to avoid returning
+  // frames after reset.
+  client_weak_this_factory_.InvalidateWeakPtrs();
+  client_weak_this_ = client_weak_this_factory_.GetWeakPtr();
+
   return true;
 }
 
+void V4L2ImageProcessor::ResetTask(base::WaitableEvent* event) {
+  DVLOGF(3);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(device_sequence_checker_);
+
+  input_job_queue_ = {};
+  running_jobs_ = {};
+
+  event->Signal();
+}
+
 bool V4L2ImageProcessor::CreateInputBuffers() {
   VLOGF(2);
   DCHECK_CALLED_ON_VALID_SEQUENCE(device_sequence_checker_);
@@ -699,7 +723,7 @@ void V4L2ImageProcessor::EnqueueInput(const JobRecord* job_record) {
   }
 }
 
-void V4L2ImageProcessor::EnqueueOutput(const JobRecord* job_record) {
+void V4L2ImageProcessor::EnqueueOutput(JobRecord* job_record) {
   DVLOGF(4);
   DCHECK_CALLED_ON_VALID_SEQUENCE(device_sequence_checker_);
   DCHECK(output_queue_);
@@ -792,7 +816,11 @@ void V4L2ImageProcessor::Dequeue() {
     }
 
     // Jobs are always processed in FIFO order.
-    DCHECK(!running_jobs_.empty());
+    if (running_jobs_.empty() ||
+        running_jobs_.front()->output_buffer_id != buffer->BufferId()) {
+      DVLOGF(3) << "previous Reset() abondoned the job, ignore.";
+      continue;
+    }
     std::unique_ptr<JobRecord> job_record = std::move(running_jobs_.front());
     running_jobs_.pop();
 
@@ -825,19 +853,31 @@ void V4L2ImageProcessor::Dequeue() {
 
     output_frame->set_timestamp(job_record->input_frame->timestamp());
 
+    base::OnceClosure output_cb;
     if (!job_record->legacy_ready_cb.is_null()) {
-      client_task_runner_->PostTask(
-          FROM_HERE,
-          base::BindOnce(std::move(job_record->legacy_ready_cb),
-                         buffer->BufferId(), std::move(output_frame)));
+      output_cb = base::BindOnce(std::move(job_record->legacy_ready_cb),
+                                 buffer->BufferId(), std::move(output_frame));
     } else {
-      client_task_runner_->PostTask(
-          FROM_HERE, base::BindOnce(std::move(job_record->ready_cb),
-                                    std::move(output_frame)));
+      output_cb = base::BindOnce(std::move(job_record->ready_cb),
+                                 std::move(output_frame));
     }
+    // The task might be cancelled when Reset() is called and then
+    // |client_weak_this_| becomes invalid.
+    client_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&V4L2ImageProcessor::OutputFrameOnClientSequence,
+                       client_weak_this_, std::move(output_cb)));
   }
 }
 
+void V4L2ImageProcessor::OutputFrameOnClientSequence(
+    base::OnceClosure output_cb) {
+  DVLOGF(4);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(client_sequence_checker_);
+
+  std::move(output_cb).Run();
+}
+
 bool V4L2ImageProcessor::EnqueueInputRecord(const JobRecord* job_record) {
   DVLOGF(4);
   DCHECK_CALLED_ON_VALID_SEQUENCE(device_sequence_checker_);
@@ -877,7 +917,7 @@ bool V4L2ImageProcessor::EnqueueInputRecord(const JobRecord* job_record) {
   return true;
 }
 
-bool V4L2ImageProcessor::EnqueueOutputRecord(const JobRecord* job_record) {
+bool V4L2ImageProcessor::EnqueueOutputRecord(JobRecord* job_record) {
   DVLOGF(4);
   DCHECK_CALLED_ON_VALID_SEQUENCE(device_sequence_checker_);
   DCHECK_GT(output_queue_->FreeBuffersCount(), 0u);
@@ -885,6 +925,8 @@ bool V4L2ImageProcessor::EnqueueOutputRecord(const JobRecord* job_record) {
   V4L2WritableBufferRef buffer(output_queue_->GetFreeBuffer());
   DCHECK(buffer.IsValid());
 
+  job_record->output_buffer_id = buffer.BufferId();
+
   switch (buffer.Memory()) {
     case V4L2_MEMORY_MMAP:
       return std::move(buffer).QueueMMap();

media/gpu/v4l2/v4l2_image_processor.h

@@ -91,6 +91,7 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
     FrameReadyCB ready_cb;
     LegacyFrameReadyCB legacy_ready_cb;
     scoped_refptr<VideoFrame> output_frame;
+    size_t output_buffer_id;
   };
 
   V4L2ImageProcessor(
@@ -106,10 +107,10 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
 
   bool Initialize();
   void EnqueueInput(const JobRecord* job_record);
-  void EnqueueOutput(const JobRecord* job_record);
+  void EnqueueOutput(JobRecord* job_record);
   void Dequeue();
   bool EnqueueInputRecord(const JobRecord* job_record);
-  bool EnqueueOutputRecord(const JobRecord* job_record);
+  bool EnqueueOutputRecord(JobRecord* job_record);
   bool CreateInputBuffers();
   bool CreateOutputBuffers();
 
@@ -135,6 +136,9 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
   void ProcessJobsTask();
   void ServiceDeviceTask();
 
+  // Call |output_cb| on |client_task_runner_|.
+  void OutputFrameOnClientSequence(base::OnceClosure output_cb);
+
   // Allocate/Destroy the input/output V4L2 buffers.
   void AllocateBuffersTask(bool* result, base::WaitableEvent* done);
 
@@ -146,6 +150,10 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
   // Stop all processing on |poll_task_runner_|.
   void DestroyOnPollSequence(base::WaitableEvent* event);
 
+  // Clean up pending job on |device_task_runner_|, and signal |event| after
+  // reset is finished.
+  void ResetTask(base::WaitableEvent* event);
+
   const v4l2_memory input_memory_type_;
   const v4l2_memory output_memory_type_;