Bug 116798 – Support the 'onended' EventListener property for AudioBufferSourceNode and OscillatorNode.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Support the 'onended' EventListener property for AudioBufferSourceNode and OscillatorNode.

[Jer Noble]

See <https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html>, section 4.10.1 and 4.23.1.

[Jer Noble]

Created attachment 202932 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/webaudio/audiobuffersource-ended-expected.txt', u'LayoutTests/webaudio/audiobuffersource-ended.html', u'LayoutTests/webaudio/oscillator-ended-expected.txt', u'LayoutTests/webaudio/oscillator-ended.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/webaudio/AudioBufferSourceNode.cpp', u'Source/WebCore/Modules/webaudio/AudioBufferSourceNode.h', u'Source/WebCore/Modules/webaudio/AudioBufferSourceNode.idl', u'Source/WebCore/Modules/webaudio/OscillatorNode.cpp', u'Source/WebCore/Modules/webaudio/OscillatorNode.h', u'Source/WebCore/Modules/webaudio/OscillatorNode.idl', u'Source/WebCore/dom/EventTarget.h', u'Source/WebCore/dom/EventTargetFactory.in']" exit_code: 1
Source/WebCore/dom/EventTarget.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


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

[Chris Rogers]

Thanks Jer, a couple of high-level comments:

* I'd love to have AudioNode itself inherit from EventTarget according to the current spec:
We should be able to easily back-port this recent Blink patch:
https://codereview.chromium.org/14028011/

* Once that's done, then I think it's easy to move the notification up into AudioScheduledSourceNode::finish(), so we can share between AudioBufferSourceNode and OscillatorNode

* I think it's very important to add an optimization such that the callOnMainThread() does *not* happen unless an event listener has actually been added to "onended".  We can avoid the overhead of the thread hops in most cases.  It should be possible to keep track if an event listener has been added by over-riding addEventListener() and also not directly using the DEFINE_ATTRIBUTE_EVENT_LISTENER macro, but instead related lower-level macros to know when it has been set...

* Just to make matters more complicated, we'll also need to make AudioNode inherit from ActiveDOMObject, so that the JS object doesn't get garbage collected before the listeners have had a chance to fire (can happen now sometimes in ScriptProcessorNode).

[Jer Noble]

(In reply to comment #4)
> Thanks Jer, a couple of high-level comments:
> 
> * I'd love to have AudioNode itself inherit from EventTarget according to the current spec:
> We should be able to easily back-port this recent Blink patch:
> https://codereview.chromium.org/14028011/
> 
> * Once that's done, then I think it's easy to move the notification up into AudioScheduledSourceNode::finish(), so we can share between AudioBufferSourceNode and OscillatorNode

Sure.

> * I think it's very important to add an optimization such that the callOnMainThread() does *not* happen unless an event listener has actually been added to "onended".  We can avoid the overhead of the thread hops in most cases.  It should be possible to keep track if an event listener has been added by over-riding addEventListener() and also not directly using the DEFINE_ATTRIBUTE_EVENT_LISTENER macro, but instead related lower-level macros to know when it has been set...

That seems like a lot of paperwork to avoid a thread hop: the call to the main thread isn't synchronous. And that paperwork will have to be thread-safe.  But i'll look into it.

> * Just to make matters more complicated, we'll also need to make AudioNode inherit from ActiveDOMObject, so that the JS object doesn't get garbage collected before the listeners have had a chance to fire (can happen now sometimes in ScriptProcessorNode).

That should be easy enough; it comes for free with EventTarget nowadays.

[Chris Rogers]

(In reply to comment #5)
> (In reply to comment #4)
> > Thanks Jer, a couple of high-level comments:
> > 
> > * I'd love to have AudioNode itself inherit from EventTarget according to the current spec:
> > We should be able to easily back-port this recent Blink patch:
> > https://codereview.chromium.org/14028011/
> > 
> > * Once that's done, then I think it's easy to move the notification up into AudioScheduledSourceNode::finish(), so we can share between AudioBufferSourceNode and OscillatorNode
> 
> Sure.
> 
> > * I think it's very important to add an optimization such that the callOnMainThread() does *not* happen unless an event listener has actually been added to "onended".  We can avoid the overhead of the thread hops in most cases.  It should be possible to keep track if an event listener has been added by over-riding addEventListener() and also not directly using the DEFINE_ATTRIBUTE_EVENT_LISTENER macro, but instead related lower-level macros to know when it has been set...
> 
> That seems like a lot of paperwork to avoid a thread hop: the call to the main thread isn't synchronous. And that paperwork will have to be thread-safe.  But i'll look into it.

It shouldn't be that bad, and I'm happy to help with that part.

> 
> > * Just to make matters more complicated, we'll also need to make AudioNode inherit from ActiveDOMObject, so that the JS object doesn't get garbage collected before the listeners have had a chance to fire (can happen now sometimes in ScriptProcessorNode).
> 
> That should be easy enough; it comes for free with EventTarget nowadays.

[Chris Rogers]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Thanks Jer, a couple of high-level comments:
> > > 
> > > * I'd love to have AudioNode itself inherit from EventTarget according to the current spec:
> > > We should be able to easily back-port this recent Blink patch:
> > > https://codereview.chromium.org/14028011/
> > > 
> > > * Once that's done, then I think it's easy to move the notification up into AudioScheduledSourceNode::finish(), so we can share between AudioBufferSourceNode and OscillatorNode
> > 
> > Sure.
> > 
> > > * I think it's very important to add an optimization such that the callOnMainThread() does *not* happen unless an event listener has actually been added to "onended".  We can avoid the overhead of the thread hops in most cases.  It should be possible to keep track if an event listener has been added by over-riding addEventListener() and also not directly using the DEFINE_ATTRIBUTE_EVENT_LISTENER macro, but instead related lower-level macros to know when it has been set...
> > 
> > That seems like a lot of paperwork to avoid a thread hop: the call to the main thread isn't synchronous. And that paperwork will have to be thread-safe.  But i'll look into it.
> 
> It shouldn't be that bad, and I'm happy to help with that part.

It should just boil down to a "bool" being set, so the thread safety shouldn't be an issue.  Actually what I'm concerned with other than the in-efficiency of triggering lots of main thread activity when nothing will actually happen is that the callOnMainThread() itself takes a lock which can be contended on the real-time audio thread.  It's true we already take this risk with ScriptProcessorNode, but I don't want to take this hit for every single short sound which is played with AudioBufferSourceNode.

> 
> > 
> > > * Just to make matters more complicated, we'll also need to make AudioNode inherit from ActiveDOMObject, so that the JS object doesn't get garbage collected before the listeners have had a chance to fire (can happen now sometimes in ScriptProcessorNode).
> > 
> > That should be easy enough; it comes for free with EventTarget nowadays.

[Jer Noble]

Okay, I filed bug #116871 to track merging in the AudioNode work.

[Jer Noble]

Created attachment 203070 [details]
Patch

[EFL EWS Bot]

Comment on attachment 203070 [details]
Patch

Attachment 203070 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/716160

[Jer Noble]

Created attachment 203071 [details]
Patch

Removed inadventant semicolon (which would be a terrible band name).

[Praveen Jadhav]

*** Bug 115955 has been marked as a duplicate of this bug. ***

[Eric Carlson]

Comment on attachment 203071 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        (WebCore::AudioScheduledSourceNode::onended): Added; boilerplate.

Might want to remove this inadvertent semicolon too (which is an equally bad band name, even when spelled correctly :¬) ).

> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:200
> +    if (!onended())
> +        return;
> +
> +    RefPtr<Event> event = Event::create(eventNames().endedEvent, FALSE, FALSE);
> +    event->setTarget(this);
> +    onended()->handleEvent(context()->scriptExecutionContext(), event.get());

You might as well cache the listener in a local instead of calling onended() twice because getAttributeEventListener() does a linear search of all listeners so it can be relative expensive.

> LayoutTests/webaudio/audiobuffersource-ended.html:10
> +        function runTest() {

A function's opening brace should be on a new line.

> LayoutTests/webaudio/audiobuffersource-ended.html:22
> +            source.onended = function() {

Ditto.

> LayoutTests/webaudio/oscillator-ended.html:10
> +        function runTest() {

Ditto.

> LayoutTests/webaudio/oscillator-ended.html:22
> +            osc.onended = function() {

Ditto.

[Jer Noble]

Committed r150905: <http://trac.webkit.org/changeset/150905>