Fix flaky behavior in pipeline teardown.

Fix flaky behavior in pipeline teardown.


BUG=61012
TEST=Covered by existing media tests.

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

[scherkus (not reviewing), 2011-01-13 00:47:38]

LGTM w/ questions

what do you think about merging error + stop

I've been thinking it's rather silly to have two different "stop" callbacks and
different terminal states (kError and kStopped) when we could have one
(kStopped) and a "reason" for stopping code

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

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:329: bool PipelineImpl::IsPipelineInitializing() {
I wonder if we still need this method given the only other place is in a
DCHECK...

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:684: if (IsPipelineTearingDown() &&
error_caused_teardown_) {
to confirm: this happens in the case someone calls Stop() while we've errored?

in which case we want to override the error with normal stop behaviour?

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:925: if (error_caused_teardown_  && GetError() !=
PIPELINE_OK &&
nit: get rid of extra space between error_caused_teardown_ and &&

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1157: pipeline_filter_->Stop(NewCallback(
drop NewCallback to next line

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1164: pipeline_filter_->Pause(NewCallback(
drop NewCallback to next line

[acolwell GONE FROM CHROMIUM, 2011-01-13 01:39:16]

Applied changes suggested.


On 2011/01/13 00:47:38, scherkus wrote:
> what do you think about merging error + stop

I'm not sure that's a good idea. kError represents the
pipeline being stopped, but the caller hasn't called Stop()
yet. kStopped indicates when the caller initiated stop has
completed. I like having these 2 different states. There is also a slight
difference in the pipeline state. When OnError() is called you can still call
the GetXXX() methods and get sane values. After "stop done" is called all these
methods return pre-Start() defaults.

> 
> I've been thinking it's rather silly to have two different "stop" callbacks
and
> different terminal states (kError and kStopped) when we could have one
> (kStopped) and a "reason" for stopping code

I'd be open to exploring such a change ... in a new CL. :)

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

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:329: bool PipelineImpl::IsPipelineInitializing() {
Moved method body to DCHECK()and removed method.

On 2011/01/13 00:47:38, scherkus wrote:
> I wonder if we still need this method given the only other place is in a
> DCHECK...

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:684: if (IsPipelineTearingDown() &&
error_caused_teardown_) {
On 2011/01/13 00:47:38, scherkus wrote:
> to confirm: this happens in the case someone calls Stop() while we've errored?
> 
> in which case we want to override the error with normal stop behaviour?

This happens in the case where an error occurs that causes the pipeline to
shutdown. Before that teardown completes and the OnError() callback is called,
the pipeline calls Stop(). 
 This basically prevents the OnError() call from occurring since the pipeline
already decided to stop before it even knew about the error.

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:925: if (error_caused_teardown_  && GetError() !=
PIPELINE_OK &&
On 2011/01/13 00:47:38, scherkus wrote:
> nit: get rid of extra space between error_caused_teardown_ and &&

Done.

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1157: pipeline_filter_->Stop(NewCallback(
On 2011/01/13 00:47:38, scherkus wrote:
> drop NewCallback to next line

Done.

http://codereview.chromium.org/6179006/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:1164: pipeline_filter_->Pause(NewCallback(
On 2011/01/13 00:47:38, scherkus wrote:
> drop NewCallback to next line

Done.

[scherkus (not reviewing), 2011-01-13 05:38:48]

cool!  check'er in!