Allow transitioning to HAVE_METADATA before pipeline initialization completes.

Allow transitioning to HAVE_METADATA before pipeline initialization completes.


BUG=141429
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151988

[acolwell GONE FROM CHROMIUM, 2012-08-14 22:47:57]

Here is a patch outlining one way to signal HAVE_METADATA before Pipeline
initialization has completed. These changes allow MediaSource applications to
append data for the middle of the clip and seek to that position w/o having to
append media data for the start of the clip. Here are some things to keep in
mind.

- currentTime cannot be set until HTMLMediaElement.readyState has reached
HAVE_METADATA
- The current code doesn't allow us to reach HAVE_METADATA until prerolling the
start of the clip has completed.
- The application doesn't want to append data at the start of the clip because
that isn't where it wants playback to start.
- I've use the existing "pending seek" logic to keep track of the desired seek
position while the pipeline finishes initializing.
- The proxy_->DemuxerCancelPendingSeek() in WebMediaPlayerImpl::seek() is what
cancels the "seek to start" that happens during pipeline initialization.

This is the solution with the least code smell I've come up with so far. I'd
definitely like some feedback on how to proceed.

[Ami GONE FROM CHROMIUM, 2012-08-15 18:00:08]

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc#newcode555
media/base/pipeline.cc:555: const PipelineStatusCB& start_cb,
AFAICT start_cb.Run() is always called with OK (else error_cb.Run() is called),
and start_cb is called exactly when ready_state_cb reports HaveEnoughData.  Is
it time to drop start_cb entirely?

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h#newcode103
media/base/pipeline.h:103: HaveMetadata,
kChromiumStyle

http://codereview.chromium.org/10854151/diff/1/media/filters/pipeline_integra...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10854151/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:192: Pipeline::ReadyStateCB());
Any reason not to make these actually test that the expected transitions happen?

http://codereview.chromium.org/10854151/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10854151/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:764: starting_ = false;
put below DCHECK below.

[scherkus (not reviewing), 2012-08-15 19:53:43]

the idea I had in my mind (see TODO at pipeline.h:388) was to have
initialization cover us from HAVE_NOTHING to HAVE_METADATA then have separate
operations for seek/preroll but I understand that's a larger scale change and
possibly something we tackle after I land my big ol' refactor

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc#newcode285
media/base/pipeline.cc:285: ready_state_cb_.Run(HaveEnoughData);
fix indent

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc#newcode555
media/base/pipeline.cc:555: const PipelineStatusCB& start_cb,
On 2012/08/15 18:00:08, Ami Fischman wrote:
> AFAICT start_cb.Run() is always called with OK (else error_cb.Run() is
called),
> and start_cb is called exactly when ready_state_cb reports HaveEnoughData.  Is
> it time to drop start_cb entirely?

start_cb is set as seek_cb, meaning it will get called w/ the error code and
error_cb is not called

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h#newcode126
media/base/pipeline.h:126: const ReadyStateCB& ready_state_cb);
docs?

[acolwell GONE FROM CHROMIUM, 2012-08-15 20:01:13]

On 2012/08/15 19:53:43, scherkus wrote:
> the idea I had in my mind (see TODO at pipeline.h:388) was to have
> initialization cover us from HAVE_NOTHING to HAVE_METADATA then have separate
> operations for seek/preroll but I understand that's a larger scale change and
> possibly something we tackle after I land my big ol' refactor
I tried doing this, but it just moves the multi-stage seek process out into WMPI
which felt wrong. It was especially noticable when updating all the unit tests
to do the Init -> Preroll & Init -> Preroll -> Seek -> Preroll transitions.

[scherkus (not reviewing), 2012-08-15 20:50:01]

On 2012/08/15 20:01:13, acolwell wrote:
> On 2012/08/15 19:53:43, scherkus wrote:
> > the idea I had in my mind (see TODO at pipeline.h:388) was to have
> > initialization cover us from HAVE_NOTHING to HAVE_METADATA then have
separate
> > operations for seek/preroll but I understand that's a larger scale change
and
> > possibly something we tackle after I land my big ol' refactor
> I tried doing this, but it just moves the multi-stage seek process out into
WMPI
> which felt wrong. It was especially noticable when updating all the unit tests
> to do the Init -> Preroll & Init -> Preroll -> Seek -> Preroll transitions.

Good point! Hmm... fischman@ touched upon this too -- but I wonder if we'll end
up removing start_cb at some point. If we reach the metadata state it seems like
the pipeline should be ready to accept Seek() calls and cancel whatever is in
flight and hide that complexity from WMPI

as far as this CL goes I think it's a good middle of the ground approach --
everything else seems quite invasive at this time

[acolwell GONE FROM CHROMIUM, 2012-08-15 23:21:15]

PTAL

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc#newcode285
media/base/pipeline.cc:285: ready_state_cb_.Run(HaveEnoughData);
On 2012/08/15 19:53:43, scherkus wrote:
> fix indent

Done.

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.cc#newcode555
media/base/pipeline.cc:555: const PipelineStatusCB& start_cb,
On 2012/08/15 19:53:43, scherkus wrote:
> On 2012/08/15 18:00:08, Ami Fischman wrote:
> > AFAICT start_cb.Run() is always called with OK (else error_cb.Run() is
> called),
> > and start_cb is called exactly when ready_state_cb reports HaveEnoughData. 
Is
> > it time to drop start_cb entirely?
> 
> start_cb is set as seek_cb, meaning it will get called w/ the error code and
> error_cb is not called

So I've kept the callback for now, but I've renamed it to seek_cb since it
primarily reflects the success of the initial seek. I've also reorganized the
logic in WebMediaPlayerImpl so that we don't need initialization specific logic.
We just use the existing seek logic. Let me know what you think.

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h#newcode103
media/base/pipeline.h:103: HaveMetadata,
On 2012/08/15 18:00:08, Ami Fischman wrote:
> kChromiumStyle

Done.

http://codereview.chromium.org/10854151/diff/1/media/base/pipeline.h#newcode126
media/base/pipeline.h:126: const ReadyStateCB& ready_state_cb);
On 2012/08/15 19:53:43, scherkus wrote:
> docs?

Done.

http://codereview.chromium.org/10854151/diff/1/media/filters/pipeline_integra...
File media/filters/pipeline_integration_test.cc (right):

http://codereview.chromium.org/10854151/diff/1/media/filters/pipeline_integra...
media/filters/pipeline_integration_test.cc:192: Pipeline::ReadyStateCB());
On 2012/08/15 18:00:08, Ami Fischman wrote:
> Any reason not to make these actually test that the expected transitions
happen?
Nope. Done

http://codereview.chromium.org/10854151/diff/1/webkit/media/webmediaplayer_im...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10854151/diff/1/webkit/media/webmediaplayer_im...
webkit/media/webmediaplayer_impl.cc:764: starting_ = false;
On 2012/08/15 18:00:08, Ami Fischman wrote:
> put below DCHECK below.

Done.

[scherkus (not reviewing), 2012-08-15 23:43:16]

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:806:
SetNetworkState(WebMediaPlayer::NetworkStateFormatError);
does this impact any layout tests?

do we have layout tests for stuff like this?

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:859: media::Pipeline::ReadyState readyState)
{
unix_hacker

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:992: state ==
WebMediaPlayer::ReadyStateHaveMetadata) {
this assumes we always set to metadata at some point in time

typical practice in HTMLMediaElement land is to use >= -- thoughts?

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:998:
SetNetworkState(WebMediaPlayer::NetworkStateLoaded);
{} for if statements >1 line

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.h:232: void
OnPipelineReadyState(media::Pipeline::ReadyState readyState);
unix_hacker

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_proxy.cc (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.cc:109: media::Pipeline::ReadyState
readyState) {
unix_hacker

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.cc:145: media::Pipeline::ReadyState
readyState) {
unix_hacker

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:81: void
PipelineReadyStateCallback(media::Pipeline::ReadyState readyState);
unix_hacker

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:137: // Notify |webmediaplayer_| of
readyState changes.
unix_hacker

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:138: void
PipelineReadyStateTask(media::Pipeline::ReadyState readyState);
unix_hacker

[scherkus (not reviewing), 2012-08-15 23:46:55]

http://codereview.chromium.org/10854151/diff/8005/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/8005/media/base/pipeline.h#newco...
media/base/pipeline.h:100: // Current buffering state in terms of
HTMLMediaElement.readyState values.
layering violation in comment form!

instead of a catch-all comment we should define these states in terms of
pipeline operations (i.e., kHaveMetadata -> natural size / duration / etc... are
queryable)

http://codereview.chromium.org/10854151/diff/8005/media/base/pipeline.h#newco...
media/base/pipeline.h:121: //                    that should trigger a
HTMLMediaElement.readyState
ditto -- this comment shouldn't reference what the client should do (it's up to
the client!)

what about mentioning that this cb is optional?

[acolwell GONE FROM CHROMIUM, 2012-08-16 00:54:18]

http://codereview.chromium.org/10854151/diff/8005/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/8005/media/base/pipeline.h#newco...
media/base/pipeline.h:100: // Current buffering state in terms of
HTMLMediaElement.readyState values.
On 2012/08/15 23:46:55, scherkus wrote:
> layering violation in comment form!
> 
> instead of a catch-all comment we should define these states in terms of
> pipeline operations (i.e., kHaveMetadata -> natural size / duration / etc...
are
> queryable)

Done.

http://codereview.chromium.org/10854151/diff/8005/media/base/pipeline.h#newco...
media/base/pipeline.h:121: //                    that should trigger a
HTMLMediaElement.readyState
On 2012/08/15 23:46:55, scherkus wrote:
> ditto -- this comment shouldn't reference what the client should do (it's up
to
> the client!)
> 
> what about mentioning that this cb is optional?

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:806:
SetNetworkState(WebMediaPlayer::NetworkStateFormatError);
On 2012/08/15 23:43:16, scherkus wrote:
> does this impact any layout tests?
This does not break any existing tests if that is what you mean. This just moves
the logic from OnPipelineInitialize(). We are always in ReadyStateHaveNothing
when that method is called.

> 
> do we have layout tests for stuff like this?
Yes. there are several tests that check for MEDIA_ERROR_SRC_NOT_SUPPORTED which
is what this error eventually turns into.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:859: media::Pipeline::ReadyState readyState)
{
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:992: state ==
WebMediaPlayer::ReadyStateHaveMetadata) {
On 2012/08/15 23:43:16, scherkus wrote:
> this assumes we always set to metadata at some point in time
> 
> typical practice in HTMLMediaElement land is to use >= -- thoughts?
Good point. Changed to >=

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.cc:998:
SetNetworkState(WebMediaPlayer::NetworkStateLoaded);
On 2012/08/15 23:43:16, scherkus wrote:
> {} for if statements >1 line

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_impl.h (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_impl.h:232: void
OnPipelineReadyState(media::Pipeline::ReadyState readyState);
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_proxy.cc (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.cc:109: media::Pipeline::ReadyState
readyState) {
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.cc:145: media::Pipeline::ReadyState
readyState) {
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
File webkit/media/webmediaplayer_proxy.h (right):

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:81: void
PipelineReadyStateCallback(media::Pipeline::ReadyState readyState);
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:137: // Notify |webmediaplayer_| of
readyState changes.
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

http://codereview.chromium.org/10854151/diff/8005/webkit/media/webmediaplayer...
webkit/media/webmediaplayer_proxy.h:138: void
PipelineReadyStateTask(media::Pipeline::ReadyState readyState);
On 2012/08/15 23:43:16, scherkus wrote:
> unix_hacker

Done.

[scherkus (not reviewing), 2012-08-16 01:36:02]

lgtm w/ ocdnit

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h#newc...
media/base/pipeline.h:101: // kHaveMetadata : Indicates that the following
things are known:
OCD nits: no space before :, and don't worry about maintaining an indent on
subsequent lines (longer enum names get penalized on all that delicious
available whitespace!)

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

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc#new...
media/base/pipeline.cc:662: if (!ready_state_cb_.is_null())
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.

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc#new...
media/base/pipeline.cc:927: ready_state_cb_.Run(kHavePrerolled);
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.

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h#newc...
media/base/pipeline.h:109: kHavePrerolled,
Unfortunate that both of these use "Have" but in different senses.

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

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc#new...
media/base/pipeline.cc:662: if (!ready_state_cb_.is_null())
On 2012/08/16 03:31:56, Ami Fischman wrote:
> 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.

Done.

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc#new...
media/base/pipeline.cc:927: ready_state_cb_.Run(kHavePrerolled);
On 2012/08/16 03:31:56, Ami Fischman wrote:
> Does it make sense to reset this CB at this point?
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.

> 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.  
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.

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h#newc...
media/base/pipeline.h:101: // kHaveMetadata : Indicates that the following
things are known:
On 2012/08/16 01:36:02, scherkus wrote:
> OCD nits: no space before :, and don't worry about maintaining an indent on
> subsequent lines (longer enum names get penalized on all that delicious
> available whitespace!)
Fixed space before:
Changed to a 2 space indent for the state descriptions since I think it makes it
easier to quickly find the description for a particular state.

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.h#newc...
media/base/pipeline.h:109: kHavePrerolled,
On 2012/08/16 03:31:56, Ami Fischman wrote:
> Unfortunate that both of these use "Have" but in different senses.
Agreed.. Changed it to kPrerollCompleted

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

LGTM % readystate business.

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc#new...
media/base/pipeline.cc:927: ready_state_cb_.Run(kHavePrerolled);
On 2012/08/16 16:36:03, acolwell wrote:
> Hmm.. Perhaps I should move away from ReadyState in favor some something like
> Progress or BufferingState?

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]

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10854151/diff/14002/media/base/pipeline.cc#new...
media/base/pipeline.cc:927: ready_state_cb_.Run(kHavePrerolled);
On 2012/08/16 16:43:47, Ami Fischman wrote:
> On 2012/08/16 16:36:03, acolwell wrote:
> > Hmm.. Perhaps I should move away from ReadyState in favor some something
like
> > Progress or BufferingState?
> 
> 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!).

Done.

[commit-bot: I haz the power, 2012-08-16 17:55:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/10854151/11003

[commit-bot: I haz the power, 2012-08-17 00:02:43]

Change committed as 151988