Allow transitioning to HAVE_METADATA before pipeline initialization completes.

media/base/pipeline.cc

Index: media/base/pipeline.cc
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 55f8a408f3d392aca5e1065a93954fad5fd49c53..df942f28897727c981e2ded4003632bc3b7a547b 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -22,6 +22,7 @@
 #include "media/base/filter_collection.h"
 #include "media/base/media_log.h"
 #include "media/base/video_decoder.h"
+#include "media/base/video_decoder_config.h"
 #include "media/base/video_renderer.h"
 
 using base::TimeDelta;
@@ -108,14 +109,15 @@ Pipeline::~Pipeline() {
 void Pipeline::Start(scoped_ptr<FilterCollection> collection,
                      const PipelineStatusCB& ended_cb,
                      const PipelineStatusCB& error_cb,
-                     const PipelineStatusCB& start_cb) {
+                     const PipelineStatusCB& seek_cb,
+                     const ReadyStateCB& ready_state_cb) {
   base::AutoLock auto_lock(lock_);
   CHECK(!running_) << "Media pipeline is already running";
 
   running_ = true;
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Pipeline::StartTask, this, base::Passed(&collection),
-      ended_cb, error_cb, start_cb));
+      ended_cb, error_cb, seek_cb, ready_state_cb));
 }
 
 void Pipeline::Stop(const base::Closure& stop_cb) {
@@ -281,12 +283,14 @@ void Pipeline::ReportStatus(const PipelineStatusCB& cb, PipelineStatus status) {
     error_cb_.Reset();
 }
 
-void Pipeline::FinishInitialization() {
+void Pipeline::FinishSeek() {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  seek_timestamp_ = kNoTimestamp();
+  seek_pending_ = false;
+
   // Execute the seek callback, if present.  Note that this might be the
   // initial callback passed into Start().
   ReportStatus(seek_cb_, status_);
-  seek_timestamp_ = kNoTimestamp();
   seek_cb_.Reset();
 }
 
@@ -551,7 +555,8 @@ void Pipeline::OnUpdateStatistics(const PipelineStatistics& stats) {
 void Pipeline::StartTask(scoped_ptr<FilterCollection> filter_collection,
                          const PipelineStatusCB& ended_cb,
                          const PipelineStatusCB& error_cb,
-                         const PipelineStatusCB& start_cb) {
+                         const PipelineStatusCB& seek_cb,
+                         const ReadyStateCB& ready_state_cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   CHECK_EQ(kCreated, state_)
       << "Media pipeline cannot be started more than once";
@@ -559,7 +564,8 @@ void Pipeline::StartTask(scoped_ptr<FilterCollection> filter_collection,
   filter_collection_ = filter_collection.Pass();
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
-  seek_cb_ = start_cb;
+  seek_cb_ = seek_cb;
+  ready_state_cb_ = ready_state_cb;
 
   // Kick off initialization.
   pipeline_init_state_.reset(new PipelineInitState());
@@ -625,9 +631,15 @@ void Pipeline::InitializeTask(PipelineStatus last_stage_status) {
   // Assuming audio renderer was created, create video renderer.
   if (state_ == kInitAudioRenderer) {
     SetState(kInitVideoRenderer);
-    if (InitializeVideoRenderer(demuxer_->GetStream(DemuxerStream::VIDEO))) {
+    scoped_refptr<DemuxerStream> video_stream =
+        demuxer_->GetStream(DemuxerStream::VIDEO);
+    if (InitializeVideoRenderer(video_stream)) {
       base::AutoLock auto_lock(lock_);
       has_video_ = true;
+
+      // Get an initial natural size so we have something when we signal
+      // the kHaveMetadata ready state.
+      natural_size_ = video_stream->video_decoder_config().natural_size();
       return;
     }
   }
@@ -647,6 +659,9 @@ void Pipeline::InitializeTask(PipelineStatus last_stage_status) {
     PlaybackRateChangedTask(GetPlaybackRate());
     VolumeChangedTask(GetVolume());
 
+    if (!ready_state_cb_.is_null())

[Ami GONE FROM CHROMIUM, 2012/08/16 03:31:56]

This is only here for the benefit of things like player_x11, right?
I don't like that the nullity of CBs needs to be checked at each call-site, and
would much rather DCHECK non-nullity at assignment time and give tools that
don't care an explicit DoNothing CB to feed the args.

[acolwell GONE FROM CHROMIUM, 2012/08/16 16:36:03]

Done.

+      ready_state_cb_.Run(kHaveMetadata);
+
     // Fire a seek request to get the renderers to preroll. We can skip a seek
     // here as the demuxer should be at the start of the stream.
     seek_pending_ = true;
@@ -904,10 +919,14 @@ void Pipeline::FilterStateTransitionTask() {
       NOTREACHED() << "Unexpected state: " << state_;
     }
   } else if (state_ == kStarted) {
-    FinishInitialization();
 
-    // Finally, complete the seek.
-    seek_pending_ = false;
+    // Fire canplaythrough immediately after playback begins because of
+    // crbug.com/106480.
+    // TODO(vrk): set ready state to HaveFutureData when bug above is fixed.
+    if (!ready_state_cb_.is_null() && status_ == PIPELINE_OK)
+      ready_state_cb_.Run(kHavePrerolled);

[Ami GONE FROM CHROMIUM, 2012/08/16 03:31:56]

Does it make sense to reset this CB at this point?
More generally, the .h doco promises to call this whenever the "pipeline's ready
state changes", but the "ready state" terminology implies HTML spec ready state,
which implies there should be many more than 2 states, and correspondingly more
transitions & calls to this CB.  

[acolwell GONE FROM CHROMIUM, 2012/08/16 16:36:03]

No. Technically we should signal a kHaveMetadata when we start to seek and then
this signals kHavePrerolled again once the seek completes. I didn't want to add
the kHaveMetadata because that would be an extra change to the existing behavior
and I wanted to save that for a different CL just in case it caused problems.
Hmm.. Perhaps I should move away from ReadyState in favor some something like
Progress or BufferingState? Right now we don't have logic to support the other
ready states which is why I left them out. I figured they would be added once
the logic was actually there to support them. For instance I can imagine a
kHaveFirstFrame value being added so that we could support transitions to
HAVE_CURRENT_DATA.

[Ami GONE FROM CHROMIUM, 2012/08/16 16:43:47]

Yes, I think using a different name would be a good idea.
In order to come up with that name it might be helpful to enumerate all the
other states you hope to support in the future, to get a sense of the shape of
the thing (but don't add those enum values yet!).

[acolwell GONE FROM CHROMIUM, 2012/08/16 17:55:06]

Done.

+
+    FinishSeek();
 
     // If a playback rate change was requested during a seek, do it now that
     // the seek has compelted.
@@ -1143,7 +1162,7 @@ void Pipeline::TearDownPipeline() {
       SetState(kStopping);
       DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
 
-      FinishInitialization();
+      FinishSeek();
       break;
 
     case kPausing:
@@ -1153,10 +1172,8 @@ void Pipeline::TearDownPipeline() {
       SetState(kStopping);
       DoStop(base::Bind(&Pipeline::OnTeardownStateTransition, this));
 
-      if (seek_pending_) {
-        seek_pending_ = false;
-        FinishInitialization();
-      }
+      if (seek_pending_)
+        FinishSeek();
 
       break;