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

content/common/gpu/client/gpu_video_decode_accelerator_host.h

Index: content/common/gpu/client/gpu_video_decode_accelerator_host.h
diff --git a/content/common/gpu/client/gpu_video_decode_accelerator_host.h b/content/common/gpu/client/gpu_video_decode_accelerator_host.h
index 552813e95f89ece6e5a6acc0887a55453b9e6c8d..6e24a39f4e2251db6055f6434a4b4a2833821083 100644
--- a/content/common/gpu/client/gpu_video_decode_accelerator_host.h
+++ b/content/common/gpu/client/gpu_video_decode_accelerator_host.h
@@ -25,9 +25,9 @@ class GpuVideoDecodeAcceleratorHost
       public CommandBufferProxyImpl::DeletionObserver,
       public base::NonThreadSafe {
  public:
-  // |channel| is used to send IPC messages to GPU process.
+  // Both |channel| and |impl| are guaranteed to outlive |this|.  (See comments

[no sievers, 2014/03/19 21:33:16]

Doesn't look like that's true, since you are observing destruction of |impl|.

[sheu, 2014/03/20 00:30:31]

The difference as I meant it is that the pointer is valid for the entire
lifetime, guaranteed by the fact that we observe destruction so we will never
see an invalid pointer.  I'm going to try to reword this.

+  // for |channel_| and |impl_|.)
   GpuVideoDecodeAcceleratorHost(GpuChannelHost* channel,
-                                int32 decoder_route_id,
                                 CommandBufferProxyImpl* impl);
 
   // IPC::Listener implementation.
@@ -52,8 +52,13 @@ class GpuVideoDecodeAcceleratorHost
   // Only Destroy() should be deleting |this|.
   virtual ~GpuVideoDecodeAcceleratorHost();
 
+  // Notify |client_| of an error.  Posts a task to avoid re-entrancy.
+  void PostNotifyError(Error);
+
   void Send(IPC::Message* message);
 
+  // IPC handlers, proxying media::VideoDecodeAccelerator::Client for the GPU
+  // process.  Should not be called directly.
   void OnBitstreamBufferProcessed(int32 bitstream_buffer_id);
   void OnProvidePictureBuffer(uint32 num_requested_buffers,
                               const gfx::Size& dimensions,
@@ -62,9 +67,10 @@ class GpuVideoDecodeAcceleratorHost
   void OnPictureReady(int32 picture_buffer_id, int32 bitstream_buffer_id);
   void OnFlushDone();
   void OnResetDone();
-  void OnErrorNotification(uint32 error);
+  void OnNotifyError(uint32 error);
 
-  // Sends IPC messages to the Gpu process.
+  // Unowned reference to the GpuChannelHost to send IPC messages to the GPU
+  // process.  |channel_| outlives |impl_|, so |channel_| also outlives |this|.

[no sievers, 2014/03/19 21:33:16]

see above. You should rather say that |channel| may be NULL if it was lost.

[sheu, 2014/03/20 00:30:31]

channel_ is set in the constructor, so it may be valid even if it is lost, if
this->Initialize() isn't called yet.

My meaning here is that the reference is valid (e.g. not dangling) whenever it
is not NULL.

   GpuChannelHost* channel_;
 
   // Route ID for the associated decoder in the GPU process.
@@ -73,12 +79,16 @@ class GpuVideoDecodeAcceleratorHost
   // The client that will receive callbacks from the decoder.
   Client* client_;
 
-  // Unowned reference to the CommandBufferProxyImpl that created us.
+  // Unowned reference to the CommandBufferProxyImpl that created us.  |this|
+  // registers as a DeletionObserver of |impl_|, so |impl_| outlives |this|.

[no sievers, 2014/03/19 21:33:16]

see above. You should rather say that |impl| may be NULL.

[sheu, 2014/03/20 00:30:31]

Done.

   CommandBufferProxyImpl* impl_;
 
   // Requested dimensions of the buffer, from ProvidePictureBuffers().
   gfx::Size picture_buffer_dimensions_;
 
+  // WeakPtr factory for posting tasks back to itself.
+  base::WeakPtrFactory<GpuVideoDecodeAcceleratorHost> weak_this_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(GpuVideoDecodeAcceleratorHost);
 };