Fix erratic HTML5 audio playback

Fix erratic HTML5 audio playback

This was affecting all platforms.  The major issue was that he SetPlaybackRate(1) which is used to start playback was being done in the middle of the series of Seek operations.  So doing Seek() followed by SetPlaybackRate(1) was done, the Seek could immediately stop the playback.  To fix, Seek() was made atomic with regards to SetPlaybackRate() by delaying the execution of SetPlaybackRate until after the Seek sequence has completely finished.

To make things run even smoother, a short delay was added before recyclilng a used audio stream in the Dispatcher.  This gives the stream time to 'power down' before being reused.

Tested on Linux, Chrome-OS (Cr48) and Mac with great results, making HTML5 audio much more usable for games.

BUG=73045, 59369, 59370, 65618
TEST=Manual, Quickly playing/stopping HTML5 audio should play cleanly.

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

[davejcool, 2011-04-09 03:50:56]

I'm getting very responsive real-time results playing HTML5 audio with this.

[Avi (use Gerrit), 2011-04-09 05:35:24]

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

http://codereview.chromium.org/6822019/diff/1/media/base/pipeline_impl.h#newc...
media/base/pipeline_impl.h:333: // Wheather or not a playback rate change should
be done once seeking is done.
drive-by: spelling of "Whether"

[Sergey Ulanov, 2011-04-09 07:02:04]

Thanks for fixing this, I like this approach.
Some nits. The only serious issue is that there may be some leaks.
Also, audio_output_proxy_unittest.cc may need to be updated.

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:67: idle_streams_.push_front(stream);
Looks to me that we may leak streams that are in idle_streams_ when dispatcher
is destroyed. Normally  StreamClosed() ensures that streams are destroyed
necessary, but it would not happen with this change, because unused streams may
be in idle_streams_

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:69: // Don't recycle stream until two
buffers worth of time has elapsed
nit: period at the end of the comment.

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:72: 2 * params_.samples_per_packet * 1000
/ params_.sample_rate);
nit: This line is not indented properly. I would move FROM_HERE to a separate
line.

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:72: 2 * params_.samples_per_packet * 1000
/ params_.sample_rate);
nit: replace 1000 with Time::kMillisecondsPerSecond from base/time.h

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.h:93: std::list<AudioOutputStream*>
idle_streams_;
Please add comments that explain difference between streams_ and idle_streams_.
I'm not sure if idle_streams_ is a good name. Maybe call it pausing_streams_,
and rename streams_ to idle_streams_, to show the difference? In that case we
would not need a comment.

[scherkus (not reviewing), 2011-04-11 22:32:33]

more nits

could you add a test to pipeline_impl_unittests?

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.cc:72: 2 * params_.samples_per_packet * 1000
/ params_.sample_rate);
it looks like you can pre-calculate this since params_ is passed in via the ctor
and (shouldn't) change

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/6822019/diff/1/media/audio/audio_output_dispat...
media/audio/audio_output_dispatcher.h:25: #include <list>
nit: list comes before vector

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

http://codereview.chromium.org/6822019/diff/1/media/base/pipeline_impl.cc#new...
media/base/pipeline_impl.cc:788: base::AutoLock auto_lock(lock_);
I believe we don't need the lock here since both of these variables are only
accessed on message_loop_ thread

[davejcool, 2011-04-12 02:37:41]

Here's an update addressing the various nits and such.

[Sergey Ulanov, 2011-04-12 05:09:25]

http://codereview.chromium.org/6822019/diff/1006/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/1006/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:18: pause_delay_milliseconds( 2 *
params.samples_per_packet *
nit: remove space before 2

http://codereview.chromium.org/6822019/diff/1006/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:26:
AudioOutputDispatcher::~AudioOutputDispatcher() {
Cleanup pausing_streams here.

http://codereview.chromium.org/6822019/diff/1006/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:132: pausing_streams_.pop_back();
If you close |pausing_streams_| here you may be closing a stream that is still
playing. Please add close_timer_.Reset() in StreamStopped() to avoid this.
Normally |pausing_streams_| will be empty here.

http://codereview.chromium.org/6822019/diff/1006/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/6822019/diff/1006/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.h:92: int64 pause_delay_milliseconds;
pause_delay_milliseconds_

[davejcool, 2011-04-12 22:54:52]

I put the close_timer_.Reset() back in StreamStopped(), and it didn't need to be
in the StopStreamTask() since that gets called much sooner
(pause_delay_milliseconds_ is much less than the close_delay_ms).

[Sergey Ulanov, 2011-04-13 02:02:54]

Changes in AudioOutputDispatcher LGTM.
I'll leave pipeline review to Andrew.

http://codereview.chromium.org/6822019/diff/7001/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/7001/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:77: // Don't recycle stream until two
buffers worth of time has elapsed
nit: period at the end of the sentence.

[scherkus (not reviewing), 2011-04-13 16:02:01]

LGTM -- this might affect how media layout tests runs so try giving them a run
before committing

thanks again -- this looks great!

[davejcool, 2011-04-14 00:43:26]

A couple of the media unit tests were failing because of a close stream not
actually being closed, and more than one stream being opened unexpectedly. 
These changes should fix that... When the 'try' finishes, we'll see if it
worked.

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:90: 
When a stream is finally closed, it should be added to idle_streams_ and not
just stick around in the pausing_streams_.  Also, unit tests fail without this
since a close stream is expected to be closed for real right away.

[scherkus (not reviewing), 2011-04-14 17:11:08]

thanks for making sure tests still passed!

one tiny tiny style nit but this is all LGTM so check in whenever trybots are
happy

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:127: && pausing_streams_.empty()) {
nit: && goes on end of previous line

[Sergey Ulanov, 2011-04-14 18:05:08]

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:92:
idle_streams_.push_back(pausing_streams_.back());
I don't like this. Basically when a single stream closing, you are moving all
other streams from pausing_streams_ to idle_streams_, so playback on other
streams may be disrupted.
Can we update tests to reflect new behaviour instead of cleaning
|pausing_streams_| here?

[davejcool, 2011-04-15 01:12:51]

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/6822019/diff/8003/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:92:
idle_streams_.push_back(pausing_streams_.back());
On 2011/04/14 18:05:09, sergeyu wrote:
> I don't like this. Basically when a single stream closing, you are moving all
> other streams from pausing_streams_ to idle_streams_, so playback on other
> streams may be disrupted.
> Can we update tests to reflect new behaviour instead of cleaning
> |pausing_streams_| here?

Closing the pausing_streams_ later can not be done in the constructor because
that's not on the right thread.  Some other ideas are:
(1) If we knew what physical stream the caller was using when StreamClosed() is
called, we could just move that one only.
(2) If idle_streams_ was also a <list>, the pausing streams could be pushed to
the front, so at they'd be getting closed later than the other streams, and much
less likely to be disrupted.