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

media/filters/video_renderer_base.cc

 // Copyright (c) 2011 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 "base/bind.h"
 #include "base/callback.h"
 #include "base/threading/platform_thread.h"
 #include "media/base/buffers.h"
 #include "media/base/callback.h"
 #include "media/base/filter_host.h"
@@ skipping 111 matching lines @@
 
   // Signal the subclass we're stopping.
   OnStop(callback);
 }
 
 void VideoRendererBase::SetPlaybackRate(float playback_rate) {
   base::AutoLock auto_lock(lock_);
   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_);
+void VideoRendererBase::Seek(base::TimeDelta time, const FilterStatusCB&  cb) {
+  bool run_callback = false;
 
-  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);
+  {
+    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,
                                    FilterCallback* callback,
                                    StatisticsCallback* stats_callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK(decoder);
   DCHECK(callback);
   DCHECK(stats_callback);
   DCHECK_EQ(kUninitialized, state_);
@@ skipping 291 matching lines @@
       first_frame = frames_queue_ready_.front();
       if (!first_frame->IsEndOfStream()) {
         frames_queue_ready_.pop_front();
         current_frame_ = first_frame;
       }
       new_frame_available = true;
 
       // 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_) {
     OnFlushDone();
   }
 
   if (new_frame_available) {
     base::AutoUnlock auto_unlock(lock_);
     OnFrameAvailable();
   }
@@ skipping 92 matching lines @@
 
   // Scale our sleep based on the playback rate.
   // TODO(scherkus): floating point badness and degrade gracefully.
   return base::TimeDelta::FromMicroseconds(
       static_cast<int64>(sleep.InMicroseconds() / playback_rate));
 }
 
 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:
     case kPaused:
     case kFlushed:
     case kEnded:
     case kPlaying:
       break;
 
     case kFlushing:
       CHECK(flush_callback_.get());
       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.";
       return;
 
     case kError:
       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())
     callback->Run();
 }
 
 }  // namespace media