Update AudioRenderer API to fire changes in BufferingState.

Update AudioRenderer API to fire changes in BufferingState.

As a result, Pipeline now handles prerolling and underflow/rebuffering
by listening for BUFFERING_HAVE_NOTHING/ENOUGH callbacks. Preroll() is
renamed StartPlayingFrom() and no longer accepts a completion callback.
In this new model, AudioRenderers immediately enter and remain in
the "playing" state and fire buffering state callbacks to let Pipeline
know when to start/stop the clock.

BUG=144683
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272465

[scherkus (not reviewing), 2014-05-13 04:15:16]

acolwell: looking for preliminary feedback on API names, etc... but feel free to
do a full review if you think it's good

as far as local testing is concerned, everything is pretty much passing (e.g.,
media_unittests, content_browsertests, layout tests) and I did a bunch of manual
tests using simulated bandwidth and underflow/rebuffer works great

[scherkus (not reviewing), 2014-05-13 04:18:20]

https://codereview.chromium.org/284763002/diff/50001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test_base.cc (left):

https://codereview.chromium.org/284763002/diff/50001/media/filters/pipeline_i...
media/filters/pipeline_integration_test_base.cc:290:
audio_renderer_impl->DisableUnderflowForTesting();
note to self ... I need to verify if this still works

for example do we now need a Pipeline::DisableUnderflowForTesting()?

I kind of forget why this is in here

[DaleCurtis, 2014-05-13 17:54:00]

https://codereview.chromium.org/284763002/diff/50001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test_base.cc (left):

https://codereview.chromium.org/284763002/diff/50001/media/filters/pipeline_i...
media/filters/pipeline_integration_test_base.cc:290:
audio_renderer_impl->DisableUnderflowForTesting();
On 2014/05/13 04:18:20, scherkus wrote:
> note to self ... I need to verify if this still works
> 
> for example do we now need a Pipeline::DisableUnderflowForTesting()?
> 
> I kind of forget why this is in here

It's so frames aren't dropped which would mess up the hash.

[scherkus (not reviewing), 2014-05-13 18:00:45]

On 2014/05/13 17:54:00, DaleCurtis wrote:
>
https://codereview.chromium.org/284763002/diff/50001/media/filters/pipeline_i...
> File media/filters/pipeline_integration_test_base.cc (left):
> 
>
https://codereview.chromium.org/284763002/diff/50001/media/filters/pipeline_i...
> media/filters/pipeline_integration_test_base.cc:290:
> audio_renderer_impl->DisableUnderflowForTesting();
> On 2014/05/13 04:18:20, scherkus wrote:
> > note to self ... I need to verify if this still works
> > 
> > for example do we now need a Pipeline::DisableUnderflowForTesting()?
> > 
> > I kind of forget why this is in here
> 
> It's so frames aren't dropped which would mess up the hash.

We don't drop audio frames though.

Perhaps you mean Render() returning muted audio due to being underflowed? Anyway
I tried running all of media_unittests and they still pass. Do you know which
tests uses audio hashing?

[DaleCurtis, 2014-05-13 18:03:34]

Hmm, I thought underflow of audio could cause the video to drop frames. 
Definitely muting would affect the hash.  Any pipeline test that has a
GetAudioHash() call would be affected.

[scherkus (not reviewing), 2014-05-13 18:53:41]

On 2014/05/13 18:03:34, DaleCurtis wrote:
> Hmm, I thought underflow of audio could cause the video to drop frames. 
> Definitely muting would affect the hash.  Any pipeline test that has a
> GetAudioHash() call would be affected.

http://crrev.com/140123 claims it's due to silence when underflowed, which would
happen under valgrind.

However it *might* not be necessary anymore since this CL pauses the audio sink
when we underflow whereas before we'd keep the sink running while we rebuffered.
I'll play around to see if I can get it to repro.

[acolwell GONE FROM CHROMIUM, 2014-05-13 20:44:26]

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc
File media/base/pipeline.cc (left):

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc#o...
media/base/pipeline.cc:883: if (!task_runner_->BelongsToCurrentThread()) {
\o/ I'm happy to see this method die. It lived WAY longer than I had originally
intended.

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc#n...
media/base/pipeline.cc:866:
BindToCurrentLoop(base::Bind(&Pipeline::BufferingStateChanged,
nit: Shouldn't we hide this wrapping in the AudioRendererImpl? That way the
followup CL only has to include renderer changes.

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc#n...
media/base/pipeline.cc:934: task_runner_->PostTask(
Why do you need to PostTask() here and above now? Can't this open the door for
WaitingForEnoughData() to change state before StartPlayback() is called?

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:895: //DoSeek(state, stop_or_error);
remove?

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:185: ChangeState_Locked(kFlushed);
Why do we need this? The DCHECK on line 171 indicates that we are already in
this state.

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
media/filters/audio_renderer_impl.h:108: //         `---------> kPlaying
--------'
\o/ so much nicer

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:578:
//event.RunAndWaitForStatus(PIPELINE_OK);
remove?

[scherkus (not reviewing), 2014-05-13 20:45:32]

On 2014/05/13 18:53:41, scherkus wrote:
> On 2014/05/13 18:03:34, DaleCurtis wrote:
> > Hmm, I thought underflow of audio could cause the video to drop frames. 
> > Definitely muting would affect the hash.  Any pipeline test that has a
> > GetAudioHash() call would be affected.
> 
> http://crrev.com/140123 claims it's due to silence when underflowed, which
would
> happen under valgrind.
> 
> However it *might* not be necessary anymore since this CL pauses the audio
sink
> when we underflow whereas before we'd keep the sink running while we
rebuffered.
> I'll play around to see if I can get it to repro.

Did some digging. I think this code hasn't been necessary since we removed the
"return 8192 bytes of zero when we have no data" hack. i.e., I couldn't repro
hash failures running trunk code w/ underflow-disabling removed under valgrind.

I'll try sending out a separate CL that removes it so we can verify separately
from this CL.

[scherkus (not reviewing), 2014-05-15 23:28:42]

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc#n...
media/base/pipeline.cc:866:
BindToCurrentLoop(base::Bind(&Pipeline::BufferingStateChanged,
On 2014/05/13 20:44:27, acolwell_OOO_5-12_to_5-15 wrote:
> nit: Shouldn't we hide this wrapping in the AudioRendererImpl? That way the
> followup CL only has to include renderer changes.

Done.

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline.cc#n...
media/base/pipeline.cc:934: task_runner_->PostTask(
On 2014/05/13 20:44:27, acolwell_OOO_5-12_to_5-15 wrote:
> Why do you need to PostTask() here and above now? Can't this open the door for
> WaitingForEnoughData() to change state before StartPlayback() is called? 

it's to avoid reentrancy into renderers ... but I think this was before I did
the BindToCurrentLoop() so this shouldn't be necessary anymore

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:895: //DoSeek(state, stop_or_error);
On 2014/05/13 20:44:27, acolwell_OOO_5-12_to_5-15 wrote:
> remove?

Done.

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:185: ChangeState_Locked(kFlushed);
On 2014/05/13 20:44:27, acolwell_OOO_5-12_to_5-15 wrote:
> Why do we need this? The DCHECK on line 171 indicates that we are already in
> this state.

Done.

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
File media/filters/audio_renderer_impl_unittest.cc (right):

https://codereview.chromium.org/284763002/diff/50001/media/filters/audio_rend...
media/filters/audio_renderer_impl_unittest.cc:578:
//event.RunAndWaitForStatus(PIPELINE_OK);
On 2014/05/13 20:44:27, acolwell_OOO_5-12_to_5-15 wrote:
> remove?

Done.

[acolwell GONE FROM CHROMIUM, 2014-05-16 17:25:08]

lgtm % comments

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:175: buffering_state_ =
BUFFERING_HAVE_NOTHING;
Why don't we dispatch a callback in this case? The 2 other sets send a callback.

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:257: // TODO(scherkus): Enforce
AudioRendererImpl to call on right thread.
Is this still a TODO? It looks like you are ensuring this right here.

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:419: if (buffer->end_of_stream()) {
nit: add lock_.AssertAcquired() and possibly _Locked to the method name.

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:457:
buffering_state_cb_.Run(BUFFERING_HAVE_ENOUGH);
nit: Put these 2 operations in a SetBufferingState_Locked() method so it's easy
to debug transitions like with ChangeState_Locked()?

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.h:224: 
This should go somewhere below the lock_ decl since this variable is accessed by
both threads and should only be modified under lock.

[scherkus (not reviewing), 2014-05-22 17:49:12]

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:175: buffering_state_ =
BUFFERING_HAVE_NOTHING;
On 2014/05/16 17:25:08, acolwell wrote:
> Why don't we dispatch a callback in this case? The 2 other sets send a
callback.

as discussed offline ... updated API docs + impls + tests to have clients expect
this callback

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:257: // TODO(scherkus): Enforce
AudioRendererImpl to call on right thread.
On 2014/05/16 17:25:08, acolwell wrote:
> Is this still a TODO? It looks like you are ensuring this right here.

yeah .. I guess so :\

would rather have ARI be single threaded I guess, but it's not really
TODO-worthy

changed to comment describing why it's BTCL'd

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:419: if (buffer->end_of_stream()) {
On 2014/05/16 17:25:08, acolwell wrote:
> nit: add lock_.AssertAcquired() and possibly _Locked to the method name.

Done.

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:457:
buffering_state_cb_.Run(BUFFERING_HAVE_ENOUGH);
On 2014/05/16 17:25:08, acolwell wrote:
> nit: Put these 2 operations in a SetBufferingState_Locked() method so it's
easy
> to debug transitions like with ChangeState_Locked()?

Done.

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/284763002/diff/90001/media/filters/audio_rend...
media/filters/audio_renderer_impl.h:224: 
On 2014/05/16 17:25:08, acolwell wrote:
> This should go somewhere below the lock_ decl since this variable is accessed
by
> both threads and should only be modified under lock.

Done.

[scherkus (not reviewing), 2014-05-22 21:45:17]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-05-22 21:46:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/284763002/170001

[commit-bot: I haz the power, 2014-05-23 03:40:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-23 06:49:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-23 06:49:33]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[scherkus (not reviewing), 2014-05-23 07:54:48]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-05-23 07:58:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/284763002/170001

[commit-bot: I haz the power, 2014-05-23 10:30:47]

Change committed as 272465

[alph, 2014-05-23 12:06:48]

A revert of this CL has been created in
https://codereview.chromium.org/291093013/ by alph@chromium.org.

The reason for reverting is: Broke blink tests:
mediasource-redundant-seek.html
mediasource-seek-during-pending-seek.html

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds/....