Cast: IPC glue between cast library transport and encoders.

chrome/renderer/media/cast_ipc_dispatcher.cc

Index: chrome/renderer/media/cast_ipc_dispatcher.cc
diff --git a/chrome/renderer/media/cast_ipc_dispatcher.cc b/chrome/renderer/media/cast_ipc_dispatcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8d3ab60a9aabfbd2393c346b74d4dee334533edc
--- /dev/null
+++ b/chrome/renderer/media/cast_ipc_dispatcher.cc
@@ -0,0 +1,111 @@
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.

[acolwell GONE FROM CHROMIUM, 2014/02/13 19:28:58]

nit: Remove "(c) ". New files are not supposed to have it.

[hubbe, 2014/02/14 00:03:24]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/renderer/media/cast_ipc_dispatcher.h"
+
+#include "chrome/common/cast_messages.h"
+#include "chrome/renderer/media/cast_transport_sender_ipc.h"
+#include "ipc/ipc_message_macros.h"
+
+CastIPCDispatcher* CastIPCDispatcher::global_instance_ = NULL;

[acolwell GONE FROM CHROMIUM, 2014/02/13 19:28:58]

This global static makes me nervous. Is this how other IPCDispatchers in the
code base address the problem you are running into?

[hubbe, 2014/02/14 00:03:24]

No, most filters use RenderThread::Current()->get_something() but
RenderThread::Current() doesn't work on the IO thread, and it just seemed really
redundant to keep a pointer to the ipc dispatcher in each
CastTransportSenderIPC.

I'm open to other suggestions.

[acolwell GONE FROM CHROMIUM, 2014/02/14 02:17:18]

This response actually makes me more nervous.

not lgtm. After looking at the code again it appears that you are accessing
|global_instance_| from multiple threads w/o a lock. IIUC the dispatcher is
created and destroyed on the renderer thread but calls to Add/RemoveSender and
Send() are occuring on the IO thread. I believe this means you have a race on
dispatcher destruction here.

I'm surprised that there isn't another example in the codebase that you could
derive inspiration from. Perhaps someone else on the reviewer list more familiar
w/ IPC techniques could point you in the right direction.

[hubbe, 2014/02/14 19:34:46]

I thought about it and changed it so that the global variable is handled in the
safe way.
It is now only accessed from the IO thread (except in one case, see the code)

+
+CastIPCDispatcher::CastIPCDispatcher(
+    const scoped_refptr<base::MessageLoopProxy>& io_message_loop)
+    : channel_(NULL),
+      io_message_loop_(io_message_loop) {
+  DCHECK(io_message_loop_);
+  DCHECK(!global_instance_);
+  global_instance_ = this;
+}
+
+CastIPCDispatcher::~CastIPCDispatcher() {
+  global_instance_ = NULL;
+}
+
+CastIPCDispatcher* CastIPCDispatcher::Get() {
+  return global_instance_;
+}
+
+void CastIPCDispatcher::Send(IPC::Message* message) {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  if (channel_) {
+    channel_->Send(message);
+  } else {
+    delete message;
+  }
+}
+
+int32 CastIPCDispatcher::AddSender(CastTransportSenderIPC* sender) {
+  return id_map_.Add(sender);
+}
+
+void CastIPCDispatcher::RemoveSender(int32 channel_id) {
+  return id_map_.Remove(channel_id);
+}
+
+bool CastIPCDispatcher::OnMessageReceived(const IPC::Message& message) {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(CastIPCDispatcher, message)
+    IPC_MESSAGE_HANDLER(CastMsg_ReceivedPacket, OnReceivedPacket)
+    IPC_MESSAGE_HANDLER(CastMsg_NotifyStatusChange, OnNotifyStatusChange)
+    IPC_MESSAGE_HANDLER(CastMsg_RtpStatistics, OnRtpStatistics)
+    IPC_MESSAGE_UNHANDLED(handled = false);
+  IPC_END_MESSAGE_MAP();
+  return handled;
+}
+
+void CastIPCDispatcher::OnFilterAdded(IPC::Channel* channel) {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  channel_ = channel;
+}
+
+void CastIPCDispatcher::OnFilterRemoved() {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  channel_ = NULL;
+}
+
+void CastIPCDispatcher::OnChannelClosing() {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  channel_ = NULL;
+}
+
+void CastIPCDispatcher::OnReceivedPacket(
+    int32 channel_id,
+    const media::cast::transport::Packet& packet) {
+  CastTransportSenderIPC* ptr = id_map_.Lookup(channel_id);

[acolwell GONE FROM CHROMIUM, 2014/02/13 19:28:58]

nit: s/ptr/sender/ here and below just to make the name a little more
descriptive.

[hubbe, 2014/02/14 00:03:24]

Done.

+  if (ptr) {
+    ptr->OnReceivedPacket(packet);
+  } else {
+    LOG(ERROR) << "CastIPCDispatcher::OnReceivedPacket "
+               << "on non-existing channel.";
+  }
+}
+
+void CastIPCDispatcher::OnNotifyStatusChange(
+    int32 channel_id,
+    media::cast::transport::CastTransportStatus status) {
+  CastTransportSenderIPC* ptr = id_map_.Lookup(channel_id);
+  if (ptr) {
+    ptr->OnNotifyStatusChange(status);
+  } else {
+    LOG(ERROR)
+        << "CastIPCDispatcher::OnNotifystatusChange on non-existing channel.";
+  }
+}
+
+void CastIPCDispatcher::OnRtpStatistics(
+    int32 channel_id,
+    bool audio,
+    const media::cast::transport::RtcpSenderInfo& sender_info,
+    base::TimeTicks time_sent,
+    uint32 rtp_timestamp) {
+  CastTransportSenderIPC* ptr = id_map_.Lookup(channel_id);
+  if (ptr) {
+    ptr->OnRtpStatistics(audio, sender_info, time_sent, rtp_timestamp);
+  } else {
+    LOG(ERROR)
+        << "CastIPCDispatcher::OnNotifystatusChange on non-existing channel.";
+  }
+}