gpu: Remove gpu channel filter and queue from header.

media/gpu/ipc/service/media_gpu_channel.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/gpu/ipc/service/media_gpu_channel.h"
 
 #include "base/unguessable_token.h"
 #include "gpu/ipc/service/gpu_channel.h"
 #include "ipc/message_filter.h"
 #include "media/gpu/ipc/common/media_messages.h"
 #include "media/gpu/ipc/service/gpu_video_decode_accelerator.h"
 #include "media/gpu/ipc/service/gpu_video_encode_accelerator.h"
 
 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)
       : channel_(channel), routing_id_(routing_id) {}
 
   bool Send(IPC::Message* msg) { return channel_->Send(msg); }
 
   void OnCreateVideoDecoder(const VideoDecodeAccelerator::Config& config,
                             int32_t decoder_route_id,
                             IPC::Message* reply_message) {
@@ skipping 12 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(MediaGpuChannelDispatchHelper);
 };
 
 // Filter to respond to GetChannelToken on the IO thread.
 class MediaGpuChannelFilter : public IPC::MessageFilter {
  public:
   explicit MediaGpuChannelFilter(const base::UnguessableToken& channel_token)
       : 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;
     IPC_BEGIN_MESSAGE_MAP(MediaGpuChannelFilter, msg)
       IPC_MESSAGE_HANDLER_DELAY_REPLY(GpuCommandBufferMsg_GetChannelToken,
                                       OnGetChannelToken)
       IPC_MESSAGE_UNHANDLED(handled = false)
     IPC_END_MESSAGE_MAP()
     return handled;
   }
 
   void OnGetChannelToken(IPC::Message* reply_message) {
     GpuCommandBufferMsg_GetChannelToken::WriteReplyParams(reply_message,
                                                           channel_token_);
     Send(reply_message);
   }
 
+  bool Send(IPC::Message* msg) { return channel_->Send(msg); }
+
  private:
   ~MediaGpuChannelFilter() override {}
 
   IPC::Channel* channel_;
   base::UnguessableToken channel_token_;
 };
 
 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() {}
 
 bool MediaGpuChannel::Send(IPC::Message* msg) {
   return channel_->Send(msg);
 }
 
 bool MediaGpuChannel::OnMessageReceived(const IPC::Message& message) {
   MediaGpuChannelDispatchHelper helper(this, message.routing_id());
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(MediaGpuChannel, message)
     IPC_MESSAGE_FORWARD_DELAY_REPLY(
         GpuCommandBufferMsg_CreateVideoDecoder, &helper,
         MediaGpuChannelDispatchHelper::OnCreateVideoDecoder)
     IPC_MESSAGE_FORWARD_DELAY_REPLY(
         GpuCommandBufferMsg_CreateVideoEncoder, &helper,
         MediaGpuChannelDispatchHelper::OnCreateVideoEncoder)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(GpuChannelMsg_CreateJpegDecoder,
                                     OnCreateJpegDecoder)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   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);
   if (!jpeg_decoder_) {
     // The lifetime of |jpeg_decoder_| is managed by a gpu::GpuChannel. The
     // GpuChannels destroy all the GpuJpegDecodeAccelerator that they own when
     // they are destroyed. Therefore, passing |channel_| as a raw pointer is
     // safe.
     jpeg_decoder_.reset(
         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(
     int32_t command_buffer_route_id,
     const VideoDecodeAccelerator::Config& config,
     int32_t decoder_route_id,
     IPC::Message* reply_message) {
   TRACE_EVENT0("gpu", "MediaGpuChannel::OnCreateVideoDecoder");
   gpu::GpuCommandBufferStub* stub =
       channel_->LookupCommandBuffer(command_buffer_route_id);
@@ skipping 32 matching lines @@
                           params.output_profile, params.initial_bitrate);
   GpuCommandBufferMsg_CreateVideoEncoder::WriteReplyParams(reply_message,
                                                            succeeded);
   Send(reply_message);
 
   // encoder is registered as a DestructionObserver of this stub and will
   // self-delete during destruction of this stub.
 }
 
 }  // namespace media