Cast:Transport: Dividing A/V Initialization pipeline

chrome/renderer/media/cast_session_delegate.cc

 // Copyright 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.
 
 #include "chrome/renderer/media/cast_session_delegate.h"
 
 #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"
 #include "media/cast/cast_environment.h"
 #include "media/cast/cast_sender.h"
 #include "media/cast/logging/logging_defines.h"
 #include "media/cast/transport/cast_transport_config.h"
 #include "media/cast/transport/cast_transport_sender.h"
 
 using media::cast::AudioSenderConfig;
 using media::cast::CastEnvironment;
 using media::cast::CastSender;
 using media::cast::VideoSenderConfig;
 
 CastSessionDelegate::CastSessionDelegate()
     : audio_encode_thread_("CastAudioEncodeThread"),
       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_);
 }
 
 CastSessionDelegate::~CastSessionDelegate() {
   DCHECK(io_message_loop_proxy_->BelongsToCurrentThread());
 }
 
 void CastSessionDelegate::Initialize() {
   if (cast_environment_)
     return;  // Already initialized.
@@ skipping 58 matching lines @@
   // No transport, wait.
   if (!transport_configured_)
     return;
 
   // No audio or video, wait.
   if (!audio_config_ && !video_config_)
     return;
 
   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());
 }
 
 void CastSessionDelegate::InitializationResult(
     media::cast::CastInitializationStatus result) const {
   DCHECK(cast_sender_);
 
   // TODO(pwestin): handle the error codes.
   if (result == media::cast::STATUS_INITIALIZED) {
     if (!audio_frame_input_available_callback_.is_null()) {
       audio_frame_input_available_callback_.Run(cast_sender_->frame_input());
     }
     if (!video_frame_input_available_callback_.is_null()) {
       video_frame_input_available_callback_.Run(cast_sender_->frame_input());
     }
   }
 }