Fix start/stop of html5 audio stream and race condition when pausing.

Fix start/stop of html5 audio stream and race condition when pausing.
(a) Update earliest time when html5 audio can end, thus fixing problem
    when the stream was being truncated too early,
(b) Set length of data in buffer, so polling code used for several first
    buffers can stop waiting the moment data is ready,
(c) Fix zeroing out end of buffer in partially filled buffer -- code was zeroing
    correct number of bytes, but at the beginning of buffer, not at the end.
    Lot of thanks to Shijing for identifying the issue, I heard that something is
    wrong but could not figure out exact cause.
(d) Zero length of data in buffer when AudioDevice receives 'pause' command,
    thus work around race condition when one thread pausing the playback and other
    simultaneously asks for more data -- depending on exact timing buffer could
    be left in the unpredictable state, resulting in the next call for data getting
    extra buffer of silence (not fatal, but very noticeable when repeatedly playing
    short audio stream).
(e) Remove pre-rendering of first buffer, it is not necessary for <audio>,
    WebAudio (confirmed by Chris), and WebRTC (confirmed by Shijing).

WebRTC/WebAudio changes are actually no-op, as their renderers now just return
'I've filled the entire buffer'.

Note: that CL fixes html5 audio playback of short streams. It is possible to further
improve the code and maybe get rid of storing length of data in the buffer by changing
the way AudioRendererBase::FillBuffer() signals end of playback. Right now it is
synchronous, done only when host asks for more data, we probably can make it
asynchronous, by issuing delayed task into the child process IO message loop,
but that should be separate CL.

TEST=Please load the page http://www.corp.google.com/~tommi/average.html and listen.
TEST=I'll add layout test that verifies that timing is reasonable, but it will take
TEST=some time to add.

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

[Chris Rogers, 2011-12-14 20:48:35]

Hi Eugene, thanks very much for doing this.  I just had a quick look and added a
couple "nit" comments.  I haven't had a chance to look really hard at the code,
but it seems on the surface to be right.  I'm glad to see the fixes are
relatively localized.

http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_device.h:85: virtual size_t Render(const
std::vector<float*>& audio_data,
probably should add comment about the return value

http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_device.h:167: size_t FireRenderCallback(int16*
data);
comment about return value

[enal1, 2011-12-14 21:03:40]

On 2011/12/14 20:48:35, Chris Rogers wrote:
> Hi Eugene, thanks very much for doing this.  I just had a quick look and added
a
> couple "nit" comments.  I haven't had a chance to look really hard at the
code,
> but it seems on the surface to be right.  I'm glad to see the fixes are
> relatively localized.
> 
>
http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
> File content/renderer/media/audio_device.h (right):
> 
>
http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
> content/renderer/media/audio_device.h:85: virtual size_t Render(const
> std::vector<float*>& audio_data,
> probably should add comment about the return value
> 
>
http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
> content/renderer/media/audio_device.h:167: size_t FireRenderCallback(int16*
> data);
> comment about return value

Updated comments, fixed zeroing of end of buffer for the partially filled
buffer. Still no new test, working.

[enal1, 2011-12-14 22:14:53]

Sending for official review.

Addressed comments and got rid of pre-rendering of first buffer per discussion.

Added instructions for manual tests. Will add layout test that verifies that
timing is reasonable, but it will take long time to add it...

http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_device.h:85: virtual size_t Render(const
std::vector<float*>& audio_data,
On 2011/12/14 20:48:35, Chris Rogers wrote:
> probably should add comment about the return value

Made existing comment more clear.

http://codereview.chromium.org/8909006/diff/5001/content/renderer/media/audio...
content/renderer/media/audio_device.h:167: size_t FireRenderCallback(int16*
data);
On 2011/12/14 20:48:35, Chris Rogers wrote:
> comment about return value

Done.

[Chris Rogers, 2011-12-15 00:32:52]

Looks pretty good.  I have a couple of comment nits

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/audio...
content/renderer/media/audio_device.h:86: // of the shared memory (i.e. buffer),
and host is free to use or ignore it.
nit: the implementation details of "shared memory" should not be mentioned at
this level in this comment because this method is an abstract interface and
AudioDevice is free to change its implementation.  The implementor of Render()
should not even be aware of "shared memory".  Instead we should just say that
Render() is expected to put number_of_frames of a continuous stream of PCM audio
data into audio_data.  If it doesn't have that much available then it should
return a value less than number_of_frames (otherwise just return
number_of_frames).  This can probably be worded better, but you get the idea.

But we can move the part describing details of "shared memory" down below to
FireRenderCallback().  Then you can remove the comment down there which says
"see comment on RenderCallback::Render()"

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/audio...
content/renderer/media/audio_device.h:168: // RenderCallback::Render().
See my comment above.

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.h:9: #include <string>
Do we really need to include this just to get "size_t" ?

[enal1, 2011-12-15 16:54:40]

Addressed everything.

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/audio...
content/renderer/media/audio_device.h:86: // of the shared memory (i.e. buffer),
and host is free to use or ignore it.
On 2011/12/15 00:32:52, Chris Rogers wrote:
> nit: the implementation details of "shared memory" should not be mentioned at
> this level in this comment because this method is an abstract interface and
> AudioDevice is free to change its implementation.  The implementor of Render()
> should not even be aware of "shared memory".  Instead we should just say that
> Render() is expected to put number_of_frames of a continuous stream of PCM
audio
> data into audio_data.  If it doesn't have that much available then it should
> return a value less than number_of_frames (otherwise just return
> number_of_frames).  This can probably be worded better, but you get the idea.
> 
> But we can move the part describing details of "shared memory" down below to
> FireRenderCallback().  Then you can remove the comment down there which says
> "see comment on RenderCallback::Render()"

Done.

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/audio...
content/renderer/media/audio_device.h:168: // RenderCallback::Render().
On 2011/12/15 00:32:52, Chris Rogers wrote:
> See my comment above.

Done.

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/8909006/diff/2018/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.h:9: #include <string>
On 2011/12/15 00:32:52, Chris Rogers wrote:
> Do we really need to include this just to get "size_t" ?

We need it because in line 125 we are using std::string.

"gcl lint" complained, and I decided it can be fixed as well, because it does
not cost me anything.

[Chris Rogers, 2011-12-15 21:05:25]

+henrika, xians for final sanity check

LGTM

This shouldn't change normal operation of AudioDevice clients, but henrika
and/or xians should give a quick look before landing since this is a sensitive
zone

[henrika (OOO until Aug 14), 2011-12-16 10:52:05]

LGTM (with nits).

We will run some tests as well on out side and come back with feedback. 

Eugene, it would be great if you could run the WebRTC unit tests in
content_unittests as well to verify.

[henrika (OOO until Aug 14), 2011-12-16 10:52:16]

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:311: // Let the host know we are done.
Shouldn't comments have an empty line above when they are not first in scope?

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.h:171: // to it capacity.
its

[no longer working on chromium, 2011-12-16 12:21:13]

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:291: base::SyncSocket
socket(socket_handle_);
I am actually thinking about the opposite solution.
In the current AudioOutputController::DoPlay() we ask for first packet by
sync_reader_->UpdatePendingBytes(0); then we wait 3ms for the packet to be
ready, which means we will have at least 3ms delay in start playing.
I don't think it is a good approach, 3ms is not much but I don't see why we give
it up here.
An easy fix is that we still do the pre-buffer here, and in DoPlay() we check if
sync_reader_->DataReady(), if it is, start the stream immediately, otherwise
wait for 3ms.

And inside the loop, when pending_data ==
media::AudioOutputController::kPauseMark, we should call
media::SetUnknownDataSize(shared_memory_, memory_length_);

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:249: sizeof(float) *
frames_to_zero);
Do we really need this?
in AudioSyncReader::Read() we only copy the actually data and zero the
remainder.

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.h:9: #include <string>
Just want to make sure this is intentional.

[enal1, 2011-12-16 16:48:20]

Run content_unittests.

Inserted blank lines before comments, replied to other comments...

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:291: base::SyncSocket
socket(socket_handle_);
On 2011/12/16 12:21:13, xians1 wrote:
> I am actually thinking about the opposite solution.
> In the current AudioOutputController::DoPlay() we ask for first packet by
> sync_reader_->UpdatePendingBytes(0); then we wait 3ms for the packet to be
> ready, which means we will have at least 3ms delay in start playing.
> I don't think it is a good approach, 3ms is not much but I don't see why we
give
> it up here.
> An easy fix is that we still do the pre-buffer here, and in DoPlay() we check
if
> sync_reader_->DataReady(), if it is, start the stream immediately, otherwise
> wait for 3ms.
> 
> And inside the loop, when pending_data ==
> media::AudioOutputController::kPauseMark, we should call
> media::SetUnknownDataSize(shared_memory_, memory_length_);

I believe I answered once. Pre-buffering does not work for <audio>, because we
can play the same stream again and again, so Play() does not know if it is the
first DoPlay() ever called -- in this case there should be pre-buffered data --
or if it is resuming after pause, and in this case there is no pre-buffered
data. Of course I can set up flag in AudioOutputController, etc., but current
code is much cleaner. If those 3ms ever become an issue we can use polling loop
with yield() there.

Pre-buffering can hurt when stopping <audio> as well. We are stopping audio
streams using "earliest ending time", and in case of very short stream it would
be "time at the moment of pre-buffering", not "time when we started playback".
It is bad right now, we are losing several last milliseconds, as "time now" is
time for 1st buffer, and we would call OnMoreData() up to 2 more times before
starting playback, with wait if data is not ready. My pending CL should improve
things a lot, but pre-buffering would hurt again.

And we'll change to software mixing soon, and have to modify that code anyways.

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.cc:311: // Let the host know we are done.
On 2011/12/16 10:52:17, henrika wrote:
> Shouldn't comments have an empty line above when they are not first in scope?

Done.

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_device.h:171: // to it capacity.
On 2011/12/16 10:52:17, henrika wrote:
> its

Done.

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio...
content/renderer/media/audio_renderer_impl.cc:249: sizeof(float) *
frames_to_zero);
On 2011/12/16 12:21:13, xians1 wrote:
> Do we really need this?
> in AudioSyncReader::Read() we only copy the actually data and zero the
> remainder.

Yes, we probably do not need that, but I feel safer, and zeroing out 16Kb is not
a big deal...

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_impl.h:9: #include <string>
On 2011/12/16 12:21:13, xians1 wrote:
> Just want to make sure this is intentional.

Yes, it is intentional, I already explained to Chris. We are using std::string
in line 125 of this file, and "gcl lint" complains -- "using string without
including <string>". I thought I can fix that as well, it would not cost
anything. Obviously I was wrong :-(...

[no longer working on chromium, 2011-12-16 17:46:04]

Regarding to the pre buffer:
For the first doplay(), dataready() returns true because of the prebuffer,
and for the following  doplay(), since pause signal will set the share
memory to unknowndata, dataready() returns false, so doplay() will buffer
the data. So we don't need a new flag in audiooutputcontroller.

For the stopping of the stream, if you take the 3ms into account, the
prebiffee will not be worse than what it is now. And I did some tests
before, old audiorendererimpl takes about 15s to finish playing the 100
times click, and audiodevice with your cl takes 19s. I don't know the
length of the file, so don't know which one is more correct, but I do worry
that the problem is not that we end too early, but the opposite, we end too
late.

Generally I am quite happy with this cl, I don't think i should block the
cl. Lgtm regardless you agree with the pre buffer things or not.
On Dec 16, 2011 5:48 PM, <enal@google.com> wrote:

> Run content_unittests.
>
> Inserted blank lines before comments, replied to other comments...
>
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_device.cc<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_device.cc>
> File content/renderer/media/audio_**device.cc (right):
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_device.**cc#newcode291<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_device.cc#newcode291>
> content/renderer/media/audio_**device.cc:291: base::SyncSocket
> socket(socket_handle_);
> On 2011/12/16 12:21:13, xians1 wrote:
>
>> I am actually thinking about the opposite solution.
>> In the current AudioOutputController::DoPlay(**) we ask for first packet
>>
> by
>
>> sync_reader_->**UpdatePendingBytes(0); then we wait 3ms for the packet
>>
> to be
>
>> ready, which means we will have at least 3ms delay in start playing.
>> I don't think it is a good approach, 3ms is not much but I don't see
>>
> why we give
>
>> it up here.
>> An easy fix is that we still do the pre-buffer here, and in DoPlay()
>>
> we check if
>
>> sync_reader_->DataReady(), if it is, start the stream immediately,
>>
> otherwise
>
>> wait for 3ms.
>>
>
>  And inside the loop, when pending_data ==
>> media::AudioOutputController::**kPauseMark, we should call
>> media::SetUnknownDataSize(**shared_memory_, memory_length_);
>>
>
> I believe I answered once. Pre-buffering does not work for <audio>,
> because we can play the same stream again and again, so Play() does not
> know if it is the first DoPlay() ever called -- in this case there
> should be pre-buffered data -- or if it is resuming after pause, and in
> this case there is no pre-buffered data. Of course I can set up flag in
> AudioOutputController, etc., but current code is much cleaner. If those
> 3ms ever become an issue we can use polling loop with yield() there.
>
> Pre-buffering can hurt when stopping <audio> as well. We are stopping
> audio streams using "earliest ending time", and in case of very short
> stream it would be "time at the moment of pre-buffering", not "time when
> we started playback". It is bad right now, we are losing several last
> milliseconds, as "time now" is time for 1st buffer, and we would call
> OnMoreData() up to 2 more times before starting playback, with wait if
> data is not ready. My pending CL should improve things a lot, but
> pre-buffering would hurt again.
>
> And we'll change to software mixing soon, and have to modify that code
> anyways.
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_device.**cc#newcode311<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_device.cc#newcode311>
> content/renderer/media/audio_**device.cc:311: // Let the host know we are
> done.
> On 2011/12/16 10:52:17, henrika wrote:
>
>> Shouldn't comments have an empty line above when they are not first in
>>
> scope?
>
> Done.
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_device.h<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_device.h>
> File content/renderer/media/audio_**device.h (right):
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_device.h#**newcode171<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_device.h#newcode171>
> content/renderer/media/audio_**device.h:171: // to it capacity.
> On 2011/12/16 10:52:17, henrika wrote:
>
>> its
>>
>
> Done.
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_renderer_**impl.cc<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_renderer_impl.cc>
> File content/renderer/media/audio_**renderer_impl.cc (right):
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/audio_renderer_**impl.cc#newcode249<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/audio_renderer_impl.cc#newcode249>
> content/renderer/media/audio_**renderer_impl.cc:249: sizeof(float) *
> frames_to_zero);
> On 2011/12/16 12:21:13, xians1 wrote:
>
>> Do we really need this?
>> in AudioSyncReader::Read() we only copy the actually data and zero the
>> remainder.
>>
>
> Yes, we probably do not need that, but I feel safer, and zeroing out
> 16Kb is not a big deal...
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/webrtc_audio_**device_impl.h<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/webrtc_audio_device_impl.h>
> File content/renderer/media/webrtc_**audio_device_impl.h (right):
>
> http://codereview.chromium.**org/8909006/diff/8001/content/**
>
renderer/media/webrtc_audio_**device_impl.h#newcode9<http://codereview.chromium.org/8909006/diff/8001/content/renderer/media/webrtc_audio_device_impl.h#newcode9>
> content/renderer/media/webrtc_**audio_device_impl.h:9: #include <string>
> On 2011/12/16 12:21:13, xians1 wrote:
>
>> Just want to make sure this is intentional.
>>
>
> Yes, it is intentional, I already explained to Chris. We are using
> std::string in line 125 of this file, and "gcl lint" complains -- "using
> string without including <string>". I thought I can fix that as well, it
> would not cost anything. Obviously I was wrong :-(...
>
>
http://codereview.chromium.**org/8909006/<http://codereview.chromium.org/8909...
>

[enal1, 2011-12-16 18:05:25]

> Regarding to the pre buffer:> For the first doplay(), dataready() returns true
because of the prebuffer,> and for the following  doplay(), since pause signal
will set the share> memory to unknowndata, dataready() returns false, so
doplay() will buffer> the data. So we don't need a new flag in
audiooutputcontroller.
Unfortunately that is not so -- it is not enough to set buffer state
to "unknown", after resuming eventually you should have valid data in
buffer. You can resume from the arbitrary position, so you have to
send request for data anyways.

I am not saying it cannot be done, you probably can implement
pre-buffering in DoSeek() as well, but protocol becomes more and more
complex, and it is very complex and time-sensitive already.

Thanks for lgtm.

[commit-bot: I haz the power, 2011-12-16 18:09:16]

No LGTM from valid reviewers yet.

[enal1, 2011-12-16 18:12:49]

On 2011/12/16 18:09:16, I haz the power (commit-bot) wrote:
> No LGTM from valid reviewers yet.

Oops, still no lgtm from Tommi... :-(

[tommi (sloooow) - chröme, 2011-12-16 22:51:31]

oops, sorry. Rubber stamp lgtm
On Dec 16, 2011 7:12 PM, <enal@google.com> wrote:

> On 2011/12/16 18:09:16, I haz the power (commit-bot) wrote:
>
>> No LGTM from valid reviewers yet.
>>
>
> Oops, still no lgtm from Tommi... :-(
>
>
http://codereview.chromium.**org/8909006/<http://codereview.chromium.org/8909...
>

[commit-bot: I haz the power, 2011-12-17 01:26:46]

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

[commit-bot: I haz the power, 2011-12-17 01:26:52]

Presubmit check for 8909006-13003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/renderer/renderer_webaudiodevice_impl.h,content/renderer/renderer_webaudiodevice_impl.cc

Presubmit checks took 1.8s to calculate.

[Avi (use Gerrit), 2011-12-20 23:01:59]

owner rubberstamp LGTM

[commit-bot: I haz the power, 2011-12-20 23:51:41]

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

[commit-bot: I haz the power, 2011-12-21 01:27:05]

Change committed as 115253