Defer changes to channelInterpretation to pre/post rendering.

Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update to the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none

Committed: https://crrev.com/dd2bffb1f8812c26400a2aca38f1eb9c1fced985
Cr-Commit-Position: refs/heads/master@{#396317}

[Raymond Toy, 2016-05-26 20:47:49]

Description was changed from

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615903, 613332
TEST=none
==========

to

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none
==========

[Raymond Toy, 2016-05-26 20:48:20]

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

[Raymond Toy, 2016-05-26 20:48:21]

PTAL

[hongchan, 2016-05-26 21:00:57]

lgtm - does this resolve associated bugs? (perhaps we want to dry run with tsan
bots?

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (left):

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:238: #endif
What is the meaning of this? Perhaps there was a time that ASSERT was used
rarely?

[Raymond Toy, 2016-05-26 21:06:59]

I tested with a local tsan build that the bug in 615093 is actually fixed.

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (left):

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:238: #endif
On 2016/05/26 21:00:57, hoch wrote:
> What is the meaning of this? Perhaps there was a time that ASSERT was used
> rarely?

I discovered this just now.  Previously ASSERT(...) would generate no code if
asserts were disabled.  But DCHECK always emits something (so variables in
DCHECK are used), so now we need to have isGraphOwner defined so that DCHECK is
valid.

[hongchan, 2016-05-26 21:09:37]

On 2016/05/26 21:06:59, Raymond Toy wrote:
> I tested with a local tsan build that the bug in 615093 is actually fixed.
> 
>
https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (left):
> 
>
https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:238: #endif
> On 2016/05/26 21:00:57, hoch wrote:
> > What is the meaning of this? Perhaps there was a time that ASSERT was used
> > rarely?
> 
> I discovered this just now.  Previously ASSERT(...) would generate no code if
> asserts were disabled.  But DCHECK always emits something (so variables in
> DCHECK are used), so now we need to have isGraphOwner defined so that DCHECK
is
> valid.

Nice! Great work here!

[Raymond Toy, 2016-05-26 22:06:25]

Description was changed from

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none
==========

to

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update to the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none
==========

[Raymond Toy, 2016-05-26 23:03:28]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2016-05-26 23:03:56]

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

[commit-bot: I haz the power, 2016-05-26 23:34:27]

Description was changed from

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update to the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none
==========

to

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update to the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none
==========

[commit-bot: I haz the power, 2016-05-26 23:34:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-26 23:36:54]

Description was changed from

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update to the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none
==========

to

==========
Defer changes to channelInterpretation to pre/post rendering.

If the main thread changes the channel interpretation, record the
change and do the update to the channel interpretation in the pre and/or
post rendering phase in the deferred task handler.

This removes the race in setting the interpretation in the main thread
while also reading it in the audio thread which uses the
interpretation to determine how to sum a bunch of connections
together.

BUG=615093, 613332
TEST=none

Committed: https://crrev.com/dd2bffb1f8812c26400a2aca38f1eb9c1fced985
Cr-Commit-Position: refs/heads/master@{#396317}
==========

[commit-bot: I haz the power, 2016-05-26 23:36:55]

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

[Nico, 2016-05-27 13:09:32]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-27 13:09:33]

This broke release component builds.

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h (left):

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.h:238: #endif
On 2016/05/26 21:06:58, Raymond Toy wrote:
> On 2016/05/26 21:00:57, hoch wrote:
> > What is the meaning of this? Perhaps there was a time that ASSERT was used
> > rarely?
> 
> I discovered this just now.  Previously ASSERT(...) would generate no code if
> asserts were disabled.  But DCHECK always emits something (so variables in
> DCHECK are used), so now we need to have isGraphOwner defined so that DCHECK
is
> valid.

That doesn't sound right. DCHECK() compiles to nothing if dchecks are not
enabled.

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h (left):

https://codereview.chromium.org/2014343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h:120: #if
ENABLE(ASSERT)
You removed this here but not in the cc file. So now the function is declared
but not defined in non-assert builds. Things generally build since nothing calls
this in non-assert builds -- but in release component builds the whole class is
exported and hence clients try to import this method and links fail
(https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dll%29/build...)

Rather than remove the ENABLE(ASSERT) in the cc file, I think it's better to add
it back here -- since things build, nothing needs this to be defined. And I
don't understand why you had to remove the #if in the other file.

[Nico, 2016-05-27 13:11:43]

I'll try to fix. The CQ will probably let me know why you removed the #if in the
first file :-)

[Nico, 2016-05-27 13:11:53]

(fix wip at  https://codereview.chromium.org/2017923002 )