Have AudioRendererImpl advance time until it's told to stop.

Have AudioRendererImpl advance time until it's told to stop.

This removes the hacky fall-back-to-interpolated-time code when audio
ends before video. Considering we're still getting audio render
callbacks during this time, we can simply account for how much
silence we've written to compute the media time.

More importantly, this brings AudioRendererImpl's TimeSource
implementation in line with how WallClockTimeSource operates and gets
us one step closer to removing TimeDeltaInterpolator entirely.

BUG=370634
R=dalecurtis@chromium.org, xhwang@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/af1afa545ba0eecc6daf38be9d94073798f86848

[scherkus (not reviewing), 2014-08-29 19:23:26]

scherkus@chromium.org changed reviewers:
+ dalecurtis@chromium.org, xhwang@chromium.org

[scherkus (not reviewing), 2014-08-29 19:23:27]

dalecurtis: audio stuff
xhwang: renderer stuff


I still think there are some content_browsertests timing out, but wanted to get
early feedback.

[DaleCurtis, 2014-08-29 20:17:23]

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_cloc...
media/filters/audio_clock.h:38: // Returns the latest media timestamp taking
silence and changes in
The difference between current and latest isn't really clear here. Can you write
some more docs?

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

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:600:
SetBufferingState_Locked(BUFFERING_HAVE_NOTHING);
Shouldn't you be accounting for the frames written last cycle here too?

[scherkus (not reviewing), 2014-08-29 20:32:03]

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

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:600:
SetBufferingState_Locked(BUFFERING_HAVE_NOTHING);
On 2014/08/29 20:17:23, DaleCurtis wrote:
> Shouldn't you be accounting for the frames written last cycle here too?

Not sure I follow.

What case are you thinking of?

[xhwang, 2014-08-29 20:45:48]

renderer_impl changes look good.

One question though: What's the long term plan to handle the annoying fact that
GetMediaTime() could be called on the render main thread, which requires us to
have locks in multiple places? I imagine we could let Renderer update media time
periodically and let Pipeline handle it's own interpolation and GetMediaTime()
implementation. In that case, will we still need TimeDeltaInterpolator?

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_cloc...
media/filters/audio_clock.h:39: // playback rate into account.
Only by looking at the comments, I have no idea what's the difference b/w the
"current" and the "latest" media timestamp...

[DaleCurtis, 2014-08-29 20:59:17]

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

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:600:
SetBufferingState_Locked(BUFFERING_HAVE_NOTHING);
On 2014/08/29 20:32:02, scherkus wrote:
> On 2014/08/29 20:17:23, DaleCurtis wrote:
> > Shouldn't you be accounting for the frames written last cycle here too?
> 
> Not sure I follow.
> 
> What case are you thinking of?

Audio still advances from the last write cycle despite underflow.  Do you want
to adjust time to compensate?  I don't think I follow why you're treating the
two cases differently.

[scherkus (not reviewing), 2014-08-29 21:29:00]

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

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:600:
SetBufferingState_Locked(BUFFERING_HAVE_NOTHING);
On 2014/08/29 20:59:17, DaleCurtis wrote:
> On 2014/08/29 20:32:02, scherkus wrote:
> > On 2014/08/29 20:17:23, DaleCurtis wrote:
> > > Shouldn't you be accounting for the frames written last cycle here too?
> > 
> > Not sure I follow.
> > 
> > What case are you thinking of?
> 
> Audio still advances from the last write cycle despite underflow.  Do you want
> to adjust time to compensate?  I don't think I follow why you're treating the
> two cases differently. 

The moment we enter underflow we don't want to advance time because we know we
have more decoded audio frames that will eventually arrive when we exit
underflow. If we advanced time at this point we would throw things off.

When we know we *won't* have more decoded frames arriving (end of stream), we
keep inserting silence to keep time advancing.

I hope I understood your question! Let me know if you want to chat offline.

[DaleCurtis, 2014-08-29 22:04:42]

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

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:600:
SetBufferingState_Locked(BUFFERING_HAVE_NOTHING);
On 2014/08/29 21:29:00, scherkus wrote:
> On 2014/08/29 20:59:17, DaleCurtis wrote:
> > On 2014/08/29 20:32:02, scherkus wrote:
> > > On 2014/08/29 20:17:23, DaleCurtis wrote:
> > > > Shouldn't you be accounting for the frames written last cycle here too?
> > > 
> > > Not sure I follow.
> > > 
> > > What case are you thinking of?
> > 
> > Audio still advances from the last write cycle despite underflow.  Do you
want
> > to adjust time to compensate?  I don't think I follow why you're treating
the
> > two cases differently. 
> 
> The moment we enter underflow we don't want to advance time because we know we
> have more decoded audio frames that will eventually arrive when we exit
> underflow. If we advanced time at this point we would throw things off.
> 
> When we know we *won't* have more decoded frames arriving (end of stream), we
> keep inserting silence to keep time advancing.
> 
> I hope I understood your question! Let me know if you want to chat offline.

Okay, this makes sense, but could do with a comment explaining why :)

[scherkus (not reviewing), 2014-08-30 00:24:12]

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_cloc...
media/filters/audio_clock.h:39: // playback rate into account.
On 2014/08/29 20:45:48, xhwang wrote:
> Only by looking at the comments, I have no idea what's the difference b/w the
> "current" and the "latest" media timestamp...

Ah I guess I used "latest" in the sense of "furthest away from now/current", but
I can see how they are synonyms.

Some ideas:
1) {front,back}_media_timestamp() (sorta matches STL)
2) current_media_timestamp() -> now() ? (sorta matches the "clock/time" aspect
of this class)
3) {earliest,latest}_media_timestamp() (like #1, except more time-based)

[xhwang, 2014-08-30 00:36:31]

On 2014/08/30 00:24:12, scherkus wrote:
>
https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_clock.h
> File media/filters/audio_clock.h (right):
> 
>
https://codereview.chromium.org/518613002/diff/20001/media/filters/audio_cloc...
> media/filters/audio_clock.h:39: // playback rate into account.
> On 2014/08/29 20:45:48, xhwang wrote:
> > Only by looking at the comments, I have no idea what's the difference b/w
the
> > "current" and the "latest" media timestamp...
> 
> Ah I guess I used "latest" in the sense of "furthest away from now/current",
but
> I can see how they are synonyms.
> 
> Some ideas:
> 1) {front,back}_media_timestamp() (sorta matches STL)
> 2) current_media_timestamp() -> now() ? (sorta matches the "clock/time" aspect
> of this class)
> 3) {earliest,latest}_media_timestamp() (like #1, except more time-based)

I guess naming isn't the problem if we can't find perfect names. Keeping the
current names or earliest/latest sgtm as long as you provide comments explaining
what they exactly represent.

[scherkus (not reviewing), 2014-09-02 21:26:22]

PTAL -- I renamed the API to front/back_timestamp()

[DaleCurtis, 2014-09-03 22:00:45]

lgtm % comment fixups.

https://codereview.chromium.org/518613002/diff/40001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/40001/media/filters/audio_cloc...
media/filters/audio_clock.h:34: //
Can you add the WroteAudio() calls that generated this structure? The comment
doesn't explain why the silent frames are not considered for the
back_timestamp().

[scherkus (not reviewing), 2014-09-03 22:23:18]

https://codereview.chromium.org/518613002/diff/40001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/40001/media/filters/audio_cloc...
media/filters/audio_clock.h:34: //
On 2014/09/03 22:00:45, DaleCurtis wrote:
> Can you add the WroteAudio() calls that generated this structure? The comment
> doesn't explain why the silent frames are not considered for the
> back_timestamp().

Done.

[xhwang, 2014-09-03 23:37:34]

renderer changes lgtm

I don't really understand this AudioClock though. Left a comment asking for more
comment :)

https://codereview.chromium.org/518613002/diff/60001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/60001/media/filters/audio_cloc...
media/filters/audio_clock.h:47: base::TimeDelta back_timestamp() const { return
back_timestamp_; }
Following dalecurtis's comment, it'd be nice to provide more detail (here or in
a class level) how/why silence is used in this class. It's not obvious to me
what 10 frames of silence means. Is playback rate applicable to silence? Can we
say 10 frames of silence equals to 250ms?

[scherkus (not reviewing), 2014-09-04 01:47:22]

https://codereview.chromium.org/518613002/diff/60001/media/filters/audio_clock.h
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/518613002/diff/60001/media/filters/audio_cloc...
media/filters/audio_clock.h:47: base::TimeDelta back_timestamp() const { return
back_timestamp_; }
On 2014/09/03 23:37:34, xhwang wrote:
> Following dalecurtis's comment, it'd be nice to provide more detail (here or
in
> a class level) how/why silence is used in this class. It's not obvious to me
> what 10 frames of silence means. Is playback rate applicable to silence? Can
we
> say 10 frames of silence equals to 250ms?

tried to beef up all the comments

for the sake of progress I might go ahead and CQ this -- I'm always more than
happy to tweak 'n update documentation!

[scherkus (not reviewing), 2014-09-04 06:22:12]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-04 06:23:38]

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

[commit-bot: I haz the power, 2014-09-04 08:00:03]

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

[commit-bot: I haz the power, 2014-09-04 08:00:03]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scherkus (not reviewing), 2014-09-04 19:51:58]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-04 19:57:43]

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

[scherkus (not reviewing), 2014-09-04 21:59:42]

Committed patchset #5 (id:100001) manually as af1afa5 (presubmit successful).

[commit-bot: I haz the power, 2014-09-10 03:32:53]

Patchset 5 (id:??) landed as
https://crrev.com/af1afa545ba0eecc6daf38be9d94073798f86848
Cr-Commit-Position: refs/heads/master@{#293343}