Harden audio output controller making it safe to call Pause() before

Harden audio output controller making it safe to call Pause() before Play() really started. Not sure if it really fixes the crash, but crash started happening only after Play() was split into several tasks and become much slower (http://codereview.chromium.org/8229013/ change 105311).

Added unit test.

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

[acolwell GONE FROM CHROMIUM, 2011-10-24 03:47:15]

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller.cc (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.cc:239: message_loop_->PostDelayedTask(
This doesn't smell right. Can we add a kPauseWhileStarting state to track this?
When PollAndStartIfDataReady() is called and sees this state, it could change
state to kPlaying and then call DoPause(). If we get another DoPlay() then it
will see the kPauseWhileStarting and just transition to kStarting. Would this
work or am I missing something?

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:33: //               [ Created ]  -->  [
Playing ]  -->  [ Paused ]
Please update the diagram & comments to reflect the new Starting state.

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller_unittest.cc:271: .Times(Exactly(1))
I don't think you need the Times(Exactly(1)). I believe it assumes that by
default.

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller_unittest.cc:276: .Times(Exactly(1));
ditto

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller_unittest.cc:289: .Times(Exactly(1))
ditto

[enal1, 2011-10-24 16:28:29]

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller.cc (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.cc:239: message_loop_->PostDelayedTask(
On 2011/10/24 03:47:15, acolwell wrote:
> This doesn't smell right. Can we add a kPauseWhileStarting state to track
this?
> When PollAndStartIfDataReady() is called and sees this state, it could change
> state to kPlaying and then call DoPause(). If we get another DoPlay() then it
> will see the kPauseWhileStarting and just transition to kStarting. Would this
> work or am I missing something?

Initially I wrote code similar to what you suggest, looked at it, and decided to
go to something extra simple. Code was non-trivial and error prone -- even if
correct right now it can become obsolete later, and it always would be hard to
fully test it, as it is not on normal path...

I deliberately reusing kPollPauseInMilliseconds, not introducing new constant
kPauseWhileStarting. We want this task to work after PollAndStartIfDataReady(),
and using the same delay guarantees it. Added comment.

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:33: //               [ Created ]  -->  [
Playing ]  -->  [ Paused ]
On 2011/10/24 03:47:15, acolwell wrote:
> Please update the diagram & comments to reflect the new Starting state.

Done.

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller_unittest.cc:271: .Times(Exactly(1))
On 2011/10/24 03:47:15, acolwell wrote:
> I don't think you need the Times(Exactly(1)). I believe it assumes that by
> default.

Done.

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller_unittest.cc:276: .Times(Exactly(1));
On 2011/10/24 03:47:15, acolwell wrote:
> ditto

Done.

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller_unittest.cc:289: .Times(Exactly(1))
On 2011/10/24 03:47:15, acolwell wrote:
> ditto

Done.

[acolwell GONE FROM CHROMIUM, 2011-10-24 16:59:57]

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
File media/audio/audio_output_controller.cc (right):

http://codereview.chromium.org/8371013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.cc:239: message_loop_->PostDelayedTask(
On 2011/10/24 16:28:29, enal1 wrote:
> Initially I wrote code similar to what you suggest, looked at it, and decided
to
> go to something extra simple. Code was non-trivial and error prone -- even if
> correct right now it can become obsolete later, and it always would be hard to
> fully test it, as it is not on normal path...
I'm afraid I'm going to have to insist here. I REALLY don't like using time to
hold state. It is error prone and can introduce subtle bugs. Why would this be
hard to test? It seems that forcing state transitions would be easier to test
than verifying time based interactions.

> 
> I deliberately reusing kPollPauseInMilliseconds, not introducing new constant
> kPauseWhileStarting. We want this task to work after
PollAndStartIfDataReady(),
> and using the same delay guarantees it. Added comment.

[enal1, 2011-10-24 19:00:47]

Changed the code, introduced new state.

[acolwell GONE FROM CHROMIUM, 2011-10-24 19:45:37]

LGTM

[commit-bot: I haz the power, 2011-10-24 19:49:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/8371013/9001

[commit-bot: I haz the power, 2011-10-24 21:58:54]

Change committed as 106983