Fix problem when we did not play beginning of HTML5 audio stream in low latency mode.

Fix problem when we did not play beginning of HTML5 audio stream in low latency mode.



Problem manifested itself on Mac because Mac code is especially sensitive to timing, and because Mac code ignores length of data in buffer -- it assumes buffer is always full, and gives to OS the entire buffer (zeroed out in that case).



Fix consists of 2 parts:

* When starting audio stream, send request for the data -- previously code depended on data length in the buffer being 0, so it assumed no data would be played, and data would be returned starting from 2nd request.

* Implement polling "is data ready?" when starting new stream. Start physical stream only after first chunk data is available (or after ~9ms had passed, to correctly handle "old" clients not writing metadata into buffer). Polling is not continuous, it is done by task delayed by 3ms.



Fix is definitely not necessary on Windows, where we have mechanism that handles "bad" timing when requesting new data, but I decided it is better to implement it on all platforms.



Added test.



BUG=98674
TEST=Go to http://www.jimmysparkle.me/dump/chrome-bug/ and listen to the audio stream.





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

[acolwell GONE FROM CHROMIUM, 2011-10-12 20:14:03]

Looks good. Just a few questions and please add some unit tests.

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

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.cc:196: void
AudioOutputController::DoPollIfDataReady() {
PollAndStartIfDataReady() might be a little more clear.

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

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:111: // returns false.
Why don't all renderers write into the buffer? Shouldn't we fix things to this
can be reliable?

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:175: static const int
kPollPauseInMilliseconds;
Why not just make these file static in the .cc?

[enal1, 2011-10-13 00:39:22]

Addressed what I could, explained what I could not, added unit test.

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

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.cc:196: void
AudioOutputController::DoPollIfDataReady() {
On 2011/10/12 20:14:03, acolwell wrote:
> PollAndStartIfDataReady() might be a little more clear.

Done.

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

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:111: // returns false.
On 2011/10/12 20:14:03, acolwell wrote:
> Why don't all renderers write into the buffer? Shouldn't we fix things to this
> can be reliable?

That is long and sad story. 3 months ago I tried to edit all code that writes
audio streams, and had to give up, because it is extremely hard to change glue
code that is in the plugin. We have pepper, native client, etc., and interfaces
for them are cut in the stone... After lot of attempts and code reverts I scaled
down my changes. As a result we cannot be sure that other side in the in the IPC
writes metadata...

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:175: static const int
kPollPauseInMilliseconds;
On 2011/10/12 20:14:03, acolwell wrote:
> Why not just make these file static in the .cc?

Because previous time I tried that I was told to make constants part of class
interface :-(

Actually, here it even makes sense -- when looking at the header file you can
deduce there are knobs you can tune, and values themselves are hidden.

[acolwell GONE FROM CHROMIUM, 2011-10-13 00:52:47]

LGTM

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

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:111: // returns false.
Ok. Sorry to hear this.
On 2011/10/13 00:39:22, enal1 wrote:
> On 2011/10/12 20:14:03, acolwell wrote:
> > Why don't all renderers write into the buffer? Shouldn't we fix things to
this
> > can be reliable?
> 
> That is long and sad story. 3 months ago I tried to edit all code that writes
> audio streams, and had to give up, because it is extremely hard to change glue
> code that is in the plugin. We have pepper, native client, etc., and
interfaces
> for them are cut in the stone... After lot of attempts and code reverts I
scaled
> down my changes. As a result we cannot be sure that other side in the in the
IPC
> writes metadata...

http://codereview.chromium.org/8229013/diff/1/media/audio/audio_output_contro...
media/audio/audio_output_controller.h:175: static const int
kPollPauseInMilliseconds;
On 2011/10/13 00:39:22, enal1 wrote:
> Because previous time I tried that I was told to make constants part of class
> interface :-(
> 
> Actually, here it even makes sense -- when looking at the header file you can
> deduce there are knobs you can tune, and values themselves are hidden.

OK.

[commit-bot: I haz the power, 2011-10-13 01:57:37]

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

[commit-bot: I haz the power, 2011-10-13 02:02:38]

Try job failure for 8229013-5001 on mac_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2011-10-13 14:04:40]

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

[commit-bot: I haz the power, 2011-10-13 16:18:06]

Change committed as 105311