Move video encode accelerator IPC messages to GPU IO thread

media/gpu/ipc/service/gpu_video_encode_accelerator.cc

Index: media/gpu/ipc/service/gpu_video_encode_accelerator.cc
diff --git a/media/gpu/ipc/service/gpu_video_encode_accelerator.cc b/media/gpu/ipc/service/gpu_video_encode_accelerator.cc
index 26d2aaa55d5f2c2dea38d33181dd7e3f5944d6dc..6a6fc44dfd5f9cdf8464aefdc810ec6759d0ee17 100644
--- a/media/gpu/ipc/service/gpu_video_encode_accelerator.cc
+++ b/media/gpu/ipc/service/gpu_video_encode_accelerator.cc
@@ -99,13 +99,73 @@ std::unique_ptr<VideoEncodeAccelerator> CreateMediaFoundationVEA() {
 
 }  // anonymous namespace
 
+class GpuVideoEncodeAccelerator::MessageFilter : public IPC::MessageFilter {
+ public:
+  MessageFilter(GpuVideoEncodeAccelerator* owner, int32_t host_route_id)
+      : owner_(owner), host_route_id_(host_route_id) {}
+
+  void OnChannelError() override { sender_ = NULL; }
+
+  void OnChannelClosing() override { sender_ = NULL; }
+
+  void OnFilterAdded(IPC::Channel* channel) override { sender_ = channel; }
+
+  void OnFilterRemoved() override {
+    // This will delete |owner_| and |this|.

[sandersd (OOO until July 31), 2016/10/21 18:41:03]

Misleading comment, since it makes the (cross-class) assumption that
RemoveFilter() was called by the destructor. I would prefer the simplicity of a
model where the filter lifecycle is not so directly tied to GVEA. (No specific
suggestions to achieve that though, the existing callback model certainly
doesn't make it easy.)

[emircan, 2016/10/24 19:44:52]

I agree that it sounds like it is coming from dtor, so I removed the comment. I
actually got this model from GPUVDA directly.
OnWillDestroyStub() calls OnFilterRemoved(), then destroys GPUVEA and filter as
well.

https://cs.chromium.org/chromium/src/media/gpu/ipc/service/gpu_video_decode_a...

+    owner_->OnFilterRemoved();
+  }
+
+  bool OnMessageReceived(const IPC::Message& msg) override {
+    if (msg.routing_id() != host_route_id_)
+      return false;
+
+    IPC_BEGIN_MESSAGE_MAP(MessageFilter, msg)
+      IPC_MESSAGE_FORWARD(AcceleratedVideoEncoderMsg_Encode, owner_,
+                          GpuVideoEncodeAccelerator::OnEncode)
+      IPC_MESSAGE_FORWARD(AcceleratedVideoEncoderMsg_UseOutputBitstreamBuffer,
+                          owner_,
+                          GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer)
+      IPC_MESSAGE_FORWARD(
+          AcceleratedVideoEncoderMsg_RequestEncodingParametersChange, owner_,
+          GpuVideoEncodeAccelerator::OnRequestEncodingParametersChange)
+      IPC_MESSAGE_UNHANDLED(return false)
+    IPC_END_MESSAGE_MAP()
+    return true;
+  }
+
+  bool SendOnIOThread(IPC::Message* message) {
+    CHECK(!message->is_sync());
+    if (!sender_) {
+      delete message;
+      return false;
+    }
+    const bool rv = sender_->Send(message);
+    CHECK(rv);

[sandersd (OOO until July 31), 2016/10/21 18:41:03]

?

[emircan, 2016/10/24 19:44:52]

It was paranioa check. Removing it.

+    return rv;
+  }
+
+ protected:
+  ~MessageFilter() override {}
+
+ private:
+  GpuVideoEncodeAccelerator* const owner_;
+  const int32_t host_route_id_;
+  // The sender to which this filter was added.
+  IPC::Sender* sender_;
+};
+
 GpuVideoEncodeAccelerator::GpuVideoEncodeAccelerator(
     int32_t host_route_id,
-    gpu::GpuCommandBufferStub* stub)
+    gpu::GpuCommandBufferStub* stub,
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
     : host_route_id_(host_route_id),
       stub_(stub),
       input_format_(PIXEL_FORMAT_UNKNOWN),
       output_buffer_size_(0),
+      filter_removed_(base::WaitableEvent::ResetPolicy::MANUAL,
+                      base::WaitableEvent::InitialState::NOT_SIGNALED),
+      main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      io_task_runner_(io_task_runner),
       weak_this_factory_(this) {
   stub_->AddDestructionObserver(this);
   make_context_current_ =
@@ -122,6 +182,7 @@ bool GpuVideoEncodeAccelerator::Initialize(VideoPixelFormat input_format,
                                            const gfx::Size& input_visible_size,
                                            VideoCodecProfile output_profile,
                                            uint32_t initial_bitrate) {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   DVLOG(1) << __FUNCTION__
            << " input_format=" << VideoPixelFormatToString(input_format)
            << ", input_visible_size=" << input_visible_size.ToString()
@@ -153,6 +214,13 @@ bool GpuVideoEncodeAccelerator::Initialize(VideoPixelFormat input_format,
                              initial_bitrate, this)) {
       input_format_ = input_format;
       input_visible_size_ = input_visible_size;
+      // Attempt to set up performing encoding tasks on IO thread, if supported
+      // by the VEA.
+      if (encoder_->TryToSetupEncodeOnSeparateThread(
+              weak_this_factory_.GetWeakPtr(), io_task_runner_)) {
+        filter_ = new MessageFilter(this, host_route_id_);
+        stub_->channel()->AddFilter(filter_.get());
+      }
       return true;
     }
   }
@@ -180,8 +248,10 @@ void GpuVideoEncodeAccelerator::RequireBitstreamBuffers(
     unsigned int input_count,
     const gfx::Size& input_coded_size,
     size_t output_buffer_size) {
-  Send(new AcceleratedVideoEncoderHostMsg_RequireBitstreamBuffers(
-      host_route_id_, input_count, input_coded_size, output_buffer_size));
+  if (!Send(new AcceleratedVideoEncoderHostMsg_RequireBitstreamBuffers(
+          host_route_id_, input_count, input_coded_size, output_buffer_size))) {
+    DLOG(ERROR) << __func__ << " failed.";

[sandersd (OOO until July 31), 2016/10/21 18:41:03]

Should GVEA be entering some sort of error state on these kinds of failures?

[emircan, 2016/10/24 19:44:52]

I added these to have logs if any error occurs but it looks like we generally do
not expect these to fail at all. There was no error handling here, and VDA seems
to also just log. I think the policy is to try sending the next message even if
one fails.
https://cs.chromium.org/chromium/src/media/gpu/ipc/service/gpu_video_decode_a...

+  }
   input_coded_size_ = input_coded_size;
   output_buffer_size_ = output_buffer_size;
 }
@@ -191,17 +261,32 @@ void GpuVideoEncodeAccelerator::BitstreamBufferReady(
     size_t payload_size,
     bool key_frame,
     base::TimeDelta timestamp) {
-  Send(new AcceleratedVideoEncoderHostMsg_BitstreamBufferReady(
-      host_route_id_, bitstream_buffer_id, payload_size, key_frame, timestamp));
+  DCHECK(CheckCalledOnMessageFilterThread());
+  if (!Send(new AcceleratedVideoEncoderHostMsg_BitstreamBufferReady(
+          host_route_id_, bitstream_buffer_id, payload_size, key_frame,
+          timestamp))) {
+    DLOG(ERROR) << __func__ << " failed.";
+  }
 }
 
 void GpuVideoEncodeAccelerator::NotifyError(
     VideoEncodeAccelerator::Error error) {
-  Send(new AcceleratedVideoEncoderHostMsg_NotifyError(host_route_id_, error));
+  DCHECK(CheckCalledOnMessageFilterThread());
+  if (!Send(new AcceleratedVideoEncoderHostMsg_NotifyError(host_route_id_,
+                                                           error))) {
+    DLOG(ERROR) << __func__ << " failed.";
+  }
 }
 
 void GpuVideoEncodeAccelerator::OnWillDestroyStub() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK(stub_);
+
+  if (filter_) {
+    stub_->channel()->RemoveFilter(filter_.get());
+    filter_removed_.Wait();
+  }
+
   stub_->channel()->RemoveRoute(host_route_id_);
   stub_->RemoveDestructionObserver(this);
   encoder_.reset();
@@ -226,6 +311,12 @@ GpuVideoEncodeAccelerator::GetSupportedProfiles(
   return GpuVideoAcceleratorUtil::ConvertMediaToGpuEncodeProfiles(profiles);
 }
 
+void GpuVideoEncodeAccelerator::OnFilterRemoved() {
+  // We're destroying; cancel all callbacks.

[sandersd (OOO until July 31), 2016/10/21 18:41:03]

Are there no GPU shutdown paths where the channel filter can be removed without
GVEA being fully destructed first?

[emircan, 2016/10/24 19:44:52]

Filter is actually removed before GVEA is destructed. Only shutdown path here is
OnWillDestroyStub(), lifetime of all these objects are tied to the stub as it
is.

+  weak_this_factory_.InvalidateWeakPtrs();
+  filter_removed_.Signal();
+}
+
 // static
 std::vector<GpuVideoEncodeAccelerator::VEAFactoryFunction>
 GpuVideoEncodeAccelerator::GetVEAFactoryFunctions(
@@ -254,6 +345,7 @@ GpuVideoEncodeAccelerator::GetVEAFactoryFunctions(
 
 void GpuVideoEncodeAccelerator::OnEncode(
     const AcceleratedVideoEncoderMsg_Encode_Params& params) {
+  DCHECK(CheckCalledOnMessageFilterThread());
   DVLOG(3) << __FUNCTION__ << " frame_id = " << params.frame_id
            << ", buffer_size=" << params.buffer_size
            << ", force_keyframe=" << params.force_keyframe;
@@ -314,6 +406,7 @@ void GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(
     int32_t buffer_id,
     base::SharedMemoryHandle buffer_handle,
     uint32_t buffer_size) {
+  DCHECK(CheckCalledOnMessageFilterThread());
   DVLOG(3) << __FUNCTION__ << " buffer_id=" << buffer_id
            << ", buffer_size=" << buffer_size;
   if (!encoder_)
@@ -334,6 +427,7 @@ void GpuVideoEncodeAccelerator::OnUseOutputBitstreamBuffer(
 }
 
 void GpuVideoEncodeAccelerator::OnDestroy() {
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
   DVLOG(2) << __FUNCTION__;
   OnWillDestroyStub();
 }
@@ -341,6 +435,7 @@ void GpuVideoEncodeAccelerator::OnDestroy() {
 void GpuVideoEncodeAccelerator::OnRequestEncodingParametersChange(
     uint32_t bitrate,
     uint32_t framerate) {
+  DCHECK(CheckCalledOnMessageFilterThread());
   DVLOG(2) << __FUNCTION__ << " bitrate=" << bitrate
            << ", framerate=" << framerate;
   if (!encoder_)
@@ -351,13 +446,26 @@ void GpuVideoEncodeAccelerator::OnRequestEncodingParametersChange(
 void GpuVideoEncodeAccelerator::EncodeFrameFinished(
     int32_t frame_id,
     std::unique_ptr<base::SharedMemory> shm) {
-  Send(new AcceleratedVideoEncoderHostMsg_NotifyInputDone(host_route_id_,
-                                                          frame_id));
+  DCHECK(CheckCalledOnMessageFilterThread());
+  if (!Send(new AcceleratedVideoEncoderHostMsg_NotifyInputDone(host_route_id_,
+                                                               frame_id))) {
+    DLOG(ERROR) << __func__ << " failed.";
+  }
   // Just let |shm| fall out of scope.
 }
 
-void GpuVideoEncodeAccelerator::Send(IPC::Message* message) {
-  stub_->channel()->Send(message);
+bool GpuVideoEncodeAccelerator::Send(IPC::Message* message) {
+  if (filter_ && io_task_runner_->BelongsToCurrentThread()) {
+    DCHECK(io_task_runner_->BelongsToCurrentThread());

[sandersd (OOO until July 31), 2016/10/21 18:41:03]

DCHECK() is redundant.

[emircan, 2016/10/24 19:44:52]

Done.

+    return filter_->SendOnIOThread(message);
+  }
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+  return stub_->channel()->Send(message);
+}
+
+bool GpuVideoEncodeAccelerator::CheckCalledOnMessageFilterThread() {
+  return (io_task_runner_->BelongsToCurrentThread() && filter_) ||
+         (main_task_runner_->BelongsToCurrentThread() && !filter_);

[sandersd (OOO until July 31), 2016/10/21 18:41:03]

Nit: Putting |filter_| condition first makes more sense.

[emircan, 2016/10/24 19:44:52]

Done.

 }
 
 }  // namespace media