Implemented proper pause-then-seek behaviour for the media pipeline.

Implemented proper pause-then-seek behaviour for the media pipeline.

MediaFilter now has asynchronous Play() and Pause() methods that are implemented by default.  Since we are a completely pull-based system, it turns out only AudioRendererBase and VideoRendererBase need to override them to halt reading from decoders.

When a seek is received by the pipeline, it goes through the following state transitions:
 1) Playing -> Pausing -> Paused (notify filters to stop reading)
 2) Paused -> Seeking (notify filters to flush buffers and preroll)
 3) Seeking -> Playing (preroll completed, resume playback)

The key to this system is that there must be no pending reads in *any* filter when we start transitioning into the seeking state.  That means the audio/video renderers do not complete their pausing->paused transition until they have completed all pending reads.  To reiterate, since we're pulled based if the renderers are not reading any data, then *nothing* is happening in the pipeline.

We get a lot of nice benefits from this assertion, namely no callbacks are ever "lost", they are always replied to until we are fully paused.  Furthermore, we can guarantee that the first buffer received after a Seek() will be guaranteed to be discontinuous.

This change also properly handles end-of-stream, seeking from an end-of-stream state, and displaying frames while paused.  In summary, I was able to call Seek() in a while(true) loop without crashing or going out of sync.

BUG=16014, 16021, 17456
TEST=seeking should be way more robust

[scherkus (not reviewing), 2009-07-23 23:24:56]

Thoroughly tested.  Has no effect on existing pipeline API cilents (other than
seeking works!)

[awong, 2009-07-24 00:25:02]

Couple of small comments.  Will give a more thorough review tomorrow.

http://codereview.chromium.org/160005/diff/84/1046
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/84/1046#newcode546
Line 546: // will only execute the first Seek() request.
Aren't we attempting to have a one-reply per callback gaurantee?

http://codereview.chromium.org/160005/diff/84/1046#newcode572
Line 572: DCHECK_GE(filter_replies_, 0u);
0u?  Isn't this always true?

http://codereview.chromium.org/160005/diff/84/1046#newcode624
Line 624: NOTREACHED() << "Invalid state: " << state_;
What happens if we hit this in production?  Do we need to transition to an ERROR
state?

http://codereview.chromium.org/160005/diff/84/1049
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/160005/diff/84/1049#newcode36
Line 36: scoped_ptr<FilterCallback> c(callback);
Not going to suggest a change, but we're starting to use scoped_ptr quite
heavily for something where we can inspect the deletion within 1-2 lines.  Feels
slightly overkill.  But until we see a problem, I don't care :)

[scherkus (not reviewing), 2009-07-24 18:36:18]

http://codereview.chromium.org/160005/diff/84/1046
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/84/1046#newcode546
Line 546: // will only execute the first Seek() request.
On 2009/07/24 00:25:02, awong wrote:
> Aren't we attempting to have a one-reply per callback gaurantee?

Yeah but I don't know how to handle this situation.

If the client does:
Seek(0, &Foo::A);
Seek(10, &Foo::B);
Seek(20, &Foo::C);

... we can either:
  1) queue these requests and run Seek() after &Foo::A has completed
  2) queue the callbacks and execute all of them after &Foo::A has completed
  3) delete the callbacks and only &Foo::A will get executed (current
implementation)

I'm okay with (2)

http://codereview.chromium.org/160005/diff/84/1046#newcode572
Line 572: DCHECK_GE(filter_replies_, 0u);
On 2009/07/24 00:25:02, awong wrote:
> 0u?  Isn't this always true?

you are better than coverity!

http://codereview.chromium.org/160005/diff/84/1046#newcode624
Line 624: NOTREACHED() << "Invalid state: " << state_;
On 2009/07/24 00:25:02, awong wrote:
> What happens if we hit this in production?  Do we need to transition to an
ERROR
> state?

good catch

we'll transition to an error

http://codereview.chromium.org/160005/diff/84/1049
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/160005/diff/84/1049#newcode36
Line 36: scoped_ptr<FilterCallback> c(callback);
On 2009/07/24 00:25:02, awong wrote:
> Not going to suggest a change, but we're starting to use scoped_ptr quite
> heavily for something where we can inspect the deletion within 1-2 lines. 
Feels
> slightly overkill.  But until we see a problem, I don't care :)

yeah I was thinking "hey, we do the same for AutoLock", but I agree it's a
little overkill

I wonder what it compiles down to...

[Alpha Left Google, 2009-07-24 18:47:58]

http://codereview.chromium.org/160005/diff/84/1044
File media/base/filters.h (right):

http://codereview.chromium.org/160005/diff/84/1044#newcode101
Line 101: // downstream filters, except when they are pre-rolling during a
Seek().
I'm not so sure about the pre-rolling statement. So filters can still read when
they are paused?

http://codereview.chromium.org/160005/diff/84/1046
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/84/1046#newcode356
Line 356: start_callback_.reset(start_callback);
Start is now essentially the same as seek to 0s, so why not just save this to
seek_callback_.

http://codereview.chromium.org/160005/diff/84/1046#newcode452
Line 452: state_ = kSeeking;
Maybe adding some documentation to explain Start is equal to Init -> Seeking ->
Starting -> Started.

http://codereview.chromium.org/160005/diff/84/1046#newcode456
Line 456: NewCallback(this, &PipelineImpl::OnFilterReply));
This is super awesome! Since playing from any position is now considered a seek!
:)

http://codereview.chromium.org/160005/diff/84/1046#newcode546
Line 546: // will only execute the first Seek() request.
We should put a DCHECK here, if we have an outstanding seek we should not
receive another one. I sounds something is wrong if we receive multiple seek
calls.

http://codereview.chromium.org/160005/diff/84/1046#newcode590
Line 590: if (start_callback_.get()) {
If you keep the start_callback in seek_callback, you don't need this :)

http://codereview.chromium.org/160005/diff/84/1047
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160005/diff/84/1047#newcode36
Line 36: //        .-> [ Error ]        .->      [ Stopped ]    <-.
About the state transition of this object, this description is not accurate
anymore due to new states, what about updating it?

http://codereview.chromium.org/160005/diff/84/1052
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/160005/diff/84/1052#newcode208
Line 208: DCHECK(expecting_discontinuous_);
This is great!

http://codereview.chromium.org/160005/diff/84/1057
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/160005/diff/84/1057#newcode153
Line 153: CreateBlackFrame(&current_frame_);
hehe, this is cool, we could any picture while paused :P

http://codereview.chromium.org/160005/diff/84/1057#newcode206
Line 206: if (state_ != kPlaying) {
This if statement looks a little strange to me because you have a DCHECK above..

[awong, 2009-07-24 19:23:54]

A few more comments.

http://codereview.chromium.org/160005/diff/88/1061
File chrome/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/160005/diff/88/1061#newcode27
Line 27: // Assuming a packet
??

http://codereview.chromium.org/160005/diff/88/1061#newcode32
Line 32: : AudioRendererBase(kDefaultMaxQueueSize * 4),
Magic number alert!  Why is this *4?

http://codereview.chromium.org/160005/diff/88/1062
File media/base/filters.h (right):

http://codereview.chromium.org/160005/diff/88/1062#newcode94
Line 94: scoped_ptr<FilterCallback> play_callback(callback);
So, again, don't care since I'd want to see a known perf impact.  However, I
remember there being some discussion a while back that rampant use of scoped_ptr
could end up causing some sort cache stupidity.

But that's micro-optimization.  Just something to keep in mind.

http://codereview.chromium.org/160005/diff/88/1064
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/88/1064#newcode444
Line 444: filter_factory_ = NULL;
More detail?  I don't understand why we don't need it anymore.

http://codereview.chromium.org/160005/diff/88/1064#newcode575
Line 575: if (++filter_replies_ == filters_.size()) {
*sigh* This make me wish he had barrier closures. :-/

http://codereview.chromium.org/160005/diff/88/1064#newcode580
Line 580: break;
put newlines after "break;" please.  Here and below.

Also, stay consistent with either hte use of "break;" or "return;" from teh
switch.  I personally prefer using "break;"

http://codereview.chromium.org/160005/diff/88/1064#newcode626
Line 626: return;
stay consistent with break versus return.

http://codereview.chromium.org/160005/diff/88/1065
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160005/diff/88/1065#newcode101
Line 101: void OnFilterReply();
This is a funky name...

Shoudl it be something like OnFilterActionComplete?  OnFilterStateTransition? On
FilterStateChangeComplete?  Not sure...FilterReply is very vague.

http://codereview.chromium.org/160005/diff/88/1065#newcode273
Line 273: // the size of |filters_|, we can transition out of the state.
hmm...this reads funny to me.  It feels more natural to count towards 0 and make
it an outstanding_filter_tasks_ or something.

http://codereview.chromium.org/160005/diff/88/1070
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/160005/diff/88/1070#newcode160
Line 160: DCHECK(!demuxer_stream_);
The DCHECKS for these 2 seem a little dangerous.  Will we leak if
demuxer_stream_ is set?  And will we violate some other invariant if state_ !=
kUninitialized?

http://codereview.chromium.org/160005/diff/88/1075
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/160005/diff/88/1075#newcode63
Line 63: DCHECK_EQ(kPaused, state_);
I don't understand the consequences of failing the DCHECKs enough.  Mind going
through each DCHECK and making sure that if they are removed, we will still have
good behavior in production?  If we can't guarantee it, move to using a real
CHECK instead.

http://codereview.chromium.org/160005/diff/88/1075#newcode178
Line 178: void VideoRendererBase::ThreadMain() {
This function is getting a bit long.  Would it be hard to factor out some chunks
into sub functions?

http://codereview.chromium.org/160005/diff/88/1075#newcode193
Line 193: // Sleep for 10 milliseconds while paused.
Explain why we should sleep and why 10ms makes sense for all platforms?

http://codereview.chromium.org/160005/diff/88/1075#newcode200
Line 200: // this loop should executes our "playing" logic.
exectues -> exectue

[scherkus (not reviewing), 2009-07-24 22:24:31]

thanks for the review fellas -- I know it's not enjoyable :\

http://codereview.chromium.org/160005/diff/84/1044
File media/base/filters.h (right):

http://codereview.chromium.org/160005/diff/84/1044#newcode101
Line 101: // downstream filters, except when they are pre-rolling during a
Seek().
On 2009/07/24 18:47:58, Alpha wrote:
> I'm not so sure about the pre-rolling statement. So filters can still read
when
> they are paused?

whoops it's outdated

http://codereview.chromium.org/160005/diff/84/1046
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/84/1046#newcode356
Line 356: start_callback_.reset(start_callback);
On 2009/07/24 18:47:58, Alpha wrote:
> Start is now essentially the same as seek to 0s, so why not just save this to
> seek_callback_.

Done.

http://codereview.chromium.org/160005/diff/84/1046#newcode452
Line 452: state_ = kSeeking;
On 2009/07/24 18:47:58, Alpha wrote:
> Maybe adding some documentation to explain Start is equal to Init -> Seeking
->
> Starting -> Started.

done in header file

http://codereview.chromium.org/160005/diff/84/1046#newcode546
Line 546: // will only execute the first Seek() request.
On 2009/07/24 18:47:58, Alpha wrote:
> We should put a DCHECK here, if we have an outstanding seek we should not
> receive another one. I sounds something is wrong if we receive multiple seek
> calls.

yeah I'm not sure what to do... technically it's the client misbehaving

http://codereview.chromium.org/160005/diff/84/1046#newcode590
Line 590: if (start_callback_.get()) {
On 2009/07/24 18:47:58, Alpha wrote:
> If you keep the start_callback in seek_callback, you don't need this :)
> 

Done.

http://codereview.chromium.org/160005/diff/84/1047
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160005/diff/84/1047#newcode36
Line 36: //        .-> [ Error ]        .->      [ Stopped ]    <-.
On 2009/07/24 18:47:58, Alpha wrote:
> About the state transition of this object, this description is not accurate
> anymore due to new states, what about updating it?

Done.

http://codereview.chromium.org/160005/diff/84/1057
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/160005/diff/84/1057#newcode153
Line 153: CreateBlackFrame(&current_frame_);
On 2009/07/24 18:47:58, Alpha wrote:
> hehe, this is cool, we could any picture while paused :P
> 

I want to have a CreateSadFrame(...) that paints a sad frame when we crash :)

http://codereview.chromium.org/160005/diff/84/1057#newcode206
Line 206: if (state_ != kPlaying) {
On 2009/07/24 18:47:58, Alpha wrote:
> This if statement looks a little strange to me because you have a DCHECK
above..

I'm very paranoid about this loop so for production if we continue, we should
exit the loop the next time

http://codereview.chromium.org/160005/diff/88/1061
File chrome/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/160005/diff/88/1061#newcode27
Line 27: // Assuming a packet
On 2009/07/24 19:23:54, awong wrote:
> ??

gaaaaaaaaahhhhh debugging code got uploaded

http://codereview.chromium.org/160005/diff/88/1061#newcode32
Line 32: : AudioRendererBase(kDefaultMaxQueueSize * 4),
On 2009/07/24 19:23:54, awong wrote:
> Magic number alert!  Why is this *4?

Done.

http://codereview.chromium.org/160005/diff/88/1062
File media/base/filters.h (right):

http://codereview.chromium.org/160005/diff/88/1062#newcode94
Line 94: scoped_ptr<FilterCallback> play_callback(callback);
On 2009/07/24 19:23:54, awong wrote:
> So, again, don't care since I'd want to see a known perf impact.  However, I
> remember there being some discussion a while back that rampant use of
scoped_ptr
> could end up causing some sort cache stupidity.
> 
> But that's micro-optimization.  Just something to keep in mind.

I'll fix it here because it actually does look a little hairy

http://codereview.chromium.org/160005/diff/88/1064
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/88/1064#newcode444
Line 444: filter_factory_ = NULL;
On 2009/07/24 19:23:54, awong wrote:
> More detail?  I don't understand why we don't need it anymore.

Done.

http://codereview.chromium.org/160005/diff/88/1064#newcode575
Line 575: if (++filter_replies_ == filters_.size()) {
On 2009/07/24 19:23:54, awong wrote:
> *sigh* This make me wish he had barrier closures. :-/

:(

http://codereview.chromium.org/160005/diff/88/1064#newcode580
Line 580: break;
On 2009/07/24 19:23:54, awong wrote:
> put newlines after "break;" please.  Here and below.
> 
> Also, stay consistent with either hte use of "break;" or "return;" from teh
> switch.  I personally prefer using "break;"

Done.

http://codereview.chromium.org/160005/diff/88/1064#newcode626
Line 626: return;
On 2009/07/24 19:23:54, awong wrote:
> stay consistent with break versus return.

Done.

http://codereview.chromium.org/160005/diff/88/1065
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/160005/diff/88/1065#newcode101
Line 101: void OnFilterReply();
On 2009/07/24 19:23:54, awong wrote:
> This is a funky name...
> 
> Shoudl it be something like OnFilterActionComplete?  OnFilterStateTransition?
On
> FilterStateChangeComplete?  Not sure...FilterReply is very vague.

Done.

http://codereview.chromium.org/160005/diff/88/1065#newcode273
Line 273: // the size of |filters_|, we can transition out of the state.
On 2009/07/24 19:23:54, awong wrote:
> hmm...this reads funny to me.  It feels more natural to count towards 0 and
make
> it an outstanding_filter_tasks_ or something.

since I renamed the functions to FilterStateTransition, how about this counts
the number of transitions

http://codereview.chromium.org/160005/diff/88/1070
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/160005/diff/88/1070#newcode160
Line 160: DCHECK(!demuxer_stream_);
On 2009/07/24 19:23:54, awong wrote:
> The DCHECKS for these 2 seem a little dangerous.  Will we leak if
> demuxer_stream_ is set?  And will we violate some other invariant if state_ !=
> kUninitialized?

hmm well the invariant broken was by the callee who is double initializing us

everything will break, more or less

sorry not sure what you mean here :\

http://codereview.chromium.org/160005/diff/88/1075
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/160005/diff/88/1075#newcode178
Line 178: void VideoRendererBase::ThreadMain() {
On 2009/07/24 19:23:54, awong wrote:
> This function is getting a bit long.  Would it be hard to factor out some
chunks
> into sub functions?

It's hard to break this up, but I extracted the sleep-duration-calculation

http://codereview.chromium.org/160005/diff/88/1075#newcode193
Line 193: // Sleep for 10 milliseconds while paused.
On 2009/07/24 19:23:54, awong wrote:
> Explain why we should sleep and why 10ms makes sense for all platforms?

Done.

http://codereview.chromium.org/160005/diff/88/1075#newcode200
Line 200: // this loop should executes our "playing" logic.
On 2009/07/24 19:23:54, awong wrote:
> exectues -> exectue

Done.

[awong, 2009-07-24 22:35:17]

Couple more small comments.

http://codereview.chromium.org/160005/diff/88/1070
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/160005/diff/88/1070#newcode160
Line 160: DCHECK(!demuxer_stream_);
On 2009/07/24 22:24:31, scherkus wrote:
> On 2009/07/24 19:23:54, awong wrote:
> > The DCHECKS for these 2 seem a little dangerous.  Will we leak if
> > demuxer_stream_ is set?  And will we violate some other invariant if state_
!=
> > kUninitialized?
> 
> hmm well the invariant broken was by the callee who is double initializing us
> 
> everything will break, more or less
> 
> sorry not sure what you mean here :\

Basically, I'm wondering if this should be a CHECK isntead of a DCHECK.

http://codereview.chromium.org/160005/diff/97/1086
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/97/1086#newcode596
Line 596: // Invalid states will be caught in the switch statement below.
I like the new version of the code.  However, the logic did change a bit
unforutnately.  If we enter into this fucntion with kStarted, we'll actually
have the wrong behavior I think.

How about doing a

if (!ValidNonFinalTranstitionState(state_)) {  
  SetError(PIPELINE_ERROR_ABORT);
  return;
}

if (--remaining_transitions_ == 0) {
  state_ = FindNextState(state_);
}

http://codereview.chromium.org/160005/diff/97/1086#newcode604
Line 604: filters_[filters_.size() - remaining_transitions_]->Pause(
Pull this index expression out into a local variable outside of the switch with
a sensible name please.

[scherkus (not reviewing), 2009-07-25 00:18:55]

http://codereview.chromium.org/160005/diff/97/1086
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/97/1086#newcode596
Line 596: // Invalid states will be caught in the switch statement below.
On 2009/07/24 22:35:17, awong wrote:
> I like the new version of the code.  However, the logic did change a bit
> unforutnately.  If we enter into this fucntion with kStarted, we'll actually
> have the wrong behavior I think.
> 
> How about doing a
> 
> if (!ValidNonFinalTranstitionState(state_)) {  
>   SetError(PIPELINE_ERROR_ABORT);
>   return;
> }
> 
> if (--remaining_transitions_ == 0) {
>   state_ = FindNextState(state_);
> }
> 
> 

Done.

http://codereview.chromium.org/160005/diff/97/1086#newcode604
Line 604: filters_[filters_.size() - remaining_transitions_]->Pause(
On 2009/07/24 22:35:17, awong wrote:
> Pull this index expression out into a local variable outside of the switch
with
> a sensible name please.
> 

haha I had that before, but the fall through logic would cause an exception when
remaining_transitions_ == 0 :D

[awong, 2009-07-25 00:33:05]

LGTM

Looks good enough.  2 more nit picks.

http://codereview.chromium.org/160005/diff/115/118
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/160005/diff/115/118#newcode590
Line 590: DCHECK(StateTransitionsToStarted(state_));
The NOTREACHED() below is the equiv of the DCHECK.  Remove this DCHECK.

http://codereview.chromium.org/160005/diff/115/118#newcode614
Line 614: switch (state_) {
It seems like you have two cases. When StateTranstionsToSTarted() is true, and
when not.  Maybe  make it more explicit like this:

if (StateTransitionsToStarted(state_)) {
  MediaFilter* filter = filters_[.....];
  if (state_ == kPausing) {
  } else if (state_ == kStarting) {
  } else {
    NOTREACHED() << "Hi mom";
  }
} else if (state_ == kStarted) {
} else {
  NOTREACHED() << "hi dad";
}


It avoids having filter dangle through as NULL.

[Alpha Left Google, 2009-07-25 00:57:02]

LGTM!

[scherkus (not reviewing), 2009-07-25 01:01:14]

ooooooooookay

I think we're good here

[awong, 2009-07-25 01:02:33]

LFGTM!