Cast: Refactoring Cast API's

media/cast/cast_sender_impl.cc

Index: media/cast/cast_sender_impl.cc
diff --git a/media/cast/cast_sender_impl.cc b/media/cast/cast_sender_impl.cc
index 1a97ae4c343a526bdd1e0228cb624125486e55b6..a72b6ee3011b31c00d8e3606c37d739ef033207c 100644
--- a/media/cast/cast_sender_impl.cc
+++ b/media/cast/cast_sender_impl.cc
@@ -12,17 +12,25 @@
 namespace media {
 namespace cast {
 
-// The LocalFrameInput class posts all incoming frames; audio and video to the
-// main cast thread for processing.
-// This make the cast sender interface thread safe.
+// The LocalFrameInput class posts all incoming frames, both audio and video, to
+// the main cast thread for processing., therefore frames can be inserted from
+// any thread.
 class LocalFrameInput : public FrameInput {
  public:
-  LocalFrameInput(scoped_refptr<CastEnvironment> cast_environment,
-                  base::WeakPtr<AudioSender> audio_sender,
-                  base::WeakPtr<VideoSender> video_sender)
-      : cast_environment_(cast_environment),
-        audio_sender_(audio_sender),
-        video_sender_(video_sender) {}
+  LocalFrameInput(scoped_refptr<CastEnvironment> cast_environment)
+      : cast_environment_(cast_environment) {}
+
+  virtual void SetAudioSender(base::WeakPtr<AudioSender> audio_sender)
+      OVERRIDE {
+    DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+    audio_sender_ = audio_sender;
+  }
+
+  virtual void SetVideoSender(base::WeakPtr<VideoSender> video_sender)
+      OVERRIDE {
+    DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+    video_sender_ = video_sender;
+  }
 
   virtual void InsertRawVideoFrame(
       const scoped_refptr<media::VideoFrame>& video_frame,
@@ -60,84 +68,72 @@ class LocalFrameInput : public FrameInput {
   DISALLOW_COPY_AND_ASSIGN(LocalFrameInput);
 };
 
-CastSender* CastSender::CreateCastSender(
+CastSender* CastSender::Create(
     scoped_refptr<CastEnvironment> cast_environment,
-    const AudioSenderConfig* audio_config,
-    const VideoSenderConfig* video_config,
-    const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories,
-    const CastInitializationCallback& initialization_status,
+    const CastInitializationCallback& cast_initialization_cb,
     transport::CastTransportSender* const transport_sender) {
   CHECK(cast_environment);
-  return new CastSenderImpl(cast_environment,
-                            audio_config,
-                            video_config,
-                            gpu_factories,
-                            initialization_status,
-                            transport_sender);
+  return new CastSenderImpl(
+      cast_environment, cast_initialization_cb, transport_sender);
 }
 
 CastSenderImpl::CastSenderImpl(
     scoped_refptr<CastEnvironment> cast_environment,
-    const AudioSenderConfig* audio_config,
-    const VideoSenderConfig* video_config,
-    const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories,
-    const CastInitializationCallback& initialization_status,
+    const CastInitializationCallback& cast_initialization_cb,
     transport::CastTransportSender* const transport_sender)
-    : initialization_callback_(initialization_status),
+    : initialization_callback_(cast_initialization_cb),
+      // ReceivedPacket is valid as long as CastSenderImpl is valid, and
+      // therefore we can call base::Unretained.

[Ami GONE FROM CHROMIUM, 2014/02/14 18:23:54]

Class is valid as long as OtherClass is valid
doesn't read right to me, because the "validity" being discussed is of
instances, not classes.

You need to explain how it is guaranteed that packet_receiver_ will not be
.Run() after |this| is deleted in order to justify the use of Unretained(this).

If my code-skimming is right, that means that whoever called packet_receiver()
needs to be told to stop using it before |this| is deleted.

But if that's already being done (and the receiver callback is dropped in the
process) then I think you can avoid the Unretained by dropping packet_receiver_
entirely and returning a newly-bound (on a refcounted this) callback in
packet_receiver() (which will need to be renamed PacketReceiver() to reflect its
non-getter-ness).  Then you won't have to worry about a ref cycle b/c the
callback is dropped during teardown.
Of course, that's only ref-cycle-free _if_ the previous paragraph really holds.

[mikhal1, 2014/02/18 19:20:43]

I think you may be on to something here :o
Removed, as this is no longer needed (probably part of the bad merge).
On 2014/02/14 18:23:54, Ami Fischman wrote:

       packet_receiver_(
           base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this))),
       cast_environment_(cast_environment),
+      transport_sender_(transport_sender),
+      ssrc_of_audio_sender_(0),
+      ssrc_of_video_sender_(0),
       weak_factory_(this) {
   CHECK(cast_environment);
-  CHECK(audio_config || video_config);
-
-  base::WeakPtr<AudioSender> audio_sender_ptr;
-  base::WeakPtr<VideoSender> video_sender_ptr;
-
-  if (audio_config) {
-    CHECK(audio_config->use_external_encoder ||
-          cast_environment->HasAudioEncoderThread());
-
-    audio_sender_.reset(
-        new AudioSender(cast_environment, *audio_config, transport_sender));
-    ssrc_of_audio_sender_ = audio_config->incoming_feedback_ssrc;
-    audio_sender_ptr = audio_sender_->AsWeakPtr();
-
-    CastInitializationStatus status = audio_sender_->InitializationResult();
-    if (status != STATUS_INITIALIZED || !video_config) {
-      if (status == STATUS_INITIALIZED && !video_config) {
-        // Audio only.
-        frame_input_ = new LocalFrameInput(
-            cast_environment, audio_sender_ptr, video_sender_ptr);
-      }
-      cast_environment->PostTask(
-          CastEnvironment::MAIN,
-          FROM_HERE,
-          base::Bind(&CastSenderImpl::InitializationResult,
-                     weak_factory_.GetWeakPtr(),
-                     status));
-      return;
-    }
-  }
-  if (video_config) {
-    CHECK(video_config->use_external_encoder ||
-          cast_environment->HasVideoEncoderThread());
-
-    video_sender_.reset(
-        new VideoSender(cast_environment,
-                        *video_config,
-                        gpu_factories,
-                        base::Bind(&CastSenderImpl::InitializationResult,
-                                   weak_factory_.GetWeakPtr()),
-                        transport_sender));
-    video_sender_ptr = video_sender_->AsWeakPtr();
-    ssrc_of_video_sender_ = video_config->incoming_feedback_ssrc;
+  frame_input_ = new LocalFrameInput(cast_environment);
+}
+
+void CastSenderImpl::InitializeAudio(const AudioSenderConfig& audio_config) {
+  DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+  CHECK(audio_config.use_external_encoder ||
+        cast_environment_->HasAudioEncoderThread());
+
+  audio_sender_.reset(
+      new AudioSender(cast_environment_, audio_config, transport_sender_));
+
+  CastInitializationStatus status = audio_sender_->InitializationResult();
+
+  if (status == STATUS_AUDIO_INITIALIZED) {
+    ssrc_of_audio_sender_ = audio_config.incoming_feedback_ssrc;
+    frame_input_->SetAudioSender(audio_sender_->AsWeakPtr());
   }
-  frame_input_ =
-      new LocalFrameInput(cast_environment, audio_sender_ptr, video_sender_ptr);
 
-  // Handing over responsibility to call NotifyInitialization to the
-  // video sender.
+  cast_environment_->PostTask(CastEnvironment::MAIN,
+                              FROM_HERE,
+                              base::Bind(&CastSenderImpl::InitializationResult,
+                                         weak_factory_.GetWeakPtr(),
+                                         status));
+}
+
+void CastSenderImpl::InitializeVideo(
+    const VideoSenderConfig& video_config,
+    const scoped_refptr<GpuVideoAcceleratorFactories>& gpu_factories) {
+  DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+  CHECK(video_config.use_external_encoder ||
+        cast_environment_->HasVideoEncoderThread());
+
+  video_sender_.reset(
+      new VideoSender(cast_environment_,
+                      video_config,
+                      gpu_factories,
+                      base::Bind(&CastSenderImpl::InitializationResult,
+                                 weak_factory_.GetWeakPtr()),
+                      transport_sender_));
+
+  ssrc_of_video_sender_ = video_config.incoming_feedback_ssrc;
+  frame_input_->SetVideoSender(video_sender_->AsWeakPtr());
 }
 
 CastSenderImpl::~CastSenderImpl() {}