Cast: IPC glue between cast library transport and encoders.

chrome/renderer/media/cast_session_delegate.cc

Index: chrome/renderer/media/cast_session_delegate.cc
diff --git a/chrome/renderer/media/cast_session_delegate.cc b/chrome/renderer/media/cast_session_delegate.cc
index ca1042e2bf64d945adf58be5b391daf5a94662eb..a163e6c474090508ab056ccb25cf6a07fc96dcfe 100644
--- a/chrome/renderer/media/cast_session_delegate.cc
+++ b/chrome/renderer/media/cast_session_delegate.cc
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "chrome/renderer/media/cast_transport_sender_ipc.h"
 #include "content/public/renderer/p2p_socket_client.h"
 #include "content/public/renderer/render_thread.h"
 #include "media/cast/cast_config.h"
@@ -20,59 +21,15 @@ using media::cast::CastEnvironment;
 using media::cast::CastSender;
 using media::cast::VideoSenderConfig;
 
-namespace {
-
-// This is a dummy class that does nothing. This is needed temporarily
-// to enable tests for cast.streaming extension APIs.
-// The real implementation of CastTransportSender is to use IPC to send
-// data to the browser process.
-// See crbug.com/327482 for more details.
-class DummyTransport : public media::cast::transport::CastTransportSender {
- public:
-  DummyTransport() {}
-  virtual ~DummyTransport() {}
-
-  // CastTransportSender implementations.
-  virtual void SetPacketReceiver(
-      const media::cast::transport::PacketReceiverCallback& packet_receiver)
-      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 {}
-  virtual void ResendPackets(
-      bool is_audio,
-      const media::cast::transport::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:
-  DISALLOW_COPY_AND_ASSIGN(DummyTransport);
-};
-
-}  // namespace
-
 CastSessionDelegate::CastSessionDelegate()
     : audio_encode_thread_("CastAudioEncodeThread"),
       video_encode_thread_("CastVideoEncodeThread"),
       audio_configured_(false),
       video_configured_(false),
+      transport_configured_(false),
       io_message_loop_proxy_(
           content::RenderThread::Get()->GetIOMessageLoopProxy()) {
+  DCHECK(io_message_loop_proxy_.get());

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

don't need .get()

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

Done.

 }
 
 CastSessionDelegate::~CastSessionDelegate() {
@@ -101,6 +58,20 @@ void CastSessionDelegate::StartVideo(
   StartSendingInternal();
 }
 
+void CastSessionDelegate::StartUDP(
+    const net::IPEndPoint& local_endpoint,
+    const net::IPEndPoint& remote_endpoint) {
+  DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
+  transport_configured_ = true;
+  local_endpoint_ = local_endpoint;
+  remote_endpoint_ = remote_endpoint;
+  StartSendingInternal();
+}
+
+static void IgnoreStatusCB(
+    media::cast::transport::CastTransportStatus unused_status) {
+}
+
 // TODO(pwestin): This design does not work; as soon as audio or video is
 // initialized it will prevent the other from being enabled.
 void CastSessionDelegate::StartSendingInternal() {
@@ -109,10 +80,27 @@ void CastSessionDelegate::StartSendingInternal() {
   if (cast_environment_)
     return;
 
-  if (!audio_configured_ || !video_configured_)
+  if (!audio_configured_ || !video_configured_ || !transport_configured_)
     return;
 
-  cast_transport_.reset(new DummyTransport());
+  media::cast::transport::CastTransportConfig config;
+
+  // TODO(hubbe): receiver_endpoint, local_endpoint

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

what is this TODO referring to? unlike the TODO below about aes_key and
aes_iv_mask (which I assume is a TODO to remind you to set these), you are
setting local_endpoint and receiver_endpoint on lines 89-90

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

Yep, this TODO has been removed, sorry about that.

+  config.local_endpoint = local_endpoint_;
+  config.receiver_endpoint = remote_endpoint_;
+  config.audio_ssrc = audio_config_.sender_ssrc;
+  config.video_ssrc = video_config_.sender_ssrc;
+  config.audio_codec = audio_config_.codec;
+  config.video_codec = video_config_.codec;
+  config.audio_rtp_config = audio_config_.rtp_config;
+  config.video_rtp_config = video_config_.rtp_config;
+  config.audio_frequency = audio_config_.frequency;
+  config.audio_channels = audio_config_.channels;
+  // TODO(Hubbe): aes_key, aes_iv_mask

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

nit: s/Hubbe/hubbe/ ?

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

Done.

+
+  cast_transport_.reset(new cast::CastTransportSenderIPC(
+      config,
+      base::Bind(&IgnoreStatusCB)));

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

these sorts of things sadden me

by putting in the callback ignoring code at the endpoint in the renderer, we
have to introduce and maintain the following plumbing:
 - CastIPCDispatcher::OnNotifyStatusChange()
 - CastMsg_NotifyStatusChange IPC message
 - CastTransportHostFilter::NotifyStatusChange()

... only to throw away the result. You'd be amazed at how this type of
well-intentioned code leads to cruft!

Also as far as I can tell, it looks like the status returned is always false
because we're not setting aes_key/aes_iv_mask (the TransportEncryptionHandler
object that eventually is created seems to want properly sized keys)

It'd be nice to at least have a justification of some sort (perhaps by expanding
the TODO about setting aes_key,aes_iv_mask?) or nuking the code in this CL until
things are properly hooked up (e.g., binding the IngoreStatusCB inside of
CastTransportHostFilter vs. CastIPCDispatcher?)

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

I would gladly hook it up, but I don't know how.
I've added another TODO for it, and I fully intend to hook this up to it's
rightful place as soon as I figure out what that is.
pwestin is working on another CL which also adds async callbacks after startup,
so hopefully these two callbacks can actually go to the same place in the
javascript, but that place doesn't exist yet as far as I know.

   audio_encode_thread_.Start();
   video_encode_thread_.Start();
 
@@ -138,6 +126,7 @@ void CastSessionDelegate::StartSendingInternal() {
       base::Bind(&CastSessionDelegate::InitializationResult,
           base::Unretained(this)),
       cast_transport_.get()));
+  cast_transport_->SetPacketReceiver(cast_sender_->packet_receiver());
 }
 
 void CastSessionDelegate::InitializationResult(