Bug 84743 – JavaScriptAudioNode unexpectedly not called

• Component: New Bugs
• Status: NEW

[Raymond Toy]

JavaScriptAudioNode not called unexpectedly

[Raymond Toy]

Created attachment 138608 [details]
WIP

[Dominic Cooney]

Comment on attachment 138608 [details]
WIP

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

Some comments inline. Sorry for the delay!

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This will have tests at some point?

> Source/WebCore/ChangeLog:10
> +        * Modules/webaudio/JavaScriptAudioNode.cpp:

I think you need to add the ActiveDOMObject extended attribute to the interface definition in JavaScriptAudioNode.idl. This triggers CodeGeneratorV8.pm to generate a toActiveDOMObject method on the V8JavaScriptAudioNode wrapper. You could add a breakpoint to GCPrologueVisitor::visitDOMWrapper and see if it gets called with objects that are actually JavaScriptAudioNode.

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:-83
> -    ASSERT_UNUSED(numberOfInputChannels, numberOfInputChannels > 0);

What necessitated commenting this out?

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:96
> +    fprintf(stderr, "~JavaScriptAudioNode: %p\n", this);

Assuming this logging will go away.

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:128
> +    ActiveDOMObject::unsetPendingActivity(this);

Can you give some details in how this works? What is the typical lifecycle of a JavaScriptAudioNode?

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:214
> +

Delete this blank line.

> Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.h:63
> +    // As an audio source, we will never propagate silence.

Why is this relevant to this change?

[Raymond Toy]

Comment on attachment 138608 [details]
WIP

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> This will have tests at some point?

I will try, but I have not been able to come up with any layout tests that work.  Chris says it could be difficult because of the why offline audio contexts work.

>> Source/WebCore/ChangeLog:10
>> +        * Modules/webaudio/JavaScriptAudioNode.cpp:
> 
> I think you need to add the ActiveDOMObject extended attribute to the interface definition in JavaScriptAudioNode.idl. This triggers CodeGeneratorV8.pm to generate a toActiveDOMObject method on the V8JavaScriptAudioNode wrapper. You could add a breakpoint to GCPrologueVisitor::visitDOMWrapper and see if it gets called with objects that are actually JavaScriptAudioNode.

Add ActiveDOMObject to the idl seems to make it work.  At least the audio continues to play as expected instead of being chopped up.

Still investigating your suggestions.

>> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:-83
>> -    ASSERT_UNUSED(numberOfInputChannels, numberOfInputChannels > 0);
> 
> What necessitated commenting this out?

The demo code I was using specified 0 input channels, triggering this assert.  I need to investigate whether 0 inputs is really allows for a JavaScriptAudioNode.

>> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:96
>> +    fprintf(stderr, "~JavaScriptAudioNode: %p\n", this);
> 
> Assuming this logging will go away.

Yes.

>> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:128
>> +    ActiveDOMObject::unsetPendingActivity(this);
> 
> Can you give some details in how this works? What is the typical lifecycle of a JavaScriptAudioNode?

A JavaScriptAudioNode should stay alive as long as inputs are connected.  But with the recent addition of silence hints and tail processing, it's unclear how this works.  Before tail processing was added, the node should go away when all inputs are disconnected.

>> Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.h:63
>> +    // As an audio source, we will never propagate silence.
> 
> Why is this relevant to this change?

This is actually an oversight when the silence hint change was added.  I needed this to make sure this node would produce output even if the inputs are all silent.  (Without this, silent inputs imply silent outputs, causing no processing to be done at all by the node.)

A new bug will be filed for this.

[Raymond Toy]

Created attachment 140616 [details]
Patch

[WebKit Review Bot]

Attachment 140616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 140616 [details]
Patch

Attachment 140616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12655131

[Build Bot]

Comment on attachment 140616 [details]
Patch

Attachment 140616 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12642484