Implement the ended event for AudioBufferSourceNode

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
mozilla23
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I need to write the spec for it first...
Naive question: is 'finished' the appropriate name? WebRTC MediaStream fire an 'ended' event at the end of the stream.
(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.
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
Posted patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #745581 - Flags: review?(roc)
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.
Posted patch Patch (v2)Splinter Review
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/fb926cf3a068
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.