Bug 116798 - Support the 'onended' EventListener property for AudioBufferSourceNode and OscillatorNode.
Summary: Support the 'onended' EventListener property for AudioBufferSourceNode and Os...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
: 115955 (view as bug list)
Depends on: 116871
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-26 15:56 PDT by Jer Noble
Modified: 2013-05-29 10:12 PDT (History)
11 users (show)

See Also:


Attachments
Patch (17.45 KB, patch)
2013-05-26 16:02 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (10.38 KB, patch)
2013-05-28 10:59 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (10.38 KB, patch)
2013-05-28 11:19 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-05-26 15:56:03 PDT
Support the 'onended' EventListener property for AudioBufferSourceNode and OscillatorNode.
Comment 1 Jer Noble 2013-05-26 15:56:49 PDT
See <https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html>, section 4.10.1 and 4.23.1.
Comment 2 Jer Noble 2013-05-26 16:02:05 PDT
Created attachment 202932 [details]
Patch
Comment 3 WebKit Commit Bot 2013-05-26 16:03:53 PDT
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.
Comment 4 Chris Rogers 2013-05-27 11:30:13 PDT
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).
Comment 5 Jer Noble 2013-05-27 11:47:42 PDT
(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.
Comment 6 Chris Rogers 2013-05-27 12:24:01 PDT
(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.
Comment 7 Chris Rogers 2013-05-27 12:30:24 PDT
(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.
Comment 8 Jer Noble 2013-05-28 09:28:05 PDT
Okay, I filed bug #116871 to track merging in the AudioNode work.
Comment 9 Jer Noble 2013-05-28 10:59:07 PDT
Created attachment 203070 [details]
Patch
Comment 10 EFL EWS Bot 2013-05-28 11:15:06 PDT
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
Comment 11 Jer Noble 2013-05-28 11:19:18 PDT
Created attachment 203071 [details]
Patch

Removed inadventant semicolon (which would be a terrible band name).
Comment 12 Praveen Jadhav 2013-05-29 02:36:06 PDT
*** Bug 115955 has been marked as a duplicate of this bug. ***
Comment 13 Eric Carlson 2013-05-29 09:14:11 PDT
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.
Comment 14 Jer Noble 2013-05-29 10:12:07 PDT
Committed r150905: <http://trac.webkit.org/changeset/150905>