media::SilentSinkSuspender should simulate |delay| and |delay_timestamp|

media::SilentSinkSuspender should simulate |delay| and |delay_timestamp|

This patch provides estimation of the |delay| and |delay_timestamp| values for the wrapped |callback_|, otherwise the implementation of the 'AudioContext.getOutputTimestamp()' [1] returns inappropriate values when the "fake sink" is used.

BUG=619533
[1] https://codereview.chromium.org/2060833002/

Review-Url: https://codereview.chromium.org/2567143002
Cr-Commit-Position: refs/heads/master@{#442536}
Committed: https://chromium.googlesource.com/chromium/src/+/fa73a9762b712c779a7eda292e3e32f08d2d42c5

[Mikhail, 2016-12-12 21:19:28]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 21:20:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2016-12-12 21:21:58]

mikhail.pozdnyakov@intel.com changed reviewers:
+ chcunningham@chromium.org

[Mikhail, 2016-12-12 21:21:58]

Please take a look.

[commit-bot: I haz the power, 2016-12-12 21:39:52]

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

[commit-bot: I haz the power, 2016-12-12 21:39:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Mikhail, 2016-12-13 09:24:26]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 09:24:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 11:02:26]

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

[commit-bot: I haz the power, 2016-12-13 11:02:27]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-12-13 16:14:21]

Patchset #1 (id:1) has been deleted

[chcunningham, 2016-12-14 01:01:44]

Can your provide some examples of how things go wrong for the math in
getOutputTimestamp? I'm not sure why it isn't working as-is, and I'm skeptical
of faking it in the fake sink vs fixing the math in getOutputTimestamp. My
intuition is that it should be possible (even ideal) for sink's to render things
as close to instantaneously as possible. The fake sink essentially does this (by
doing nothing, and reporting 0 delay at time now()).

[Mikhail, 2016-12-15 14:39:16]

On 2016/12/14 01:01:44, chcunningham wrote:
> Can your provide some examples of how things go wrong for the math in
> getOutputTimestamp? I'm not sure why it isn't working as-is, and I'm skeptical
> of faking it in the fake sink vs fixing the math in getOutputTimestamp. My
> intuition is that it should be possible (even ideal) for sink's to render
things
> as close to instantaneously as possible. The fake sink essentially does this
(by
> doing nothing, and reporting 0 delay at time now()).

Thanks for having a look! The problems are:
1) the estimations made from the 'getOutputTimestamp()' values will be wrong, if
this method is called with "fake sink".
(on the other hand it might be OK since can happen after 30 sec of silence..
wdyt?)
2) timestamp gets stuck at the value obtained at
SilentSinkSuspender::TransitionSinks() and never progresses.

So, if we consider 1) is tolerable, than 2) can be fixed with a simpler
solution: just call "delay_timestamp = base::TimeTicks::Now();" each time at 
SilentSinkSuspender::Render()

[chcunningham, 2016-12-15 23:17:26]

> On 2016/12/14 01:01:44, chcunningham wrote:
> > Can your provide some examples of how things go wrong for the math in
> > getOutputTimestamp? I'm not sure why it isn't working as-is, and I'm
skeptical
> > of faking it in the fake sink vs fixing the math in getOutputTimestamp. My
> > intuition is that it should be possible (even ideal) for sink's to render
> things
> > as close to instantaneously as possible. The fake sink essentially does this
> (by
> > doing nothing, and reporting 0 delay at time now()).
> 
> Thanks for having a look! The problems are:
> 1) the estimations made from the 'getOutputTimestamp()' values will be wrong,
if
> this method is called with "fake sink".
> (on the other hand it might be OK since can happen after 30 sec of silence..
> wdyt?)

This is the part I'm confused about. Can you provide some example of pre/post
transition values and make the argument that the post-transition values are
wrong?

As I see it, the performance time pre-transition will be slightly in the future
(due to real output delay), but post transition, the performance times will be
"now" (or just a moment ago), which seems intuitively correct due to no delay.
What am I missing?

> 2) timestamp gets stuck at the value obtained at
> SilentSinkSuspender::TransitionSinks() and never progresses.
> 
> So, if we consider 1) is tolerable, than 2) can be fixed with a simpler
> solution: just call "delay_timestamp = base::TimeTicks::Now();" each time at 
> SilentSinkSuspender::Render()

IIUC, this is because we store the value of now() in the bind here. Yeah, this
looks like a bug. We should evaluate now at every render callback.
https://cs.chromium.org/chromium/src/media/base/silent_sink_suspender.cc?rcl=...

[chcunningham, 2016-12-15 23:19:29]

I should clarify...

> This is the part I'm confused about. Can you provide some example of pre/post
> transition values and make the argument that the post-transition values are
> wrong?
> 
> As I see it, the performance time pre-transition will be slightly in the
future
> (due to real output delay), but post transition, the performance times will be
> "now" (or just a moment ago), which seems intuitively correct due to no delay.
> What am I missing?


The values are obviously wrong because of the frozen time from
TransitionSinks(). But assuming we fix that bug, would you still consider always
using timestamp=Now() and delay=0 to be wrong?

[Mikhail, 2016-12-16 14:02:37]

On 2016/12/15 23:17:26, chcunningham wrote:
> > On 2016/12/14 01:01:44, chcunningham wrote:
> > > Can your provide some examples of how things go wrong for the math in
> > > getOutputTimestamp? I'm not sure why it isn't working as-is, and I'm
> skeptical
> > > of faking it in the fake sink vs fixing the math in getOutputTimestamp. My
> > > intuition is that it should be possible (even ideal) for sink's to render
> > things
> > > as close to instantaneously as possible. The fake sink essentially does
this
> > (by
> > > doing nothing, and reporting 0 delay at time now()).
> > 
> > Thanks for having a look! The problems are:
> > 1) the estimations made from the 'getOutputTimestamp()' values will be
wrong,
> if
> > this method is called with "fake sink".
> > (on the other hand it might be OK since can happen after 30 sec of silence..
> > wdyt?)
> 
> This is the part I'm confused about. Can you provide some example of pre/post
> transition values and make the argument that the post-transition values are
> wrong?
> 
> As I see it, the performance time pre-transition will be slightly in the
future
> (due to real output delay), but post transition, the performance times will be
> "now" (or just a moment ago), which seems intuitively correct due to no delay.
> What am I missing?
> 

The intended use of 'getOutputTimestamp' API is making of a liner approximation
(pls see example section at
https://webaudio.github.io/web-audio-api/#widl-AudioContext-getOutputTimestam...).

With zero delay "fake sink" will show a different correllation between two
timestamps and thus will give wrong timestamp estiamtions (i.e. with
significantly lower accuracy) for the periods when real output device is active
again (or was active before).

The proposed patch is doing the same linear approximation inside
SilentSinkSuspender based on the latest data obtained from the real output
device, so that switching to fake sink and back is transparent to the user.

[chcunningham, 2016-12-16 20:05:22]

On 2016/12/16 14:02:37, Mikhail wrote:
> On 2016/12/15 23:17:26, chcunningham wrote:
> > > On 2016/12/14 01:01:44, chcunningham wrote:
> > > > Can your provide some examples of how things go wrong for the math in
> > > > getOutputTimestamp? I'm not sure why it isn't working as-is, and I'm
> > skeptical
> > > > of faking it in the fake sink vs fixing the math in getOutputTimestamp.
My
> > > > intuition is that it should be possible (even ideal) for sink's to
render
> > > things
> > > > as close to instantaneously as possible. The fake sink essentially does
> this
> > > (by
> > > > doing nothing, and reporting 0 delay at time now()).
> > > 
> > > Thanks for having a look! The problems are:
> > > 1) the estimations made from the 'getOutputTimestamp()' values will be
> wrong,
> > if
> > > this method is called with "fake sink".
> > > (on the other hand it might be OK since can happen after 30 sec of
silence..
> > > wdyt?)
> > 
> > This is the part I'm confused about. Can you provide some example of
pre/post
> > transition values and make the argument that the post-transition values are
> > wrong?
> > 
> > As I see it, the performance time pre-transition will be slightly in the
> future
> > (due to real output delay), but post transition, the performance times will
be
> > "now" (or just a moment ago), which seems intuitively correct due to no
delay.
> > What am I missing?
> > 
> 
> The intended use of 'getOutputTimestamp' API is making of a liner
approximation
> (pls see example section at
>
https://webaudio.github.io/web-audio-api/#widl-AudioContext-getOutputTimestam...).
> 
> With zero delay "fake sink" will show a different correllation between two
> timestamps and thus will give wrong timestamp estiamtions (i.e. with
> significantly lower accuracy) for the periods when real output device is
active
> again (or was active before).
> 
> The proposed patch is doing the same linear approximation inside
> SilentSinkSuspender based on the latest data obtained from the real output
> device, so that switching to fake sink and back is transparent to the user.

Thanks, that example helped me understand. I follow that the estimation will
definitely change as the delay shrinks to zero. IIUC, whether that estimation is
right or wrong really comes down to whether the audio continues to be silent at
the piont they're estimating for. If silent (and continuously silent up to that
point), I would argue the estimate is correct (delay continues to be zero
throughout) - and that faking the delay as in this CL would make the estimate
worse. If silence stops at some point before the contextTime they're interested
in, then the estimate will be off, but IIUC, it is only off by the delay. Either
way, the estimate *may* be off, but in both cases the amount its off seems
pretty minor IMHO. Can you confirm my understanding? Do you feel its better to
favor non-silent vs continuously silent accuracy tradeoff?

[Mikhail, 2016-12-16 22:50:43]

On 2016/12/16 20:05:22, chcunningham wrote:
> 
> Thanks, that example helped me understand. I follow that the estimation will
> definitely change as the delay shrinks to zero. IIUC, whether that estimation
is
> right or wrong really comes down to whether the audio continues to be silent
at
> the piont they're estimating for. If silent (and continuously silent up to
that
> point), I would argue the estimate is correct (delay continues to be zero
> throughout) - and that faking the delay as in this CL would make the estimate
> worse. If silence stops at some point before the contextTime they're
interested
> in, then the estimate will be off, but IIUC, it is only off by the delay.
Either
> way, the estimate *may* be off, but in both cases the amount its off seems
> pretty minor IMHO. Can you confirm my understanding? Do you feel its better to
> favor non-silent vs continuously silent accuracy tradeoff?

I definitely agree with you, it indeed depends on the estimation point. 
However, "fake sink" is in action when there is no actual output, so an
estimation error there most probably won't be noticed. I'm in favor of being
aligned with non-silent delay, as 1) it will help to sync with some "real"
output 2) 'getOutputTimestamp' result will be consistent all the time (otherwise
it will look like the output audio device has changed (which is actually the
case, as it changes to "fake sink" even though user is not aware of that..)).

[Mikhail, 2016-12-19 19:06:17]

chcunningham@, which approach would you prefer?

[Mikhail, 2016-12-20 16:51:51]

sandersd@, maybe you could take a look at this CL (chcunningham@ is OOO for a
few days)? thanks

[chcunningham, 2016-12-20 18:36:43]

This generally looks good - but I'll appreciate getting sanders second opinion. 

On the one hand I understand the motivations and how it fixes things, but on the
other hand it adds complexity to a class to solve some higher layers problem... 

Without the faked delay the inaccuracy is likely to not be huge (not enough to
notice AV sync issues). If we can contain this to just SilentSinkSuspender then
I'm ok, but if we start faking delays elsewhere I'll push for just allowing the
inaccuracy.

https://codereview.chromium.org/2567143002/diff/20001/media/base/fake_audio_r...
File media/base/fake_audio_render_callback.cc (right):

https://codereview.chromium.org/2567143002/diff/20001/media/base/fake_audio_r...
media/base/fake_audio_render_callback.cc:37: constexpr int kSampleRate = 48000;
I think this should be defined and passed in (to constructor) from the tests.
You currently have it defined twice - here and in the unit test.

https://codereview.chromium.org/2567143002/diff/20001/media/base/silent_sink_...
File media/base/silent_sink_suspender.cc (right):

https://codereview.chromium.org/2567143002/diff/20001/media/base/silent_sink_...
media/base/silent_sink_suspender.cc:63: delay_timestamp += elapsedTime;
Could be a little simpler.

// Same comment...
DCHECK_EQ(delay_timestamp, fake_sink_transition_time_);
delay_timestamp = base::TimeTicks::Now()

[Mikhail, 2016-12-20 21:41:21]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 21:41:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2016-12-20 21:43:38]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[Mikhail, 2016-12-20 21:43:58]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-12-20 21:44:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2016-12-20 21:45:51]

https://codereview.chromium.org/2567143002/diff/20001/media/base/fake_audio_r...
File media/base/fake_audio_render_callback.cc (right):

https://codereview.chromium.org/2567143002/diff/20001/media/base/fake_audio_r...
media/base/fake_audio_render_callback.cc:37: constexpr int kSampleRate = 48000;
On 2016/12/20 18:36:42, chcunningham_ooo_until_jan_1 wrote:
> I think this should be defined and passed in (to constructor) from the tests.
> You currently have it defined twice - here and in the unit test. 

Done.

https://codereview.chromium.org/2567143002/diff/20001/media/base/silent_sink_...
File media/base/silent_sink_suspender.cc (right):

https://codereview.chromium.org/2567143002/diff/20001/media/base/silent_sink_...
media/base/silent_sink_suspender.cc:63: delay_timestamp += elapsedTime;
On 2016/12/20 18:36:43, chcunningham_ooo_until_jan_1 wrote:
> Could be a little simpler.
> 
> // Same comment...
> DCHECK_EQ(delay_timestamp, fake_sink_transition_time_);
> delay_timestamp = base::TimeTicks::Now()

sorry, I made a mistake in the comment :( it caches actually
|latest_output_delay_timestamp_|

[commit-bot: I haz the power, 2016-12-20 21:58:14]

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

[commit-bot: I haz the power, 2016-12-20 21:58:19]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sandersd (OOO until July 31), 2016-12-21 00:49:05]

> I'll appreciate getting sanders second opinion.

I don't know sinks well enough to be able to offer advice here. This CL by
itself looks fine, but I would need someone to fully explain the context to me
before I could comment on the overall design philosophy.

Sorry!

[chcunningham, 2016-12-21 07:02:54]

chcunningham@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[chcunningham, 2016-12-21 07:02:54]

remove: sandersd@
add: dalecurtis@

https://codereview.chromium.org/2567143002/diff/20001/media/base/silent_sink_...
File media/base/silent_sink_suspender.cc (right):

https://codereview.chromium.org/2567143002/diff/20001/media/base/silent_sink_...
media/base/silent_sink_suspender.cc:63: delay_timestamp += elapsedTime;
On 2016/12/20 21:45:50, Mikhail wrote:
> On 2016/12/20 18:36:43, chcunningham_ooo_until_jan_1 wrote:
> > Could be a little simpler.
> > 
> > // Same comment...
> > DCHECK_EQ(delay_timestamp, fake_sink_transition_time_);
> > delay_timestamp = base::TimeTicks::Now()
> 
> sorry, I made a mistake in the comment :( it caches actually
> |latest_output_delay_timestamp_|

No worries. But the core of my comment is about removing the elapsedTime
calculation. Isn't your math equivalent to just setting delay_timestamp =
base::TimeTicks::Now()

[Mikhail, 2016-12-21 08:07:41]

On 2016/12/21 07:02:54, chcunningham_ooo_until_jan_1 wrote:
> remove: sandersd@
> add: dalecurtis@
> 
> No worries. But the core of my comment is about removing the elapsedTime
> calculation. Isn't your math equivalent to just setting delay_timestamp =
> base::TimeTicks::Now()

There is a difference, let me try to explain (and sorry for not being clear
enough!):
With "normal" sink, i.e. the actual audio output device, on browser side the
{delay, delay_timestamp} pair is fetched at the moment when the current buffer
has been send for playing and renderer process is required to fulfill the buffer
with fresh data. In renderer process, WebAudio implementation is watching the
amount of samples it has fulfilled so far (let's call it 'current audio
position'), so it can estimate the '*output* position' at 'delay_timestamp' as
('current audio position in seconds' - 'delay'), this calculation is accurate
only for the 'delay_timestamp' moment of time. For the later moment of time
'output position' will be calculated as 'later output position' = 'output
position' + ('later time' - delay_timestamp). 

So, it is important that both values at {delay, delay_timestamp} pair correspond
to the moment when the current buffer is elapsed. Regarding SilentSinkSuspender,
in this CL I made a linear approximation for the 'delay_timestamp' values
(delay_timestamp = 'latest value from platfrom' + 'elapsed time') in order to
keep the correlation between all three engaged components: 'delay', 'delay
timestamp' and 'current audio position'. Using just base::TimeTicks::Now() would
affect the resulting {output position, output position timestamp } pair returned
by 'AudioContext.getOutputTimestamp()'.

[Mikhail, 2016-12-21 08:30:32]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 08:30:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 08:41:51]

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

[commit-bot: I haz the power, 2016-12-21 08:41:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[chcunningham, 2017-01-04 00:14:28]

On 2016/12/21 08:07:41, Mikhail OOO till Jan 9 wrote:
> On 2016/12/21 07:02:54, chcunningham_ooo_until_jan_1 wrote:
> > remove: sandersd@
> > add: dalecurtis@
> > 
> > No worries. But the core of my comment is about removing the elapsedTime
> > calculation. Isn't your math equivalent to just setting delay_timestamp =
> > base::TimeTicks::Now()
> 
> There is a difference, let me try to explain (and sorry for not being clear
> enough!):
> With "normal" sink, i.e. the actual audio output device, on browser side the
> {delay, delay_timestamp} pair is fetched at the moment when the current buffer
> has been send for playing and renderer process is required to fulfill the
buffer
> with fresh data. In renderer process, WebAudio implementation is watching the
> amount of samples it has fulfilled so far (let's call it 'current audio
> position'), so it can estimate the '*output* position' at 'delay_timestamp' as
> ('current audio position in seconds' - 'delay'), this calculation is accurate
> only for the 'delay_timestamp' moment of time. For the later moment of time
> 'output position' will be calculated as 'later output position' = 'output
> position' + ('later time' - delay_timestamp). 
> 
> So, it is important that both values at {delay, delay_timestamp} pair
correspond
> to the moment when the current buffer is elapsed. Regarding
SilentSinkSuspender,
> in this CL I made a linear approximation for the 'delay_timestamp' values
> (delay_timestamp = 'latest value from platfrom' + 'elapsed time') in order to
> keep the correlation between all three engaged components: 'delay', 'delay
> timestamp' and 'current audio position'. Using just base::TimeTicks::Now()
would
> affect the resulting {output position, output position timestamp } pair
returned
> by 'AudioContext.getOutputTimestamp()'.


Ah, yeah you're right. My proposal was not equivalent. 

Lets still wait for input from Dale... he gets back in office a few days before
you :)

[DaleCurtis, 2017-01-05 22:44:54]

On 2017/01/04 at 00:14:28, chcunningham wrote:
> On 2016/12/21 08:07:41, Mikhail OOO till Jan 9 wrote:
> > On 2016/12/21 07:02:54, chcunningham_ooo_until_jan_1 wrote:
> > > remove: sandersd@
> > > add: dalecurtis@
> > > 
> > > No worries. But the core of my comment is about removing the elapsedTime
> > > calculation. Isn't your math equivalent to just setting delay_timestamp =
> > > base::TimeTicks::Now()
> > 
> > There is a difference, let me try to explain (and sorry for not being clear
> > enough!):
> > With "normal" sink, i.e. the actual audio output device, on browser side the
> > {delay, delay_timestamp} pair is fetched at the moment when the current
buffer
> > has been send for playing and renderer process is required to fulfill the
buffer
> > with fresh data. In renderer process, WebAudio implementation is watching
the
> > amount of samples it has fulfilled so far (let's call it 'current audio
> > position'), so it can estimate the '*output* position' at 'delay_timestamp'
as
> > ('current audio position in seconds' - 'delay'), this calculation is
accurate
> > only for the 'delay_timestamp' moment of time. For the later moment of time
> > 'output position' will be calculated as 'later output position' = 'output
> > position' + ('later time' - delay_timestamp). 
> > 
> > So, it is important that both values at {delay, delay_timestamp} pair
correspond
> > to the moment when the current buffer is elapsed. Regarding
SilentSinkSuspender,
> > in this CL I made a linear approximation for the 'delay_timestamp' values
> > (delay_timestamp = 'latest value from platfrom' + 'elapsed time') in order
to
> > keep the correlation between all three engaged components: 'delay', 'delay
> > timestamp' and 'current audio position'. Using just base::TimeTicks::Now()
would
> > affect the resulting {output position, output position timestamp } pair
returned
> > by 'AudioContext.getOutputTimestamp()'.
> 
> 
> Ah, yeah you're right. My proposal was not equivalent. 
> 
> Lets still wait for input from Dale... he gets back in office a few days
before you :)

Hmm, I'm not clear on what this is doing. It seems like delay_timestamp will
grow indefinitely in the fake sink case now? I don't understand why that would
be correct. I would expect it to taper to zero, but I may be misremembering how
this is supposed to work.

[chcunningham, 2017-01-05 22:55:44]

On 2017/01/05 22:44:54, DaleCurtis wrote:
> On 2017/01/04 at 00:14:28, chcunningham wrote:
> > On 2016/12/21 08:07:41, Mikhail OOO till Jan 9 wrote:
> > > On 2016/12/21 07:02:54, chcunningham_ooo_until_jan_1 wrote:
> > > > remove: sandersd@
> > > > add: dalecurtis@
> > > > 
> > > > No worries. But the core of my comment is about removing the elapsedTime
> > > > calculation. Isn't your math equivalent to just setting delay_timestamp
=
> > > > base::TimeTicks::Now()
> > > 
> > > There is a difference, let me try to explain (and sorry for not being
clear
> > > enough!):
> > > With "normal" sink, i.e. the actual audio output device, on browser side
the
> > > {delay, delay_timestamp} pair is fetched at the moment when the current
> buffer
> > > has been send for playing and renderer process is required to fulfill the
> buffer
> > > with fresh data. In renderer process, WebAudio implementation is watching
> the
> > > amount of samples it has fulfilled so far (let's call it 'current audio
> > > position'), so it can estimate the '*output* position' at
'delay_timestamp'
> as
> > > ('current audio position in seconds' - 'delay'), this calculation is
> accurate
> > > only for the 'delay_timestamp' moment of time. For the later moment of
time
> > > 'output position' will be calculated as 'later output position' = 'output
> > > position' + ('later time' - delay_timestamp). 
> > > 
> > > So, it is important that both values at {delay, delay_timestamp} pair
> correspond
> > > to the moment when the current buffer is elapsed. Regarding
> SilentSinkSuspender,
> > > in this CL I made a linear approximation for the 'delay_timestamp' values
> > > (delay_timestamp = 'latest value from platfrom' + 'elapsed time') in order
> to
> > > keep the correlation between all three engaged components: 'delay', 'delay
> > > timestamp' and 'current audio position'. Using just base::TimeTicks::Now()
> would
> > > affect the resulting {output position, output position timestamp } pair
> returned
> > > by 'AudioContext.getOutputTimestamp()'.
> > 
> > 
> > Ah, yeah you're right. My proposal was not equivalent. 
> > 
> > Lets still wait for input from Dale... he gets back in office a few days
> before you :)
> 
> Hmm, I'm not clear on what this is doing. It seems like delay_timestamp will
> grow indefinitely in the fake sink case now? I don't understand why that would
> be correct. I would expect it to taper to zero, but I may be misremembering
how
> this is supposed to work.

delay_timestamp is the time at which delay was measured. 

> It seems like delay_timestamp will grow indefinitely in the fake sink case
now?

I think that is desired assuming you're on board with the big picture strategy
here. 

The delay timestamp is the time at which delay was measured. The fake sink will
report an increased delay timestamp with every render and re-use the delay value
last reported from the real sink, simulating what would happen with a real sink.
In reality the fake sink has zero delay, but using zero messes up the math for
users of the web audio api (see example:
https://webaudio.github.io/web-audio-api/#widl-AudioContext-getOutputTimestam...).

The big question for me is whether its worth this additional complexity to patch
up that downstream API. I could go either way.

[DaleCurtis, 2017-01-05 23:07:23]

OIC, this seems okay to me then. This code is only used for WebAudio at present.
Probably the WebAudio code should be fixed to handle delay transitions though
too. I don't know if it's guaranteed that value will stay constant. I.e.
consider device changes on the browser side.

https://codereview.chromium.org/2567143002/diff/80001/media/base/silent_sink_...
File media/base/silent_sink_suspender.cc (right):

https://codereview.chromium.org/2567143002/diff/80001/media/base/silent_sink_...
media/base/silent_sink_suspender.cc:57: // |delay_timestamp| contains the value
cached at
Reflow comment.

[chcunningham, 2017-01-05 23:35:01]

On 2017/01/05 23:07:23, DaleCurtis wrote:
> OIC, this seems okay to me then. 

And with that, LGTM :)

> Probably the WebAudio code should be fixed to handle delay transitions though
> too. I don't know if it's guaranteed that value will stay constant. I.e.
> consider device changes on the browser side.

They give a little warning about this in the example: "... accuracy of the
estimation depends on how close the argument value is to the current output
audio stream position: the closer the given contextTime is to
timestamp.contextTime, the better the accuracy of the obtained estimation."

[Mikhail, 2017-01-09 13:48:55]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 13:49:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 14:04:01]

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

[commit-bot: I haz the power, 2017-01-09 14:04:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mikhail, 2017-01-09 16:35:55]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 16:36:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 16:49:55]

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

[commit-bot: I haz the power, 2017-01-09 16:49:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mikhail, 2017-01-09 17:00:48]

The CQ bit was checked by mikhail.pozdnyakov@intel.com to run a CQ dry run

[Mikhail, 2017-01-09 17:01:14]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-01-09 17:01:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2017-01-09 18:21:07]

mikhail.pozdnyakov@intel.com changed reviewers:
+ miu@chromium.org

[Mikhail, 2017-01-09 18:21:07]

miu@chromium.org: Please review changes in 

content/renderer/media

[commit-bot: I haz the power, 2017-01-09 18:35:52]

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

[commit-bot: I haz the power, 2017-01-09 18:35:53]

Dry run: This issue passed the CQ dry run.

[miu, 2017-01-10 00:55:07]

content/renderer/media lgtm

[Mikhail, 2017-01-10 09:10:20]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[Mikhail, 2017-01-10 09:10:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org
Link to the patchset: https://codereview.chromium.org/2567143002/#ps120001
(title: "build fix content_unittets & media_blink_unittsests")

[commit-bot: I haz the power, 2017-01-10 09:10:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 09:14:18]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484039420128970,
"parent_rev": "0be63c49d90c37d2e7de8b814589b7d90e3548ea", "commit_rev":
"fa73a9762b712c779a7eda292e3e32f08d2d42c5"}

[commit-bot: I haz the power, 2017-01-10 09:14:46]

Description was changed from

==========
media::SilentSinkSuspender should simulate |delay| and |delay_timestamp|

This patch provides estimation of the |delay| and |delay_timestamp| values for
the wrapped |callback_|, otherwise the implementation of the
'AudioContext.getOutputTimestamp()' [1] returns inappropriate values when the
"fake sink" is used.

BUG=619533

[1] https://codereview.chromium.org/2060833002/
==========

to

==========
media::SilentSinkSuspender should simulate |delay| and |delay_timestamp|

This patch provides estimation of the |delay| and |delay_timestamp| values for
the wrapped |callback_|, otherwise the implementation of the
'AudioContext.getOutputTimestamp()' [1] returns inappropriate values when the
"fake sink" is used.

BUG=619533

[1] https://codereview.chromium.org/2060833002/

Review-Url: https://codereview.chromium.org/2567143002
Cr-Commit-Position: refs/heads/master@{#442536}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa73a9762b712c779a7eda292e3e...
==========

[commit-bot: I haz the power, 2017-01-10 09:14:48]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/fa73a9762b712c779a7eda292e3e...