Update the current frame count in a safe place

Update the current frame count in a safe place

The main thread needs access to the current sample frame to determine
the sample time.  Instead of accessing it directly when the audio
thread is possibly updating it, the main thread will get a cached
value that is updated outside of when the audio thread actively
processing the graph.

No tests for this because the webaudio layout tests may not have real
audio hardware to run a real online realtime audio context. Manually ran the test from the bug report and the test passes.

BUG=431874
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186544

[Raymond Toy, 2014-12-03 18:11:01]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org, kbr@chromium.org

[Raymond Toy, 2014-12-03 18:11:02]

PTAL.

[hongchan, 2014-12-03 18:50:47]

lgtm

It is just a practice for me but - LGTM with one suggestion.

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.h:114: size_t sampleFrame() const;
Just a thought - can't this be shadowCurrentSampleFrame()?

I think the method name is a bit general for its behavior and having 'shadow' in
the method name clarifies it.

[Raymond Toy, 2014-12-03 19:04:44]

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.h:114: size_t sampleFrame() const;
On 2014/12/03 18:50:47, hoch wrote:
> Just a thought - can't this be shadowCurrentSampleFrame()?
> 
> I think the method name is a bit general for its behavior and having 'shadow'
in
> the method name clarifies it.

I'm terrible at names.  m_shadowCurrentSampleFrame isn't a great name, and I
think shadowCurrentSampleFrame() seems to reflect the implementation too much
because it's really meant to be the currentSampleFrame of the destination that
the main thread can use.

Maybe sampleFrame() should be called currentSampleFrame() and
currentSampleFrame() should be renamed to destinationCurrentSampleFrame() or
currentDestinationSampleFrame()?

[hongchan, 2014-12-03 19:13:29]

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.h:114: size_t sampleFrame() const;
On 2014/12/03 19:04:44, Raymond Toy wrote:
> On 2014/12/03 18:50:47, hoch wrote:
> > Just a thought - can't this be shadowCurrentSampleFrame()?
> > 
> > I think the method name is a bit general for its behavior and having
'shadow'
> in
> > the method name clarifies it.
> 
> I'm terrible at names.  m_shadowCurrentSampleFrame isn't a great name, and I
> think shadowCurrentSampleFrame() seems to reflect the implementation too much
> because it's really meant to be the currentSampleFrame of the destination that
> the main thread can use.
> 
> Maybe sampleFrame() should be called currentSampleFrame() and
> currentSampleFrame() should be renamed to destinationCurrentSampleFrame() or
> currentDestinationSampleFrame()?
>   

I agree that it is too much.

How about using the word 'cached' in the issue description instead? I think it
is a good name for the frame value introduced in this patch.

currentSampleFrame() -> m_currentSampleFrame (no change)
cachedSampleFrame() -> m_cachedSampleFrame

I believe the name 'cached' implies that it is behind the actual current sample
time and also thread-safe. Maybe a tad better than 'shadow' or 'destination.'

[Raymond Toy, 2014-12-03 19:34:21]

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/773273002/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioContext.h:114: size_t sampleFrame() const;
On 2014/12/03 19:13:29, hoch wrote:
> On 2014/12/03 19:04:44, Raymond Toy wrote:
> > On 2014/12/03 18:50:47, hoch wrote:
> > > Just a thought - can't this be shadowCurrentSampleFrame()?
> > > 
> > > I think the method name is a bit general for its behavior and having
> 'shadow'
> > in
> > > the method name clarifies it.
> > 
> > I'm terrible at names.  m_shadowCurrentSampleFrame isn't a great name, and I
> > think shadowCurrentSampleFrame() seems to reflect the implementation too
much
> > because it's really meant to be the currentSampleFrame of the destination
that
> > the main thread can use.
> > 
> > Maybe sampleFrame() should be called currentSampleFrame() and
> > currentSampleFrame() should be renamed to destinationCurrentSampleFrame() or
> > currentDestinationSampleFrame()?
> >   
> 
> I agree that it is too much.
> 
> How about using the word 'cached' in the issue description instead? I think it
> is a good name for the frame value introduced in this patch.
> 
> currentSampleFrame() -> m_currentSampleFrame (no change)
> cachedSampleFrame() -> m_cachedSampleFrame
> 
> I believe the name 'cached' implies that it is behind the actual current
sample
> time and also thread-safe. Maybe a tad better than 'shadow' or 'destination.'

Sounds good.

[Raymond Toy, 2014-12-03 19:42:12]

I think there's one more issue.  The context has a currentTime attribute.  This
is derived from the currentSampleFrame.  However, in the onaudioprocess event
for a ScriptProcessorNode, the playbackTime attribute is (now) derived from
cachedSampleFrame().  I think this would make it visible to the user that
context currentTime is not the same as the playbackTime.

We probably shouldn't make this difference visible. On the other hand,
ScriptProcessorNode is has already been deprecated in favor of the
AudioWorkerNode (which isn't yet implemented in Chrome), so maybe this issue can
be ignored?

WDYT?

[hongchan, 2014-12-03 22:24:40]

On 2014/12/03 19:42:12, Raymond Toy wrote:
> I think there's one more issue.  The context has a currentTime attribute. 
This
> is derived from the currentSampleFrame.  However, in the onaudioprocess event
> for a ScriptProcessorNode, the playbackTime attribute is (now) derived from
> cachedSampleFrame().  I think this would make it visible to the user that
> context currentTime is not the same as the playbackTime.
> 
> We probably shouldn't make this difference visible. On the other hand,
> ScriptProcessorNode is has already been deprecated in favor of the
> AudioWorkerNode (which isn't yet implemented in Chrome), so maybe this issue
can
> be ignored?
> 
> WDYT?

We agreed that the inconsistency in timestamps can be ignored. Not only this
node will be deprecated in the near future, but also there is no perfect way of
synchronizing two time stamps. This is the simplest implementation we can have
currently.

[Raymond Toy, 2014-12-04 16:48:08]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2014-12-04 16:49:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773273002/20001

[commit-bot: I haz the power, 2014-12-04 16:49:14]

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

[commit-bot: I haz the power, 2014-12-04 16:49:15]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ken Russell (switch to Gerrit), 2014-12-04 18:33:47]

lgtm

[Raymond Toy, 2014-12-04 18:53:03]

On 2014/12/04 18:33:47, Ken Russell wrote:
> lgtm

Thanks!

[Raymond Toy, 2014-12-04 18:53:19]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2014-12-04 18:53:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773273002/20001

[commit-bot: I haz the power, 2014-12-04 19:47:47]

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

[commit-bot: I haz the power, 2014-12-04 19:47:48]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[Raymond Toy, 2014-12-04 21:04:31]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2014-12-04 21:04:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773273002/40001

[commit-bot: I haz the power, 2014-12-04 22:13:32]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186544