Cast: IPC glue between cast library transport and encoders.

chrome/renderer/media/cast_transport_sender_ipc.h

+// Copyright (c) 2013 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.
+
+#ifndef CHROME_RENDERER_MEDIA_CAST_TRANSPORT_SENDER_IPC_H_
+#define CHROME_RENDERER_MEDIA_CAST_TRANSPORT_SENDER_IPC_H_
+
+#include "base/callback.h"
+#include "base/id_map.h"
+#include "content/common/cast_messages.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "media/cast/cast_sender.h"
+#include "media/cast/transport/cast_transport_sender.h"
+
+namespace cast {

[scherkus (not reviewing), 2014/02/07 00:11:06]

IIRC we prefer chrome/ code to be in the global namespace:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/pcgYLSJsii8...

also these types of namespaces are generally discouraged since the types inside
of the cast namespace contain a CastXXX prefix, so it ends up being redundant

it also isn't clear to future readers which cast:: is which (i.e., the one under
media/ or the one under chrome/?)
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/h11LgwA5vgg/qJb4V...

[hubbe, 2014/02/07 00:52:22]

Done.

+
+class CastIPCDispatcher;
+
+// This implementation of the CastTransportSender interface
+// communicates with the browser process over IPC and relays
+// all calls to/from the transport sender to the browser process.
+// The primary reason for this arrangement is to give the
+// renderer less direct control over the UDP sockets.
+class CastTransportSenderIPC
+    : public media::cast::transport::CastTransportSender {
+ public:
+  CastTransportSenderIPC(
+      const media::cast::transport::CastTransportConfig& config,
+      const media::cast::transport::CastTransportStatusCallback& status_cb);
+  virtual ~CastTransportSenderIPC();
+
+  // media::cast::transport::CastTransportSender implementation

[scherkus (not reviewing), 2014/02/07 00:11:06]

comments end with periods

[hubbe, 2014/02/07 00:52:22]

Done.

+  virtual void SetPacketReceiver(
+      const media::cast::transport::PacketReceiverCallback& packet_callback)
+      OVERRIDE;
+  virtual void InsertCodedAudioFrame(
+      const media::cast::transport::EncodedAudioFrame* audio_frame,
+      const base::TimeTicks& recorded_time) OVERRIDE;
+  virtual void InsertCodedVideoFrame(
+      const media::cast::transport::EncodedVideoFrame* video_frame,
+      const base::TimeTicks& capture_time) OVERRIDE;
+
+  virtual void SendRtcpFromRtpSender(
+      uint32 packet_type_flags,
+      const media::cast::transport::RtcpSenderInfo& sender_info,
+      const media::cast::transport::RtcpDlrrReportBlock& dlrr,
+      const media::cast::transport::RtcpSenderLogMessage& sender_log,
+      uint32 sending_ssrc,
+      const std::string& c_name) OVERRIDE;
+
+  // Retransmission request.

[scherkus (not reviewing), 2014/02/07 00:11:06]

nit: this looks duplicated from the CastTransportSender .h -- I'd just nuke it

[hubbe, 2014/02/07 00:52:22]

Done.

+  virtual void ResendPackets(
+      bool is_audio,
+      const media::cast::MissingFramesAndPacketsMap& missing_packets) OVERRIDE;
+
+  virtual void SubscribeAudioRtpStatsCallback(
+      const media::cast::transport::CastTransportRtpStatistics& callback)
+      OVERRIDE;
+
+  virtual void SubscribeVideoRtpStatsCallback(
+      const media::cast::transport::CastTransportRtpStatistics& callback)
+      OVERRIDE;
+
+ private:
+  bool Send(IPC::Message *message);
+
+  friend class CastIPCDispatcher;

[scherkus (not reviewing), 2014/02/07 00:11:06]

do these two classes really need to be friends?

why not expose public methods on both instead of poking at each others privates?

something like:

class CastIPCDispatcher {
  // Returns global instance if created, NULL otherwise.
  static CastIPCDispatcher* Get();

  int AddTransport(CastTransportSenderIPC* sender);
  void RemoveTransport(int channel_id);

  void Send(IPC::Message* msg) {
    if (!channel_ || !channel_->Send(msg))
      delete msg;
  }

  // etc...
};


alternatively you can make CastTransportSenderIPC a private inner class of
CastIPCDispatcher and completely hide the existence of CastTransportSenderIPC
entirely (i.e., CastIPCDispatcher would create them and return
media::cast::transport::CastTransportSender*), but that might be going a bit
overboard

WDYT?

[hubbe, 2014/02/07 00:52:22]

Generally I think of these two classes as one unit.
Adding helper functions may or may not be nicer, but without the friendship
relationship these helper functions would be exposed to a lot of people who
don't need them.
If the two classes go in separate h/cc files, then it's probably clearer to get
rid of the friendship and just use public methods.

Actually, my preferred way to write this would be to only expose factory methods
in this file and put the classes in the .cc file. That way public methods are
only
exposed in one file. Although, that's not usually how it's done in chrome, so
I'm not sure if that is a good idea or not.

PS: Not arguing to avoid work, just checking which way to go with it before i
change it. :)

[scherkus (not reviewing), 2014/02/07 01:15:29]

Not entirely sure what you mean by factory methods ... but typically chrome is
OK exposing extra public methods and classes *within a module* (i.e., chrome/,
content/, net/)

My $0.02: it can be infuriating enough dealing with abstraction layers between
modules, hence the tendency to avoid additional abstractions within a module and
just use the types you declare and don't worry about over exposing public
methods


Anyway I don't know where this leaves us and I'm going on vacation right now.
I'd either:
  a) go the full two-file-no-friend split
  b) go the hidden internal class route and make them friendly

I'll get acolwell@ to review subsequent changes.

[hubbe, 2014/02/07 22:27:47]

Ok, I started working on (a), but it ended up ugly (in my opinion) because it
just had too many methods that looks something like:

void FOO(some arguments) {
  other_object->FOO(some_arguments);
}

Repeat for FOO, BAR, BAZ, etc. and with the inherent duplication between .h and
.cc, it just became too much for me.  So instead I decided to show you what I
meant by using "factory methods". (I really meant "factory functions" though...)

Feel free to tell me how fugly it is and then I'll go back to (a) or (b) again.
:)

+  int32 channel_id_;
+  media::cast::transport::PacketReceiverCallback packet_callback_;
+  media::cast::transport::CastTransportStatusCallback status_callback_;
+  media::cast::transport::CastTransportRtpStatistics audio_rtp_callback_;
+  media::cast::transport::CastTransportRtpStatistics video_rtp_callback_;
+  DISALLOW_COPY_AND_ASSIGN(CastTransportSenderIPC);
+};
+
+// This dispatcher listens to incoming IPC messages and sends
+// the call to the correct CastTransportSenderIPC instance.
+class CastIPCDispatcher : public IPC::ChannelProxy::MessageFilter {

[scherkus (not reviewing), 2014/02/07 00:11:06]

any reason this needs to be inside this .h vs its own .h?

it's also a bit confusing that the implementation of this class in the .cc is
first despite the definition in the .h being second ...

[hubbe, 2014/02/07 00:52:22]

See comment above (factory methods)

+ public:
+  explicit CastIPCDispatcher(
+      const scoped_refptr<base::MessageLoopProxy>& io_message_loop);
+
+  // IPC::ChannelProxy::MessageFilter implementation
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void OnFilterAdded(IPC::Channel* channel) OVERRIDE;
+  virtual void OnFilterRemoved() OVERRIDE;
+  virtual void OnChannelClosing() OVERRIDE;
+
+ protected:
+  virtual ~CastIPCDispatcher();
+
+ private:
+  void OnReceivedPacket(int32 channel_id, const media::cast::Packet& packet);
+  void OnNotifyStatusChange(
+      int32 channel_id,
+      media::cast::transport::CastTransportStatus status);
+  void OnRtpStatistics(
+      int32 channel_id,
+      bool audio,
+      const media::cast::transport::RtcpSenderInfo& sender_info,
+      base::TimeTicks time_sent,
+      uint32 rtp_timestamp);
+
+  friend CastTransportSenderIPC;
+  static CastIPCDispatcher* global_instance_;
+
+  // IPC channel for Send(); must only be accesed on |io_message_loop_|.
+  IPC::Channel* channel_;
+
+  // Message loop on which IPC calls are driven.
+  const scoped_refptr<base::MessageLoopProxy> io_message_loop_;
+
+  // A map of stream ids to delegates; must only be accessed on
+  // |io_message_loop_|.
+  IDMap<CastTransportSenderIPC> id_map_;
+  DISALLOW_COPY_AND_ASSIGN(CastIPCDispatcher);
+};
+
+}  // namespace cast
+
+#endif  // CHROME_RENDERER_MEDIA_CAST_TRANSPORT_SENDER_IPC_H_