Move Pipeline permanent callbacks into Pipeline::Client interface.

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index d1f5f2e384f0ddc5805052c11e673fac9da0fd6c..df6d23e89ff9a12fd2ffcefcfb9f44943d71d46c 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -166,7 +166,7 @@ WebMediaPlayerImpl::WebMediaPlayerImpl(
                      base::Unretained(this)),
           base::Bind(&WebMediaPlayerImpl::OnPipelineSeeked, AsWeakPtr()),
           base::Bind(&WebMediaPlayerImpl::OnPipelineSuspended, AsWeakPtr()),
-          base::Bind(&WebMediaPlayerImpl::OnPipelineError, AsWeakPtr())),
+          base::Bind(&WebMediaPlayerImpl::OnError, AsWeakPtr())),
       load_type_(LoadTypeURL),
       opaque_(false),
       playback_rate_(0.0),
@@ -441,9 +441,8 @@ void WebMediaPlayerImpl::DoSeek(base::TimeDelta time, bool time_updated) {
     // ready state change to eventually happen.
     if (old_state == ReadyStateHaveEnoughData) {
       main_task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&WebMediaPlayerImpl::OnPipelineBufferingStateChanged,
-                     AsWeakPtr(), BUFFERING_HAVE_ENOUGH));
+          FROM_HERE, base::Bind(&WebMediaPlayerImpl::OnBufferingStateChange,
+                                AsWeakPtr(), BUFFERING_HAVE_ENOUGH));
     }
     return;
   }
@@ -850,15 +849,6 @@ void WebMediaPlayerImpl::OnFFmpegMediaTracksUpdated(
   }
 }
 
-void WebMediaPlayerImpl::OnWaitingForDecryptionKey() {
-  encrypted_client_->didBlockPlaybackWaitingForKey();
-
-  // TODO(jrummell): didResumePlaybackBlockedForKey() should only be called
-  // when a key has been successfully added (e.g. OnSessionKeysChange() with
-  // |has_additional_usable_key| = true). http://crbug.com/461903
-  encrypted_client_->didResumePlaybackBlockedForKey();
-}
-
 void WebMediaPlayerImpl::SetCdm(const CdmAttachedCB& cdm_attached_cb,
                                 CdmContext* cdm_context) {
   if (!cdm_context) {
@@ -927,46 +917,51 @@ void WebMediaPlayerImpl::OnPipelineSuspended() {
   }
 }
 
-void WebMediaPlayerImpl::OnPipelineEnded() {
-  DVLOG(1) << __FUNCTION__;
+void WebMediaPlayerImpl::OnDemuxerOpened() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
-
-  // Ignore state changes until we've completed all outstanding operations.
-  if (!pipeline_controller_.IsStable())
-    return;
-
-  ended_ = true;
-  client_->timeChanged();
-
-  // We don't actually want this to run until |client_| calls seek() or pause(),
-  // but that should have already happened in timeChanged() and so this is
-  // expected to be a no-op.
-  UpdatePlayState();
+  client_->mediaSourceOpened(
+      new WebMediaSourceImpl(chunk_demuxer_, media_log_));
 }
 
-void WebMediaPlayerImpl::OnPipelineError(PipelineStatus error) {
+void WebMediaPlayerImpl::OnError(PipelineStatus status) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(main_task_runner_->BelongsToCurrentThread());
-  DCHECK_NE(error, PIPELINE_OK);
+  DCHECK_NE(status, PIPELINE_OK);
 
   if (suppress_destruction_errors_)
     return;
 
-  media_log_->AddEvent(media_log_->CreatePipelineErrorEvent(error));
+  media_log_->AddEvent(media_log_->CreatePipelineErrorEvent(status));
 
   if (ready_state_ == WebMediaPlayer::ReadyStateHaveNothing) {
     // Any error that occurs before reaching ReadyStateHaveMetadata should
     // be considered a format error.
     SetNetworkState(WebMediaPlayer::NetworkStateFormatError);
   } else {
-    SetNetworkState(PipelineErrorToNetworkState(error));
+    SetNetworkState(PipelineErrorToNetworkState(status));
   }
 
   UpdatePlayState();
 }
 
-void WebMediaPlayerImpl::OnPipelineMetadata(
-    PipelineMetadata metadata) {
+void WebMediaPlayerImpl::OnEnded() {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(main_task_runner_->BelongsToCurrentThread());
+
+  // Ignore state changes until we've completed all outstanding operations.
+  if (!pipeline_controller_.IsStable())
+    return;
+
+  ended_ = true;
+  client_->timeChanged();
+
+  // We don't actually want this to run until |client_| calls seek() or pause(),
+  // but that should have already happened in timeChanged() and so this is
+  // expected to be a no-op.
+  UpdatePlayState();
+}
+
+void WebMediaPlayerImpl::OnMetadata(PipelineMetadata metadata) {
   DVLOG(1) << __FUNCTION__;
 
   pipeline_metadata_ = metadata;
@@ -995,9 +990,8 @@ void WebMediaPlayerImpl::OnPipelineMetadata(
   UpdatePlayState();
 }
 
-void WebMediaPlayerImpl::OnPipelineBufferingStateChanged(
-    BufferingState buffering_state) {
-  DVLOG(1) << __FUNCTION__ << "(" << buffering_state << ")";
+void WebMediaPlayerImpl::OnBufferingStateChange(BufferingState state) {
+  DVLOG(1) << __FUNCTION__ << "(" << state << ")";
 
   // Ignore buffering state changes until we've completed all outstanding
   // operations.
@@ -1006,7 +1000,7 @@ void WebMediaPlayerImpl::OnPipelineBufferingStateChanged(
 
   // TODO(scherkus): Handle other buffering states when Pipeline starts using
   // them and translate them ready state changes http://crbug.com/144683
-  DCHECK_EQ(buffering_state, BUFFERING_HAVE_ENOUGH);
+  DCHECK_EQ(state, BUFFERING_HAVE_ENOUGH);
   SetReadyState(WebMediaPlayer::ReadyStateHaveEnoughData);
 
   // Let the DataSource know we have enough data. It may use this information to
@@ -1025,15 +1019,17 @@ void WebMediaPlayerImpl::OnPipelineBufferingStateChanged(
   UpdatePlayState();
 }
 
-void WebMediaPlayerImpl::OnDemuxerOpened() {
-  DCHECK(main_task_runner_->BelongsToCurrentThread());
-  client_->mediaSourceOpened(
-      new WebMediaSourceImpl(chunk_demuxer_, media_log_));
+void WebMediaPlayerImpl::OnDurationChange() {
+  // TODO(sandersd): We should call delegate_->DidPlay() with the new duration,
+  // especially if it changed from  <5s to >5s.
+  if (ready_state_ == WebMediaPlayer::ReadyStateHaveNothing)
+    return;
+
+  client_->durationChanged();
 }
 
-void WebMediaPlayerImpl::OnAddTextTrack(
-    const TextTrackConfig& config,
-    const AddTextTrackDoneCB& done_cb) {
+void WebMediaPlayerImpl::OnAddTextTrack(const TextTrackConfig& config,
+                                        const AddTextTrackDoneCB& done_cb) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   const WebInbandTextTrackImpl::Kind web_kind =
@@ -1054,6 +1050,15 @@ void WebMediaPlayerImpl::OnAddTextTrack(
   done_cb.Run(std::move(text_track));
 }
 
+void WebMediaPlayerImpl::OnWaitingForDecryptionKey() {
+  encrypted_client_->didBlockPlaybackWaitingForKey();
+
+  // TODO(jrummell): didResumePlaybackBlockedForKey() should only be called
+  // when a key has been successfully added (e.g. OnSessionKeysChange() with
+  // |has_additional_usable_key| = true). http://crbug.com/461903
+  encrypted_client_->didResumePlaybackBlockedForKey();
+}
+
 void WebMediaPlayerImpl::OnHidden() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   UpdatePlayState();
@@ -1249,7 +1254,7 @@ void WebMediaPlayerImpl::StartPipeline() {
                                      encrypted_media_init_data_cb,
                                      media_tracks_updated_cb, media_log_));
 #else
-    OnPipelineError(PipelineStatus::DEMUXER_ERROR_COULD_NOT_OPEN);
+    OnError(PipelineStatus::DEMUXER_ERROR_COULD_NOT_OPEN);
     return;
 #endif
   } else {
@@ -1271,14 +1276,7 @@ void WebMediaPlayerImpl::StartPipeline() {
 
   // TODO(sandersd): On Android, defer Start() if the tab is not visible.
   bool is_streaming = (data_source_ && data_source_->IsStreaming());
-  pipeline_controller_.Start(
-      demuxer_.get(), is_streaming, is_static,
-      BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineEnded),
-      BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineMetadata),
-      BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnPipelineBufferingStateChanged),
-      BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnDurationChanged),
-      BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnAddTextTrack),
-      BIND_TO_RENDER_LOOP(&WebMediaPlayerImpl::OnWaitingForDecryptionKey));
+  pipeline_controller_.Start(demuxer_.get(), this, is_streaming, is_static);

[xhwang, 2016/04/25 22:49:45]

In the old code we use BIND_TO_RENDER_LOOP and BindToCurrentLoop a lot in WMPI
and PipelineController so that all callbacks will be fired on the main render
thread. The reason of this is that historically WMPI (as a blink WMP
implementation), lives exclusively on the main render thread. In theory,
PipelineImpl should the only class that knows about both threads.

Now with this CL, it appears we could call into Blink on the main thread (e.g.
encrypted_client_->didBlockPlaybackWaitingForKey()), which seems wrong.

Can we provide a Pipeline::Client implementation (e.g. ClientProxy) that makes
sure all calls are forwarded to the main render thread to avoid this issue?

[sandersd (OOO until July 31), 2016/04/26 23:16:35]

I would argue that Pipeline should take a loop along with the client interface,
and should be responsible for posting those calls. This isn't different, other
than whether there is an additional helper class involved.

[xhwang, 2016/04/26 23:35:49]

That would also work for me.

[alokp, 2016/04/28 00:29:39]

Good catch - I missed the threading considerations. After much delibration I
decided to push all the threading logic to PipelineImpl because as Xiaohan
mentioned that is the only class that knows about the existence of multiple
threads.

Now the clients of PipelineImpl use it on the main thread and receive all
notifications/callbacks on the main thread. I also made Pipeline::Stop blocking
so that the clients do not need to worry about waiting for stop_cb.

There is still copious amount of logic and ugliness inside PipelineImpl but it
is at least isolated in one file. The client interface is much cleaner at least
from threading POV. I have a bunch of DCHECK to ensure that each function is
running on correct thread and a few TODOs to add to a some other functions.
PTAL.

 }
 
 void WebMediaPlayerImpl::SetNetworkState(WebMediaPlayer::NetworkState state) {
@@ -1320,15 +1318,6 @@ double WebMediaPlayerImpl::GetPipelineDuration() const {
   return duration.InSecondsF();
 }
 
-void WebMediaPlayerImpl::OnDurationChanged() {
-  // TODO(sandersd): We should call delegate_->DidPlay() with the new duration,
-  // especially if it changed from  <5s to >5s.
-  if (ready_state_ == WebMediaPlayer::ReadyStateHaveNothing)
-    return;
-
-  client_->durationChanged();
-}
-
 void WebMediaPlayerImpl::OnNaturalSizeChanged(gfx::Size size) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DCHECK_NE(ready_state_, WebMediaPlayer::ReadyStateHaveNothing);