Support the 'onended' EventListener property for AudioBufferSourceNode and OscillatorNode.
See <https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html>, section 4.10.1 and 4.23.1.
Created attachment 202932 [details] Patch
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.
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).
(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.
(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.
(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.
Okay, I filed bug #116871 to track merging in the AudioNode work.
Created attachment 203070 [details] Patch
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
Created attachment 203071 [details] Patch Removed inadventant semicolon (which would be a terrible band name).
*** Bug 115955 has been marked as a duplicate of this bug. ***
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.
Committed r150905: <http://trac.webkit.org/changeset/150905>