gpu: Remove gpu channel filter and queue from header.

media/gpu/ipc/service/media_gpu_channel.cc

Index: media/gpu/ipc/service/media_gpu_channel.cc
diff --git a/media/gpu/ipc/service/media_gpu_channel.cc b/media/gpu/ipc/service/media_gpu_channel.cc
index 2f5732d046bbbf24b1e819b3fbae5a53f87c26f6..5dcb2d3710cd206bb38cf49fddca4c927058f4a2 100644
--- a/media/gpu/ipc/service/media_gpu_channel.cc
+++ b/media/gpu/ipc/service/media_gpu_channel.cc
@@ -13,25 +13,6 @@
 
 namespace media {
 
-namespace {
-
-void SendCreateJpegDecoderResult(
-    std::unique_ptr<IPC::Message> reply_message,
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
-    base::WeakPtr<gpu::GpuChannel> channel,
-    scoped_refptr<gpu::GpuChannelMessageFilter> filter,
-    bool result) {
-  GpuChannelMsg_CreateJpegDecoder::WriteReplyParams(reply_message.get(),
-                                                    result);
-  if (io_task_runner->BelongsToCurrentThread()) {
-    filter->Send(reply_message.release());
-  } else if (channel) {
-    channel->Send(reply_message.release());
-  }
-}
-
-}  // namespace
-
 class MediaGpuChannelDispatchHelper {
  public:
   MediaGpuChannelDispatchHelper(MediaGpuChannel* channel, int32_t routing_id)
@@ -64,7 +45,6 @@ class MediaGpuChannelFilter : public IPC::MessageFilter {
       : channel_token_(channel_token) {}
 
   void OnFilterAdded(IPC::Channel* channel) override { channel_ = channel; }
-  bool Send(IPC::Message* msg) { return channel_->Send(msg); }
 
   bool OnMessageReceived(const IPC::Message& msg) override {
     bool handled = true;
@@ -82,6 +62,8 @@ class MediaGpuChannelFilter : public IPC::MessageFilter {
     Send(reply_message);
   }
 
+  bool Send(IPC::Message* msg) { return channel_->Send(msg); }
+
  private:
   ~MediaGpuChannelFilter() override {}
 
@@ -91,8 +73,8 @@ class MediaGpuChannelFilter : public IPC::MessageFilter {
 
 MediaGpuChannel::MediaGpuChannel(gpu::GpuChannel* channel,
                                  const base::UnguessableToken& channel_token)
-    : channel_(channel) {
-  channel_->AddFilter(new MediaGpuChannelFilter(channel_token));
+    : channel_(channel), filter_(new MediaGpuChannelFilter(channel_token)) {

[sandersd (OOO until July 31), 2017/05/11 21:09:18]

I'm not very comfortable keeping a reference to the filter, because it makes it
very easy to get the threading wrong (and in particular, the filter lifetime is
no longer tied to the channel lifetime).

The new code doesn't fundamentally change anything, so I suppose for now I'm
just asking you to add comments explaining the threading and lifetime
considerations that future maintainers should be aware of.

[sunnyps, 2017/05/11 22:21:46]

Filter lifetime has never been tied to the (gpu or media) channel lifetime. Can
you clarify what you mean by that?

I can't remove GpuChannelMessageFilter from the header if MediaGpuChannel
depends on it. It seems fundamentally better to rely on the lifetime and
behavior of MediaGpuChannelFilter than GpuChannelMessageFilter here. I was also
hoping to remove GpuChannel::AsWeakPtr and have MediaGpuChannel have a
WeakPtrFactory of its own. WDYT?

I'll add a comment about this nonetheless.

Side note: MediaGpuChannelFilter should implement OnFilterRemoved and set
channel_ to nullptr (and if (channel_) in Send). GpuChannelMessageFilter also
doesn't check ipc_channel_ in Send but that's ok because it's not used in a
PostTask (maybe needs a DCHECK in Send?).

[sandersd (OOO until July 31), 2017/05/11 22:45:03]

The specific implementation of this filter guarantees that it won't access the
channel unless the channel exists. That's no longer true if its methods can be
called by something other than the channel.
This makes sense, but the current implementation takes no special steps to
manage that lifetime. We shouldn't rely on it until it is proven safe.
The current implementation is safe (because the methods are all sync and are
only called by the channel), but these are standard checks that are reasonable
to add, especially if the implementation is going to become accessible to code
other than the channel.

[sunnyps, 2017/05/11 22:53:24]

Oh you mean accessing |channel_| right? That should be solved by implementing
OnFilterRemoved and not accessing the channel after that.
IPC::MessageFilter (the base class) is RefCountedThreadSafe so I don't
understand your concern here about lifetime modulo implementing OnFilterRemoved
as mentioned above. GpuChannelMessageFilter also doesn't take any special steps
here.
Agreed.

[sandersd (OOO until July 31), 2017/05/11 23:14:38]

I believe that OnFilterRemoved is a sufficient solution to the concerns I have.

+  channel_->AddFilter(filter_.get());
 }
 
 MediaGpuChannel::~MediaGpuChannel() {}
@@ -118,6 +100,25 @@ bool MediaGpuChannel::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
+namespace {
+
+void SendCreateJpegDecoderResult(
+    std::unique_ptr<IPC::Message> reply_message,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    base::WeakPtr<IPC::Sender> channel,
+    scoped_refptr<MediaGpuChannelFilter> filter,
+    bool result) {
+  GpuChannelMsg_CreateJpegDecoder::WriteReplyParams(reply_message.get(),
+                                                    result);
+  if (io_task_runner->BelongsToCurrentThread()) {
+    filter->Send(reply_message.release());
+  } else if (channel) {
+    channel->Send(reply_message.release());
+  }
+}
+
+}  // namespace
+
 void MediaGpuChannel::OnCreateJpegDecoder(int32_t route_id,
                                           IPC::Message* reply_msg) {
   std::unique_ptr<IPC::Message> msg(reply_msg);
@@ -130,9 +131,9 @@ void MediaGpuChannel::OnCreateJpegDecoder(int32_t route_id,
         new GpuJpegDecodeAccelerator(channel_, channel_->io_task_runner()));
   }
   jpeg_decoder_->AddClient(
-      route_id, base::Bind(&SendCreateJpegDecoderResult, base::Passed(&msg),
-                           channel_->io_task_runner(), channel_->AsWeakPtr(),
-                           channel_->filter()));
+      route_id,
+      base::Bind(&SendCreateJpegDecoderResult, base::Passed(&msg),
+                 channel_->io_task_runner(), channel_->AsWeakPtr(), filter_));
 }
 
 void MediaGpuChannel::OnCreateVideoDecoder(