Cast:Transport: Dividing A/V Initialization pipeline

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 d5133b61cc0259899b38a5d422f96ca694b321a3..463c0bdfa1c9710f3503669730c83e1b01b0d946 100644
--- a/chrome/renderer/media/cast_session_delegate.cc
+++ b/chrome/renderer/media/cast_session_delegate.cc
@@ -26,7 +26,8 @@ CastSessionDelegate::CastSessionDelegate()
       video_encode_thread_("CastVideoEncodeThread"),
       transport_configured_(false),
       io_message_loop_proxy_(
-          content::RenderThread::Get()->GetIOMessageLoopProxy()) {
+          content::RenderThread::Get()->GetIOMessageLoopProxy()),
+      weak_factory_(this) {
   DCHECK(io_message_loop_proxy_);
 }
 
@@ -105,36 +106,37 @@ void CastSessionDelegate::StartSendingInternal() {
 
   Initialize();
 
-  media::cast::transport::CastTransportConfig config;
+  media::cast::transport::CastTransportAudioConfig audio_config;
+  media::cast::transport::CastTransportVideoConfig video_config;
 
   // TODO(hubbe): set config.aes_key and config.aes_iv_mask.
-  config.local_endpoint = local_endpoint_;
-  config.receiver_endpoint = remote_endpoint_;
+  cast_transport_.reset(new CastTransportSenderIPC(
+      local_endpoint_,
+      remote_endpoint_,
+      base::Bind(&CastSessionDelegate::StatusNotificationCB,
+                 weak_factory_.GetWeakPtr())));

[hubbe, 2014/02/21 23:41:23]

How can we get callbacks if the CastTransportSenderIPC has been destroyed? Why
do we need weak pointers?

[mikhal1, 2014/02/24 17:02:50]

Specifically here, probably not. But we already have the weak_ptr, so how is
this harmful?
On 2014/02/21 23:41:23, hubbe wrote:

[hubbe, 2014/02/25 00:37:04]

It is harmful in two ways:
1: There is extra overhead for the CPU when running the callback
2: Whenever you do something redundant, people who read the code will go: Why? 
(Extra overhead for code readers...)

[Ami GONE FROM CHROMIUM, 2014/02/25 00:57:25]

FWIW I consider #1 above to be a non-issue, and #2 to be a huge issue.
If cast_transport_ is owner by |this| for its entire lifetime (which I believe
it is) and if cast_transport_ doesn't hand off its status_callback_ to anyone
else (which I don't think it can) and if all the dtors & callbacks in question
run on the same thread (which I think they do) using a base::Unretained(this) is
safe.  Best to include a comment explaining why that is.

[mikhal1, 2014/02/25 18:07:02]

Back to unretained, added a comment,
On 2014/02/25 00:57:25, Ami Fischman wrote:

   if (audio_config_) {
-    config.audio_ssrc = audio_config_->sender_ssrc;
-    config.audio_codec = audio_config_->codec;
-    config.audio_rtp_config = audio_config_->rtp_config;
-    config.audio_frequency = audio_config_->frequency;
-    config.audio_channels = audio_config_->channels;
+    audio_config.base.ssrc = audio_config_->sender_ssrc;
+    audio_config.codec = audio_config_->codec;
+    audio_config.base.rtp_config = audio_config_->rtp_config;
+    audio_config.frequency = audio_config_->frequency;
+    audio_config.channels = audio_config_->channels;
+    cast_transport_->InitializeAudio(audio_config);
   }
   if (video_config_) {
-    config.video_ssrc = video_config_->sender_ssrc;
-    config.video_codec = video_config_->codec;
-    config.video_rtp_config = video_config_->rtp_config;
+    video_config.base.ssrc = video_config_->sender_ssrc;
+    video_config.codec = video_config_->codec;
+    video_config.base.rtp_config = video_config_->rtp_config;
+    cast_transport_->InitializeVideo(video_config);
   }
 
-  cast_transport_.reset(new CastTransportSenderIPC(
-      config,
-      base::Bind(&CastSessionDelegate::StatusNotificationCB,
-                 base::Unretained(this))));
-
   cast_sender_.reset(CastSender::CreateCastSender(
       cast_environment_,
       audio_config_.get(),
       video_config_.get(),
       NULL,  // GPU.
       base::Bind(&CastSessionDelegate::InitializationResult,
-                 base::Unretained(this)),
+                 weak_factory_.GetWeakPtr()),
       cast_transport_.get()));
   cast_transport_->SetPacketReceiver(cast_sender_->packet_receiver());
 }