Bug 76685 – Fix ASSERT fail within AudioBus::processWithGainFrom()

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond]

There are ASSERT Fail within AudioBus::processWithGainFrom when DelayNode connected to GainNode. If you try to load http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html  and then refresh it by F5, you will probably encounter this issue.

I spent a lot of time to investigate this issue, finnal I found this is actually related to a AudioBasicProcessorNode and GainNode.

When an node is connect to another node,  the input will be marked to dirty and thus the node who owns the input will be checked with checkNumberOfChannelsForInput (And the renderingOutput will be updated)
And GainNode use checkNumberOfChannelsForInput to reallocate it's output bus according to the input bus's channel count. And thus when it do pull, it will assume it's output bus's topology should be the same as it's input bus, which in the single connection case, will probably be its upstream node's outputbus's toplogy.

Usually, this is true. While for the case of AudioBasicProcessorNode , its output bus is set to 0 channel by default and will not be set to the correct value until itself is connected by an upstream node, usually this is also ok if you check it.

But then, the AudioNodeInput's numberOfChannels() will force 0 to 1 as the mini channel. So when delay node is connected to GainNode while itself not yet been connected to, it breaks AudioGainNode's assumption. And then lead to this issue. Since it depends on master thread and audio thread's running sequence, it happens randomly.

The fix to this issue, is either change AudioNodeInput's numberOfChannels() to return 0 when it do have connection but the connection's channel is really a 0, or set AudioBasicProcessorNode's default output bus's channel to 1. The later one seems more easy to solve this issue at present and seems to me not bring extra problems. The first approaching might influence other codes and in order to make it work fine, some logic might need to be taken care of. but I really do think it will help to reduce this kind of unsync issues in the future?

While anyway, I will upload the patch using the second approaching.

[Raymond]

Created attachment 123254 [details]
Patch

[Daniel Bates]

Comment on attachment 123254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123254&action=review

> Source/WebCore/ChangeLog:3
> +        Fix ASSERT Fail within AudioBus::processWithGainFrom

Nit: Fail => fail

> Source/WebCore/ChangeLog:8
> +        No new tests required.

Can you elaborate on why we can't write a test for this issue? The description of this bug (comment #0) states that we may be able to use the page <http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html> to exhibit this assertion failure. Can we derive a test case from this page?

[Raymond]

(In reply to comment #2)
> (From update of attachment 123254 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123254&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Fix ASSERT Fail within AudioBus::processWithGainFrom
> 
> Nit: Fail => fail
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests required.
> 
> Can you elaborate on why we can't write a test for this issue? The description of this bug (comment #0) states that we may be able to use the page <http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html> to exhibit this assertion failure. Can we derive a test case from this page?

oh, I am not sure, for it's a random issue regarding to thread. I guess there can be a case written, but pass it once doesn't mean ok. But I will try.

[Raymond]

> > Can you elaborate on why we can't write a test for this issue? The description of this bug (comment #0) states that we may be able to use the page <http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html> to exhibit this assertion failure. Can we derive a test case from this page?
> 
> oh, I am not sure, for it's a random issue regarding to thread. I guess there can be a case written, but pass it once doesn't mean ok. But I will try.

think again, then I realized that you are right, I can establish a case deliberately for this specific case.

[Raymond]

Created attachment 124491 [details]
Patch

[Raymond]

(In reply to comment #2)
> (From update of attachment 123254 [details])
> > Source/WebCore/ChangeLog:8
> > +        No new tests required.
> 
> Can you elaborate on why we can't write a test for this issue? The description of this bug (comment #0) states that we may be able to use the page <http://chromium.googlecode.com/svn/trunk/samples/audio/wavetable-synth.html> to exhibit this assertion failure. Can we derive a test case from this page?

Hi Daniel

Layout test case added. Basicly, a delay node without input is connected to the gain node which itself is connected to the context destination. And to pump up, a sourcebuffer node is connected directly to the destination. Not sure is there better way to run a async test case.

[Chris Rogers]

Comment on attachment 124491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124491&action=review

Great test!  But I think the name of the test should be changed to "audionode-connect-order".  We shouldn't talk about "audiobus" in the layout tests because it is a low-level C++ class not exposed to the JS API.  The layout tests are meant to be (potentially) shared across browser implementations, so must hide these types of details and instead use terminology only exposed through the JS API.

> LayoutTests/webaudio/audiobus-assert.html:55
> +    delay.connect(gain);

I think we should have a fairly detailed descriptive comment which explains that the test involves the *order* that we connect nodes.
In this case, the delay node is connected to the gain node, *before* anything is connected to the delay node.  So this test is very much involved with the order of connections.

[Raymond]

Created attachment 124633 [details]
Patch

[Raymond]

(In reply to comment #7)
> (From update of attachment 124491 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124491&action=review
> 
> Great test!  But I think the name of the test should be changed to "audionode-connect-order".  We shouldn't talk about "audiobus" in the layout tests because it is a low-level C++ class not exposed to the JS API.  The layout tests are meant to be (potentially) shared across browser implementations, so must hide these types of details and instead use terminology only exposed through the JS API.
> 
> > LayoutTests/webaudio/audiobus-assert.html:55
> > +    delay.connect(gain);
> 
> I think we should have a fairly detailed descriptive comment which explains that the test involves the *order* that we connect nodes.
> In this case, the delay node is connected to the gain node, *before* anything is connected to the delay node.  So this test is very much involved with the order of connections.

OK, patch updated ;)

[Chris Rogers]

Comment on attachment 124633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124633&action=review

> LayoutTests/webaudio/audionode-connect-order.html:43
> +    var context = new webkitAudioContext(1, sampleRate * renderLengthSeconds, sampleRate);

Sorry, I should have caught this the first time...

We shouldn't use an "offline" AudioContext here because we're not actually looking at the rendered audio output and instead just want to make sure an ASSERT doesn't happen
We should use a "regular" AudioContext instead:

var context = new webkitAudioContext();


You can use "audionode.html" as an example.

> LayoutTests/webaudio/audionode-connect-order.html:64
> +    context.startRendering();

Since we don't want to use an offline audio context, lines 63:64 can be removed.

and instead simply call finishJSTest() directly...

[Raymond]

> We shouldn't use an "offline" AudioContext here because we're not actually looking at the rendered audio output and instead just want to make sure an ASSERT doesn't happen
> We should use a "regular" AudioContext instead:
> 
> var context = new webkitAudioContext();
> 
> 
> You can use "audionode.html" as an example.
> 
> > LayoutTests/webaudio/audionode-connect-order.html:64
> > +    context.startRendering();
> 
> Since we don't want to use an offline audio context, lines 63:64 can be removed.
> 
> and instead simply call finishJSTest() directly...

I am testing it, while I am wondering, this actually happens when the audio thread do its pull, so it will be an async process, if we don't have control on the stream, the finishJSTest() might be called before the ASSERT actually happens? that's why I choose the offline approaching at first, not for audio output but time control.

[Raymond]

(In reply to comment #10)

Hi Chris

I have tested it. without offline context's oncomplete event, the ASSERT won't be catched...

[Chris Rogers]

Raymond, sorry for the extra trouble.  I see what you mean...

Patch looks good to me.

[Daniel Bates]

Comment on attachment 124633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124633&action=review

Thanks Raymond for updating the patch and writing a test. This patch looks good. I have some very minor spelling/grammar nits.

> LayoutTests/webaudio/audionode-connect-order.html:14
> +description("Test AudioNode connection order for checking ASSERT.");

Maybe the following text is more descriptive: "This tests that we don't trigger an assertion failure due to the AudioNode connection order"?

> LayoutTests/webaudio/audionode-connect-order.html:27
> +        data[i] = Math.sin(frequency * 2.0*Math.PI * i / sampleRate);

Nit: "2.0*Math.PI" => "2 * Math.PI"

(Notice the space on either side of the multiplication operator '*'; also, it's unnecessary to write 2 as 2.0 since all JavaScript numbers are represented using double precision floating point format)

> LayoutTests/webaudio/audionode-connect-order.html:54
> +    // We do this becacuse we try to catch the ASSERT which might be fired due to audionode connection order,

I think you meant to say "try to trigger the ASSERT"? I tend to associate the word "catch" with try/catch blocks, which is a construct that provides a way to recover from an exception by transferring control to the catch clause. In contrast, we don't usually have recovery code associated with an ASSERT(). And, in a debug build, an assertion failure causes a crash. Therefore, I suggest using the word "trigger" as in "trigger the ASSERT".

becacuse => because

And

audionode => AudioNode

> LayoutTests/webaudio/audionode-connect-order.html:55
> +    // especiallly when gain node and delay node is involved e.g. https://bugs.webkit.org/show_bug.cgi?id=76685.

especiallly => especially

[Raymond]

Created attachment 124666 [details]
Patch

[Raymond]

(In reply to comment #14)

Thanks Daniel! Patch updated according to your review. Need to double check my grammar in the future...

[Daniel Bates]

Comment on attachment 124666 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 124666 [details]
Patch

Clearing flags on attachment: 124666

Committed r106332: <http://trac.webkit.org/changeset/106332>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.