Make VEA client of command buffer; move sync. IPC to VDA/VEA::Initialize()

content/common/gpu/client/gpu_video_encode_accelerator_host.cc

Index: content/common/gpu/client/gpu_video_encode_accelerator_host.cc
diff --git a/content/common/gpu/client/gpu_video_encode_accelerator_host.cc b/content/common/gpu/client/gpu_video_encode_accelerator_host.cc
index 14d330e960fb223ca43b4920d240f8559796fc5d..423363b1be20555ea2a08ae3165be49f966e33d0 100644
--- a/content/common/gpu/client/gpu_video_encode_accelerator_host.cc
+++ b/content/common/gpu/client/gpu_video_encode_accelerator_host.cc
@@ -13,19 +13,30 @@
 
 namespace content {
 
+#define NOTIFY_ERROR(error) \
+  PostNotifyError(error);   \
+  DLOG(ERROR)
+
 GpuVideoEncodeAcceleratorHost::GpuVideoEncodeAcceleratorHost(
-    const scoped_refptr<GpuChannelHost>& gpu_channel_host,
-    int32 route_id)
-    : client_(NULL),
-      channel_(gpu_channel_host),
-      route_id_(route_id),
-      next_frame_id_(0) {
-  channel_->AddRoute(route_id_, AsWeakPtr());
+    GpuChannelHost* channel,
+    CommandBufferProxyImpl* impl)
+    : channel_(channel),
+      encoder_route_id_(-1),
+      client_(NULL),
+      impl_(impl),
+      next_frame_id_(0),
+      weak_this_factory_(this) {
+  DCHECK(channel_);
+  DCHECK(impl_);
+  impl_->AddDeletionObserver(this);
 }
 
 GpuVideoEncodeAcceleratorHost::~GpuVideoEncodeAcceleratorHost() {
+  DCHECK(CalledOnValidThread());
   if (channel_)
-    channel_->RemoveRoute(route_id_);
+    channel_->RemoveRoute(encoder_route_id_);
+  if (impl_)
+    impl_->RemoveDeletionObserver(this);
 }
 
 // static
@@ -38,8 +49,6 @@ bool GpuVideoEncodeAcceleratorHost::OnMessageReceived(
     const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(GpuVideoEncodeAcceleratorHost, message)
-    IPC_MESSAGE_HANDLER(AcceleratedVideoEncoderHostMsg_NotifyInitializeDone,
-                        OnNotifyInitializeDone)
     IPC_MESSAGE_HANDLER(AcceleratedVideoEncoderHostMsg_RequireBitstreamBuffers,
                         OnRequireBitstreamBuffers)
     IPC_MESSAGE_HANDLER(AcceleratedVideoEncoderHostMsg_NotifyInputDone,
@@ -57,47 +66,58 @@ bool GpuVideoEncodeAcceleratorHost::OnMessageReceived(
 }
 
 void GpuVideoEncodeAcceleratorHost::OnChannelError() {
-  DLOG(ERROR) << "OnChannelError()";
+  DCHECK(CalledOnValidThread());
   if (channel_) {
-    channel_->RemoveRoute(route_id_);
+    channel_->RemoveRoute(encoder_route_id_);
     channel_ = NULL;
+    weak_this_factory_.InvalidateWeakPtrs();
   }
-  // See OnNotifyError for why this needs to be the last thing in this
-  // function.
-  OnNotifyError(kPlatformFailureError);
+  NOTIFY_ERROR(kPlatformFailureError) << "OnChannelError()";

[Ami GONE FROM CHROMIUM, 2014/03/17 03:17:54]

ditto bug comment from GVDAH.cc

[sheu, 2014/03/18 22:38:35]

Done.

 }
 
-void GpuVideoEncodeAcceleratorHost::Initialize(
+bool GpuVideoEncodeAcceleratorHost::Initialize(
     media::VideoFrame::Format input_format,
     const gfx::Size& input_visible_size,
     media::VideoCodecProfile output_profile,
     uint32 initial_bitrate,
     Client* client) {
+  DCHECK(CalledOnValidThread());
   client_ = client;
-  client_ptr_factory_.reset(new base::WeakPtrFactory<Client>(client_));
-  Send(new AcceleratedVideoEncoderMsg_Initialize(route_id_,
-                                                 input_format,
-                                                 input_visible_size,
-                                                 output_profile,
-                                                 initial_bitrate));
+  if (!impl_) {
+    DLOG(ERROR) << "impl_ destroyed";

[Ami GONE FROM CHROMIUM, 2014/03/17 03:17:54]

"impl_" doesn't really connote CommandBufferProxyImpl to me...

[sheu, 2014/03/18 22:38:35]

Sure, but it also keeps us from having to update comments if the class name
changes.  I figure if you're debugging the message you'll be able to tell where
this is coming from.

+    return false;
+  }
+  Send(new GpuCommandBufferMsg_CreateVideoEncoder(impl_->GetRouteID(),
+                                                  input_format,
+                                                  input_visible_size,
+                                                  output_profile,
+                                                  initial_bitrate,
+                                                  &encoder_route_id_));
+  if (encoder_route_id_ < 0) {
+    DLOG(ERROR) << "Send(GpuCommandBufferMsg_CreateVideoEncoder()) failed";
+    return false;
+  }
+  channel_->AddRoute(encoder_route_id_, weak_this_factory_.GetWeakPtr());
+  return true;
 }
 
 void GpuVideoEncodeAcceleratorHost::Encode(
     const scoped_refptr<media::VideoFrame>& frame,
     bool force_keyframe) {
+  DCHECK(CalledOnValidThread());
   if (!channel_)
     return;
+
   if (!base::SharedMemory::IsHandleValid(frame->shared_memory_handle())) {
-    DLOG(ERROR) << "Encode(): cannot encode frame not backed by shared memory";
-    NotifyError(kPlatformFailureError);
+    NOTIFY_ERROR(kPlatformFailureError)
+        << "Encode(): cannot encode frame not backed by shared memory";
     return;
   }
   base::SharedMemoryHandle handle =
       channel_->ShareToGpuProcess(frame->shared_memory_handle());
   if (!base::SharedMemory::IsHandleValid(handle)) {
-    DLOG(ERROR) << "Encode(): failed to duplicate buffer handle for GPU "
-                   "process";
-    NotifyError(kPlatformFailureError);
+    NOTIFY_ERROR(kPlatformFailureError)
+        << "Encode(): failed to duplicate buffer handle for GPU process";
     return;
   }
 
@@ -113,7 +133,7 @@ void GpuVideoEncodeAcceleratorHost::Encode(
   }
 
   Send(new AcceleratedVideoEncoderMsg_Encode(
-      route_id_, next_frame_id_, handle, frame_size, force_keyframe));
+      encoder_route_id_, next_frame_id_, handle, frame_size, force_keyframe));
   frame_map_[next_frame_id_] = frame;
 
   // Mask against 30 bits, to avoid (undefined) wraparound on signed integer.
@@ -122,51 +142,77 @@ void GpuVideoEncodeAcceleratorHost::Encode(
 
 void GpuVideoEncodeAcceleratorHost::UseOutputBitstreamBuffer(
     const media::BitstreamBuffer& buffer) {
+  DCHECK(CalledOnValidThread());
   if (!channel_)
     return;
+
   base::SharedMemoryHandle handle =
       channel_->ShareToGpuProcess(buffer.handle());
   if (!base::SharedMemory::IsHandleValid(handle)) {
-    DLOG(ERROR) << "UseOutputBitstreamBuffer(): failed to duplicate buffer "
-                   "handle for GPU process: buffer.id()=" << buffer.id();
-    NotifyError(kPlatformFailureError);
+    NOTIFY_ERROR(kPlatformFailureError)
+        << "UseOutputBitstreamBuffer(): failed to duplicate buffer handle "
+           "for GPU process: buffer.id()=" << buffer.id();
     return;
   }
   Send(new AcceleratedVideoEncoderMsg_UseOutputBitstreamBuffer(
-      route_id_, buffer.id(), handle, buffer.size()));
+      encoder_route_id_, buffer.id(), handle, buffer.size()));
 }
 
 void GpuVideoEncodeAcceleratorHost::RequestEncodingParametersChange(
     uint32 bitrate,
     uint32 framerate) {
+  DCHECK(CalledOnValidThread());
+  if (!channel_)
+    return;
+
   Send(new AcceleratedVideoEncoderMsg_RequestEncodingParametersChange(
-      route_id_, bitrate, framerate));
+      encoder_route_id_, bitrate, framerate));
 }
 
 void GpuVideoEncodeAcceleratorHost::Destroy() {
-  Send(new GpuChannelMsg_DestroyVideoEncoder(route_id_));
+  DCHECK(CalledOnValidThread());
+  if (channel_)
+    Send(new AcceleratedVideoEncoderMsg_Destroy(encoder_route_id_));
+  client_ = NULL;
   delete this;
 }
 
-void GpuVideoEncodeAcceleratorHost::NotifyError(Error error) {
-  DVLOG(2) << "NotifyError(): error=" << error;
+void GpuVideoEncodeAcceleratorHost::OnWillDeleteImpl() {
+  DCHECK(CalledOnValidThread());
+  impl_ = NULL;
+
+  // The CommandBufferProxyImpl is going away; error out this VEA.
+  OnChannelError();
+}
+
+void GpuVideoEncodeAcceleratorHost::PostNotifyError(Error error) {
+  DCHECK(CalledOnValidThread());
+  DVLOG(2) << "PostNotifyError(): error=" << error;
+  // Post the error notification back to this thread, to avoid re-entrancy.
   base::MessageLoopProxy::current()->PostTask(
       FROM_HERE,
-      base::Bind(&media::VideoEncodeAccelerator::Client::NotifyError,
-                 client_ptr_factory_->GetWeakPtr(),
+      base::Bind(&GpuVideoEncodeAcceleratorHost::OnNotifyError,
+                 weak_this_factory_.GetWeakPtr(),
                  error));
 }
 
-void GpuVideoEncodeAcceleratorHost::OnNotifyInitializeDone() {
-  DVLOG(2) << "OnNotifyInitializeDone()";
-  if (client_)
-    client_->NotifyInitializeDone();
+void GpuVideoEncodeAcceleratorHost::Send(IPC::Message* message) {
+  DCHECK(CalledOnValidThread());
+  // After OnChannelError is called, the client should no longer send
+  // messages to the gpu channel through this object.  But queued posted tasks
+  // can still be draining, so we're forgiving and simply ignore them.

[Ami GONE FROM CHROMIUM, 2014/03/17 03:17:54]

It's not very forgiving to try to send on channel_ when it's NULL because
OnChannelError!

(I see you added some if(channel_) guards around some calls to Send(), but it's
unclear to me why that's better than just early-returning here if !channel_ like
the code used to do)

[sheu, 2014/03/18 22:38:35]

There's some call-site that early-out already on !channel_, so I figured I'd
un-redundantify the checks.

Removed the comment.

+  uint32 message_type = message->type();
+  if (!channel_->Send(message)) {
+    NOTIFY_ERROR(kPlatformFailureError) << "Send(" << message_type
+                                        << ") failed";
+  }
 }
 
 void GpuVideoEncodeAcceleratorHost::OnRequireBitstreamBuffers(
     uint32 input_count,
     const gfx::Size& input_coded_size,
     uint32 output_buffer_size) {
+  DCHECK(CalledOnValidThread());
   DVLOG(2) << "OnRequireBitstreamBuffers(): input_count=" << input_count
            << ", input_coded_size=" << input_coded_size.ToString()
            << ", output_buffer_size=" << output_buffer_size;
@@ -177,6 +223,7 @@ void GpuVideoEncodeAcceleratorHost::OnRequireBitstreamBuffers(
 }
 
 void GpuVideoEncodeAcceleratorHost::OnNotifyInputDone(int32 frame_id) {
+  DCHECK(CalledOnValidThread());
   DVLOG(3) << "OnNotifyInputDone(): frame_id=" << frame_id;
   // Fun-fact: std::hash_map is not spec'd to be re-entrant; since freeing a
   // frame can trigger a further encode to be kicked off and thus an .insert()
@@ -199,6 +246,7 @@ void GpuVideoEncodeAcceleratorHost::OnBitstreamBufferReady(
     int32 bitstream_buffer_id,
     uint32 payload_size,
     bool key_frame) {
+  DCHECK(CalledOnValidThread());
   DVLOG(3) << "OnBitstreamBufferReady(): "
               "bitstream_buffer_id=" << bitstream_buffer_id
            << ", payload_size=" << payload_size
@@ -208,10 +256,10 @@ void GpuVideoEncodeAcceleratorHost::OnBitstreamBufferReady(
 }
 
 void GpuVideoEncodeAcceleratorHost::OnNotifyError(Error error) {
+  DCHECK(CalledOnValidThread());
   DVLOG(2) << "OnNotifyError(): error=" << error;
   if (!client_)
     return;
-  client_ptr_factory_.reset();
 
   // Client::NotifyError() may Destroy() |this|, so calling it needs to be the
   // last thing done on this stack!
@@ -220,16 +268,4 @@ void GpuVideoEncodeAcceleratorHost::OnNotifyError(Error error) {
   client->NotifyError(error);
 }
 
-void GpuVideoEncodeAcceleratorHost::Send(IPC::Message* message) {
-  if (!channel_) {
-    DLOG(ERROR) << "Send(): no channel";
-    delete message;
-    NotifyError(kPlatformFailureError);
-  } else if (!channel_->Send(message)) {
-    DLOG(ERROR) << "Send(): sending failed: message->type()="
-                << message->type();
-    NotifyError(kPlatformFailureError);
-  }
-}
-
 }  // namespace content