Closed
Bug 865233
Opened 12 years ago
Closed 12 years ago
Implement the ended event for AudioBufferSourceNode
Categories
(Core :: Web Audio, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
I need to write the spec for it first...
|
||
Comment 1•12 years ago
|
||
Naive question: is 'finished' the appropriate name? WebRTC MediaStream fire an 'ended' event at the end of the stream.
|
Assignee | |
Comment 2•12 years ago
|
||
(In reply to comment #1)
> Naive question: is 'finished' the appropriate name? WebRTC MediaStream fire an
> 'ended' event at the end of the stream.
Hmm, finished is what we talked about on public-audio, please feel free to propose ended there.
|
|
Assignee | |
Comment 3•12 years ago
|
||
On the Audio WG call today, people were open to the idea of using the name "ended". :-)
Summary: Implement the finished event for AudioBufferSourceNode → Implement the ended event for AudioBufferSourceNode
|
|
Assignee | |
Comment 4•12 years ago
|
||
Comment on attachment 745581 [details] [diff] [review]
Patch (v1)
Review of attachment 745581 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +560,5 @@
> void
> AudioBufferSourceNode::NotifyMainThreadStateChanged()
> {
> if (mStream->IsFinished()) {
> + DispatchTrustedEvent(NS_LITERAL_STRING("ended"));
This runs during a stable state and is not a safe time to run script. Please dispatch this asynchronously.
|
|
Assignee | |
Comment 6•12 years ago
|
||
Attachment #745581 -
Attachment is obsolete: true
Attachment #745581 -
Flags: review?(roc)
Attachment #745621 -
Flags: review?(roc)
Comment on attachment 745621 [details] [diff] [review]
Patch (v2)
Review of attachment 745621 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +569,5 @@
> + : mNode(aNode) {}
> + NS_IMETHODIMP Run()
> + {
> + // If it's not safe to run scripts right now, schedule this to run later
> + if (!nsContentUtils::IsSafeToRunScript()) {
This check is unnecessary. It's always safe to run script from a main-thread runnable.
Attachment #745621 -
Flags: review?(roc) → review+
|
|
Assignee | |
Comment 8•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Comment on attachment 745621 [details] [diff] [review]
> Patch (v2)
>
> Review of attachment 745621 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: content/media/webaudio/AudioBufferSourceNode.cpp
> @@ +569,5 @@
> > + : mNode(aNode) {}
> > + NS_IMETHODIMP Run()
> > + {
> > + // If it's not safe to run scripts right now, schedule this to run later
> > + if (!nsContentUtils::IsSafeToRunScript()) {
>
> This check is unnecessary. It's always safe to run script from a main-thread
> runnable.
Nope, since they may run from nested event loops! I actually hit an assertion about this when I was debugging my ScriptProcessorNode implementation using alert()s.
|
|
Assignee | |
Comment 9•12 years ago
|
||
|
||
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
|
|
Assignee | |
Comment 11•11 years ago
|
||
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in
before you can comment on or make changes to this bug.
Description
•