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..bc0f65a3f1d887d9ff923d4b3951134d8f60cfc9 100644
--- a/media/cast/cast_sender_impl.cc
+++ b/media/cast/cast_sender_impl.cc
@@ -12,39 +12,69 @@
 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.
+// This makes the cast sender interface thread safe.
 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) {}
+
+  void SetAudioSender(base::WeakPtr<AudioSender> audio_sender) OVERRIDE {
+    DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+    audio_sender_ = audio_sender;
+  }
+
+  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_,
-                                           video_frame,
-                                           capture_time));
+    if (!video_sender_) {
+      NOTREACHED();

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

Then why not simply
CHECK(video_sender_);
?

[Alpha Left Google, 2014/02/13 19:53:59]

NOTREACHED() should be a DCHECK. So this should be DCHECK(video_sender_);

[mikhal1, 2014/02/14 18:03:16]

Adding the DCHECKs cause the code to crash when calling the wek_ptr, as it calls
audio/video sender on different threads.
On 2014/02/13 19:53:59, Alpha wrote:

+      return;
+    }
+    cast_environment_->PostTask(
+        CastEnvironment::MAIN,
+        FROM_HERE,
+        base::Bind(&LocalFrameInput::InsertRawVideoOnMainThread,
+                   base::Unretained(this),

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

what makes Unretained safe?

(in general, any Unretained should be explained away by a comment somewhere...)

[Alpha Left Google, 2014/02/13 19:53:59]

FrameInput is RefCountedThreadSafe so there's no need to enclose this by
base::Unretained.

+                   video_frame,
+                   capture_time));
+  }
+
+  void InsertRawVideoOnMainThread(
+      const scoped_refptr<media::VideoFrame>& video_frame,
+      const base::TimeTicks& capture_time) {
+    DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+    video_sender_->InsertRawVideoFrame(video_frame, capture_time);

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

What if video_sender_'s been invalidated by this point?
I suspect what you really want (instead of the Unretained at l.44) is something
more like
Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame, capture_time)
at l.44, and then you can drop this method?

[Alpha Left Google, 2014/02/13 19:53:59]

video_sender_ is a WeakPtr. That's why it's dereferenced in a separate method.

[Ami GONE FROM CHROMIUM, 2014/02/13 20:43:50]

I don't understand this comment.
Bind is magic when given a weakptr (the way I suggested) but the way it's used
in deref'd in the separate method now the magic is not used and instead the
weakptr is unconditionally deref'd which will crash after invalidation.

[Alpha Left Google, 2014/02/13 20:50:35]

The reason doing a Bind in line 44 doesn't work is because InsertRawVideoFrame
can be called on any thread while SetVideoSender is called on the main thread.
There's a race condition for accessing video_sender_.

And you're right doing a null check for audio_sender_ and video_sender_. It
should be done in InsertRawVideoOnMainThread and InsertAudioOnMainThread.

[Ami GONE FROM CHROMIUM, 2014/02/13 20:57:31]

I think you misunderstood my suggestion.
I'm suggesting the new code should be:
cast_environment_->PostTask(
    CastEnvironment::MAIN,
    FROM_HERE,
    base::Bind(&VideoSender::InsertRawVideoFrame, video_sender, video_frame,
capture_time));

[Ami GONE FROM CHROMIUM, 2014/02/13 21:02:30]

hclam@ points out in person that if SetVideoSender is called concurrently with
or after the Post happens then the bound video_sender_ would be
outdated/wrong/NULL.
If it can't be guaranteed that SetVideoSender is called only before the first
invocation of InsertRawVideoFrame then a helper/trampoline will be required.

[mikhal1, 2014/02/14 18:03:16]

Redone. Let me know what you think.
On 2014/02/13 21:02:30, Ami Fischman wrote:

   }
 
   virtual void InsertAudio(const AudioBus* audio_bus,

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

same comments apply to audio below as video above.

[mikhal1, 2014/02/14 18:03:16]

Done.

                            const base::TimeTicks& recorded_time,
                            const base::Closure& done_callback) OVERRIDE {
-    cast_environment_->PostTask(CastEnvironment::MAIN,
-                                FROM_HERE,
-                                base::Bind(&AudioSender::InsertAudio,
-                                           audio_sender_,
-                                           audio_bus,
-                                           recorded_time,
-                                           done_callback));
+    if (!audio_sender_) {
+      NOTREACHED();
+      return;
+    }
+    cast_environment_->PostTask(
+        CastEnvironment::MAIN,
+        FROM_HERE,
+        base::Bind(&LocalFrameInput::InsertAudioOnMainThread,
+                   base::Unretained(this),
+                   audio_bus,
+                   recorded_time,
+                   done_callback));
+  }
+
+  void InsertAudioOnMainThread(const AudioBus* audio_bus,
+                               const base::TimeTicks& recorded_time,
+                               const base::Closure& done_callback) {
+    DCHECK(cast_environment_->CurrentlyOn(CastEnvironment::MAIN));
+    audio_sender_->InsertAudio(audio_bus, recorded_time, done_callback);
   }
 
  protected:
@@ -62,82 +92,66 @@ class LocalFrameInput : public FrameInput {
 
 CastSender* CastSender::CreateCastSender(
     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,

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

fwiw in media/ code we've followed a convention that FooCallback param names are
suffixed with _cb

[mikhal1, 2014/02/14 18:03:16]

Done.

     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, 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,
     transport::CastTransportSender* const transport_sender)
-    : initialization_callback_(initialization_status),
+    : initialization_callback_(cast_initialization),
       packet_receiver_(
           base::Bind(&CastSenderImpl::ReceivedPacket, base::Unretained(this))),

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

I'm guessing Unretained here to avoid a reference cycle, but should doco why
it's safe.

[mikhal1, 2014/02/14 18:03:16]

Done.

       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) {
+  CHECK(audio_config.use_external_encoder ||

[Alpha Left Google, 2014/02/13 19:53:59]

DCHECK that this is called on the cast main thread.

[mikhal1, 2014/02/14 18:03:16]

Done.

+        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) {
+  CHECK(video_config.use_external_encoder ||

[Alpha Left Google, 2014/02/13 19:53:59]

DCHECK that this is called on the cast main thread.

[mikhal1, 2014/02/14 18:03:16]

Done.

+        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() {}