Convert Filter::Seek() to use new callback system.

media/filters/video_renderer_base.cc

Index: media/filters/video_renderer_base.cc
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index 92bedba8afbf052965ddf5515aef651c12b65396..475addcad452b671c2a47bd0f8b079dcdaf403dd 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -129,28 +129,37 @@ void VideoRendererBase::SetPlaybackRate(float playback_rate) {
   playback_rate_ = playback_rate;
 }
 
-void VideoRendererBase::Seek(base::TimeDelta time, FilterCallback* callback) {
-  base::AutoLock auto_lock(lock_);
-  // There is a race condition between filters to receive SeekTask().
-  // It turns out we could receive buffer from decoder before seek()
-  // is called on us. so we do the following:
-  // kFlushed => ( Receive first buffer or Seek() ) => kSeeking and
-  // kSeeking => ( Receive enough buffers) => kPrerolled. )
-  DCHECK(kPrerolled == state_ || kFlushed == state_ || kSeeking == state_);
-
-  if (state_ == kPrerolled) {
-    // Already get enough buffers from decoder.
-    callback->Run();
-    delete callback;
-  } else {
-    // Otherwise we are either kFlushed or kSeeking, but without enough buffers;
-    // we should save the callback function and call it later.
-    state_ = kSeeking;
-    seek_callback_.reset(callback);
+void VideoRendererBase::Seek(base::TimeDelta time, const FilterStatusCB&  cb) {
+  bool run_callback = false;
+
+  {
+    base::AutoLock auto_lock(lock_);
+    // There is a race condition between filters to receive SeekTask().
+    // It turns out we could receive buffer from decoder before seek()
+    // is called on us. so we do the following:
+    // kFlushed => ( Receive first buffer or Seek() ) => kSeeking and
+    // kSeeking => ( Receive enough buffers) => kPrerolled. )
+    DCHECK(kPrerolled == state_ || kFlushed == state_ || kSeeking == state_);
+    DCHECK(!cb.is_null());
+    DCHECK(seek_cb_.is_null());
+
+    if (state_ == kPrerolled) {
+      // Already get enough buffers from decoder.
+      run_callback = true;
+
+    } else {
+      // Otherwise we are either kFlushed or kSeeking, but without enough
+      // buffers we should save the callback function and call it later.
+      state_ = kSeeking;
+      seek_cb_ = cb;
+    }
+
+    seek_timestamp_ = time;
+    ScheduleRead_Locked();
   }
 
-  seek_timestamp_ = time;
-  ScheduleRead_Locked();
+  if (run_callback)
+    cb.Run(PIPELINE_OK);
 }
 
 void VideoRendererBase::Initialize(VideoDecoder* decoder,
@@ -462,9 +471,8 @@ void VideoRendererBase::ConsumeVideoFrame(scoped_refptr<VideoFrame> frame) {
       // If we reach prerolled state before Seek() is called by pipeline,
       // |seek_callback_| is not set, we will return immediately during
       // when Seek() is eventually called.
-      if ((seek_callback_.get())) {
-        seek_callback_->Run();
-        seek_callback_.reset();
+      if (!seek_cb_.is_null()) {
+        CopyAndResetCB(seek_cb_).Run(PIPELINE_OK);
       }
     }
   } else if (state_ == kFlushing && pending_reads_ == 0 && !pending_paint_) {
@@ -578,6 +586,8 @@ void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
   lock_.AssertAcquired();
 
   scoped_ptr<FilterCallback> callback;

[Ami GONE FROM CHROMIUM, 2011/05/12 20:42:16]

Typically the reason we have this sort of stack var holding member state is so
that it can be used outside the lock that has to be held to inspect the member
state.  But in this case that doesn't hold - the entire method body is under
lock, so you could as well just fire seek_cb_ inside the switch.

[acolwell GONE FROM CHROMIUM, 2011/05/12 22:30:40]

Done.

+  FilterStatusCB status_cb;
+
   switch (state_) {
     case kUninitialized:
     case kPrerolled:
@@ -592,10 +602,10 @@ void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
       callback.swap(flush_callback_);
       break;
 
-    case kSeeking:
-      CHECK(seek_callback_.get());
-      callback.swap(seek_callback_);
-      break;
+    case kSeeking: {
+      CHECK(!seek_cb_.is_null());
+      status_cb = CopyAndResetCB(seek_cb_);

[Ami GONE FROM CHROMIUM, 2011/05/12 20:42:16]

std::swap() fits in better here, esp. if you take my suggestion about
CopyAndResetCB().

[acolwell GONE FROM CHROMIUM, 2011/05/12 22:30:40]

Done.

+     } break;

[Ami GONE FROM CHROMIUM, 2011/05/12 20:42:16]

braces should be unnecessary.

[acolwell GONE FROM CHROMIUM, 2011/05/12 22:30:40]

Done.

 
     case kStopped:
       NOTREACHED() << "Should not error if stopped.";
@@ -605,6 +615,12 @@ void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
       return;
   }
   state_ = kError;
+
+  if (!status_cb.is_null()) {
+    status_cb.Run(status);
+    return;
+  }
+
   host()->SetError(status);

[Ami GONE FROM CHROMIUM, 2011/05/12 20:42:16]

Is it really right to not set the host's error state if e.g. we were seeking? 
Wouldn't we want to know about the error when doing non-seek operations
following?
I suspect both this and the state_=kError; statement above really belong at the
top of the method, after taking a copy of state_ to switch off of.

[acolwell GONE FROM CHROMIUM, 2011/05/12 22:30:40]

Yes. Calling host()->SetError() is the old style of reporting errors for
FilterCallbacks. The idea is that using FilterStatusCB will replace the
"host()->SetError() + callback->Run()" combo. Once all the methods are converted
over, the SetError() will go away and they will all use RunAndResetCB().
Moved state_ = kError before switch.

[Ami GONE FROM CHROMIUM, 2011/05/12 22:49:25]

I understand this, but I'm questioning whether the intermediate state is safe.

[acolwell GONE FROM CHROMIUM, 2011/05/13 00:49:10]

Sorry I wasn't clear. Yes this is safe because in this case calling status_cb
will call CompositeFilter::OnStatusCB(). host()->SetError() here calls
CompositeFilter::FilterHostImpl::SetError() which ends up calling
CompositeFilter::SetError(). OnStatusCB() triggers the same code paths as this
code.

 
   if (callback.get())