Change MediaFilter::Stop() to accept a callback so that Stop() is asynchronous.

Change MediaFilter::Stop() to accept a callback so that Stop() is asynchronous. So far PipelineImpl handles asynchronous MediaFilter::Stop(). The actual change to make MediaFilter::Stop() asynchronous will be in another check in.

BUG=16059
TEST=Unit tests still runs

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

[boliu_use_chromium_pls, 2010-05-21 20:51:13]

First patch only contains preliminary changes for the pipeline for making
MediaFilter::Stop() asynchronous. Need review...

[boliu_use_chromium_pls, 2010-05-21 20:57:22]

Forgot to add Reviewers.

[scherkus (not reviewing), 2010-05-22 01:44:08]

on the right track but I think this ultimately needs a new "kStopping" state

http://codereview.chromium.org/2101015/diff/1/3
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/2101015/diff/1/3#newcode831
media/base/pipeline_impl.cc:831: if (--remaining_transitions_ > 0u) {
we should probably introduce an additional state (kStopping) and re-use
FilterStateTransitionTask

when (kStopping && remaining_transitions_ == 0) is reached we can then move to
kStopped (if error_ == PIPELINE_STOPPING) or kError (otherwise)

there's a nice NOTREACHED() in FilterStateTransitionTask() where we our
kStopping code will probably go

don't worry about making it look pretty at this stage, let's just try to
introduce kStopping, reuse FilterStateTransitionTask, get it compiling +
working, then we can iterate on the style/prettiness :)

http://codereview.chromium.org/2101015/diff/1/3#newcode991
media/base/pipeline_impl.cc:991: remaining_transitions_ = filters_.size();
we should probably transition to a new state "kStopping" here

[boliu_use_chromium_pls, 2010-05-24 23:57:05]

I reverted my last change, so don't look at diff between changesets.

[scherkus (not reviewing), 2010-05-25 03:43:17]

getting there!

http://codereview.chromium.org/2101015/diff/1004/6002
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/2101015/diff/1004/6002#newcode776
media/base/pipeline_impl.cc:776: 
I think we need to handle our kSTopping -> kStopped logic here

http://codereview.chromium.org/2101015/diff/1004/6002#newcode784
media/base/pipeline_impl.cc:784: if (StateTransitionsToStarted(state_) ||
kStopping == state_) {
I like it

lets update StateTransitionsToStarted to something more sensible

I think we have an invariant where (is_a_transient_state &&
remaining_transitions_ > 0) which we can enforce with a DCHECK inside that
function

http://codereview.chromium.org/2101015/diff/1004/6002#newcode838
media/base/pipeline_impl.cc:838: state_ = kStopped;
but we're already in kStopped :)

http://codereview.chromium.org/2101015/diff/1004/6002#newcode969
media/base/pipeline_impl.cc:969: if (remaining_transitions_ > 0) {
what if remaining_Transitions_ == 0?

[boliu_use_chromium_pls, 2010-05-26 00:53:08]

All media_unittests pass now.

Introduced new method FinishDestroyingFilters which performs the clean up after
all filters have stopped.

[scherkus (not reviewing), 2010-05-26 06:04:10]

getting there!!!!!

http://codereview.chromium.org/2101015/diff/9001/10001
File media/base/filters.h (right):

http://codereview.chromium.org/2101015/diff/9001/10001#newcode111
media/base/filters.h:111: // TODO(boliu): Remove once Stop() is asynchronous in
sublcasses
nit: sublcasses -> subclasses
nit: we end comments with periods

http://codereview.chromium.org/2101015/diff/9001/10001#newcode116
media/base/filters.h:116: // TODO(boliu): No implementation in subclasses yet
ditto

http://codereview.chromium.org/2101015/diff/9001/10001#newcode119
media/base/filters.h:119: //              all filters have asynchronous stop.
nit for future TODO/comment formatting: no need to keep the alignment here

http://codereview.chromium.org/2101015/diff/9001/10004
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/2101015/diff/9001/10004#newcode648
media/base/pipeline_impl.cc:648: if ((state_ == kError && error !=
PIPELINE_STOPPING) || state_ == kStopped) {
would it help if we removed PIPELINE_STOPPING?

error reporting is only used via error_callback_, which doesn't execute if
PIPELINE_STOPPING has been set

it seems with kStopping we can get rid of this and leave error_ as PIPELINE_OK
which intuitively seems to make sense (we're doing a graceful shutdown as
opposed to an erroneous shutdown)

http://codereview.chromium.org/2101015/diff/9001/10004#newcode660
media/base/pipeline_impl.cc:660: error_ = error; // Need to set error_ if called
from StopTask().
unfortunately we can't access error_ unless we hold onto the lock -- getting rid
of PIPELINE_STOPPING might should get rid of this

http://codereview.chromium.org/2101015/diff/9001/10004#newcode788
media/base/pipeline_impl.cc:788: //             
SetError()->DestroyFilters()->(something happens)
no need to indent

http://codereview.chromium.org/2101015/diff/9001/10004#newcode866
media/base/pipeline_impl.cc:866: if (error_ != PIPELINE_STOPPING) {
same issue here

we might have to grab the lock to check the value of error_ here :\

[boliu_use_chromium_pls, 2010-05-26 18:25:51]

Remove PIPELINE_STOPPING completely and add a kErrorStopping state. 

New kErrorStopping state is used to distringuish between regular stop
(kStopping) from stopping due to an error (kErrorStopping).

StopTask no longer calls ErrorChangedTask but calls DestroyFilters directly.

Add a number of helper methods in PipelineImpl.

Handling mixed calls of Stop() and SetError() is getting hairy...

[boliu_use_chromium_pls, 2010-05-26 21:18:57]

Remove kErrorStopping state and use error_ to distinguish normal/error stops.

If currently in Stop(), then Stop()/SetError() will do nothing.

If currently in SetError(), then SetError() will do nothing, and Stop() will
ignore the current SetError() and perform a normal stop.

Should I add unit test for any of these conditions?

[scherkus (not reviewing), 2010-05-27 17:31:21]

I think we're just about done here

don't about unit tests for those corner cases yet as thigns still might shift a
bit

http://codereview.chromium.org/2101015/diff/19001/20006
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/2101015/diff/19001/20006#newcode1009
media/base/pipeline_impl.cc:1009: void PipelineImpl::DestroyFilters() {
since this function starts the asynchronous transition how about we rename this
to StartDestroyingFilters

http://codereview.chromium.org/2101015/diff/19001/20006#newcode1011
media/base/pipeline_impl.cc:1011: remaining_transitions_ = filters_.size();
add DCHECK that we're not already  kStopping/kStopped/kError

http://codereview.chromium.org/2101015/diff/19001/20007
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/2101015/diff/19001/20007#newcode140
media/base/pipeline_impl.h:140: // TODO(boliu): Bad name?
remove TODO -- it's an ok name

http://codereview.chromium.org/2101015/diff/19001/20007#newcode143
media/base/pipeline_impl.h:143: // Helper method to execute callback and reset
filter_factory_.
nit: surround variables in comments with | |

http://codereview.chromium.org/2101015/diff/19001/20007#newcode145
media/base/pipeline_impl.h:145: // stopped or error occurs during
initialization.
which callback are we executign?

http://codereview.chromium.org/2101015/diff/19001/20007#newcode228
media/base/pipeline_impl.h:228: // Carries out stopping filter threads,
deleteing filters, running
deleteing -> deleting

[boliu_use_chromium_pls, 2010-05-28 22:51:11]

Filters under src/media/filters have Stop() with a callback now. Unit tests for
these filters are not making use of the callback yet and thus have lots of
TODO's. Is there a filter where it should stop asynchronously that currently is
not?

WebMediaPlayerImpl::Destroy() waits for Pipeline::Stop to execute the callback
before continuing. The Lock and ConditionVariable have to be created as instance
variables since NewCallback() cannot bind parameters at creation time.

http://codereview.chromium.org/2101015/diff/19001/20006
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/2101015/diff/19001/20006#newcode1011
media/base/pipeline_impl.cc:1011: remaining_transitions_ = filters_.size();
Will no-op if kStopping; logic in StopTask() / ErrorChangedTask() would be messy
if we do not call this for kStopping.

kError is actually allowed. Stop() can becalled after SetError() completes. In
this case all filters are already stopped, so skipping straight to
FinishDestroyingFiltersTask*() to reset pipeline state.

On 2010/05/27 17:31:21, scherkus wrote:
> add DCHECK that we're not already  kStopping/kStopped/kError

[boliu, 2010-06-02 00:07:00]

Hey Andrew,

Could you review the last bit of change to PipelineImpl?

I ran tests using the try bots and got confusing results. On the code review
pages they came out as green, but from the result email, it says linux and win
bots failed. Do I need to worry about the failures?

After I get a LGTM from you, I'll follow the instructions for checking in code
with git.

Bo

[scherkus (not reviewing), 2010-06-02 02:13:05]

LGTM but we need to resolve some things before committing

http://codereview.chromium.org/2101015/diff/29001/30006
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/2101015/diff/29001/30006#newcode638
media/base/pipeline_impl.cc:638: if (state_ == kStopped || (state_ == kStopping
&& error == PIPELINE_OK)) {
this bit of logic is confusing me...

so if we're stopping and then SetError() is called we'll save the error value
but continue stopping?

if we're in error state and stoptask is called we'll overwrite the error_ with
PIPELINE_OK?

also the comment seems to be out of date.. I believe StopTask can only be
executed as a result of calling Stop() -- not an error condition

we can talk about it in person

http://codereview.chromium.org/2101015/diff/29001/30006#newcode642
media/base/pipeline_impl.cc:642: } else if (state_ == kError ||
nit: we return, don't need else

http://codereview.chromium.org/2101015/diff/29001/30006#newcode1014
media/base/pipeline_impl.cc:1014: return; // Do not call Stop() on filters
twice.
nit: two spaces between ; and //