Cast: Refactoring Cast API's

media/cast/cast_sender_impl.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 "media/cast/cast_sender_impl.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "media/base/video_frame.h"
 
 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,
       const base::TimeTicks& capture_time) OVERRIDE {
     cast_environment_->PostTask(CastEnvironment::MAIN,
                                 FROM_HERE,
                                 base::Bind(&VideoSender::InsertRawVideoFrame,
                                            video_sender_,

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

Since InsertRVF() can be called on any thread, what guarantees that the write to
video_sender_ in SetVideoSender will be observed here?  Sadly I think you need
the trampoline back, or else to introduce a mutex around read/write of
video_sender_.

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

Check out the new version!
On 2014/02/14 18:23:54, Ami Fischman wrote:

                                            video_frame,
                                            capture_time));
   }
 
   virtual void InsertAudio(const AudioBus* audio_bus,
                            const base::TimeTicks& recorded_time,
                            const base::Closure& done_callback) OVERRIDE {
     cast_environment_->PostTask(CastEnvironment::MAIN,
                                 FROM_HERE,
                                 base::Bind(&AudioSender::InsertAudio,
                                            audio_sender_,

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

ditto

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

Done.

                                            audio_bus,
                                            recorded_time,
                                            done_callback));
   }
 
  protected:
   virtual ~LocalFrameInput() {}
 
  private:
   friend class base::RefCountedThreadSafe<LocalFrameInput>;
 
   scoped_refptr<CastEnvironment> cast_environment_;
   base::WeakPtr<AudioSender> audio_sender_;
   base::WeakPtr<VideoSender> video_sender_;
 
   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);
+  frame_input_ = new LocalFrameInput(cast_environment);
+}
 
-  base::WeakPtr<AudioSender> audio_sender_ptr;
-  base::WeakPtr<VideoSender> video_sender_ptr;
+void CastSenderImpl::InitializeAudio(const AudioSenderConfig& audio_config) {
+  DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+  CHECK(audio_config.use_external_encoder ||
+        cast_environment_->HasAudioEncoderThread());
 
-  if (audio_config) {
-    CHECK(audio_config->use_external_encoder ||
-          cast_environment->HasAudioEncoderThread());
+  audio_sender_.reset(
+      new AudioSender(cast_environment_, audio_config, transport_sender_));
 
-    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();
 
-    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 (status == STATUS_AUDIO_INITIALIZED) {
+    ssrc_of_audio_sender_ = audio_config.incoming_feedback_ssrc;
+    frame_input_->SetAudioSender(audio_sender_->AsWeakPtr());
   }
-  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, audio_sender_ptr, video_sender_ptr);
+  cast_environment_->PostTask(CastEnvironment::MAIN,
+                              FROM_HERE,
+                              base::Bind(&CastSenderImpl::InitializationResult,
+                                         weak_factory_.GetWeakPtr(),
+                                         status));
+}
 
-  // Handing over responsibility to call NotifyInitialization to the
-  // video sender.
+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() {}
 
 // ReceivedPacket handle the incoming packets to the cast sender
 // it's only expected to receive RTCP feedback packets from the remote cast
 // receiver. The class verifies that that it is a RTCP packet and based on the
 // SSRC of the incoming packet route the packet to the correct sender; audio or
 // video.
 //
@@ skipping 49 matching lines @@
   } else {
     VLOG(1) << "Received a RTCP packet with a non matching sender SSRC "
             << ssrc_of_sender;
   }
 }
 
 scoped_refptr<FrameInput> CastSenderImpl::frame_input() { return frame_input_; }
 
 transport::PacketReceiverCallback CastSenderImpl::packet_receiver() {
   return packet_receiver_;
   return base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this));

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

lolwat?

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

hmmm...somewhat speechless. Bad merge?
On 2014/02/14 18:23:54, Ami Fischman wrote:

 }
 
 void CastSenderImpl::InitializationResult(CastInitializationStatus status)
     const {
   initialization_callback_.Run(status);
 }
 
 }  // namespace cast
 }  // namespace media