853298 - Make AudioNode an EventTarget

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

This is necessary to implement the recent spec changes with regards to ScriptProcessorNode.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 727508 [details] [diff] [review]
Patch (v1)

Review of attachment 727508 [details] [diff] [review]:
-----------------------------------------------------------------

Won't this break the JSBindingFinalized stuff; since these are wrappercached the binding will never die and we'll never reclaim any finished nodes?

Comment 3

[(no longer active)]

Hmm, not sure.  Boris?

If we can't make these nodes wrappercached, then we can't use nsDOMEventTargetHelper.  Maybe that's OK?  I'm not really sure how this stuff is supposed to work...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Even wrappercached things can have the JS object get gced by the cycle collector.

Or will we be in a situation where CC can't collect these objects for some reason?

If you don't make this stuff wrappercached, then it can't be an event target, because event.target will give you the event target and if you don't want expandos on it disappearing you have to wrappercache it....

(By the way, this also means finished nodes that were targeted by events that are still alive can't be collected, of course.)

Comment 5

[(no longer active)]

(In reply to comment #4)
> Even wrappercached things can have the JS object get gced by the cycle
> collector.

Then that should mean that the JSBindingFinalized stuff will continue to work, right?

> Or will we be in a situation where CC can't collect these objects for some
> reason?

Hmm, the CC should be able to collect this stuff when the JSBindingFinalized function runs (if it disconnects the nodes, that is) unless there is an event object alive somewhere which is referencing the node.  But why does CC matter here?  All we care about is the JS binding going away doing the right thing, as far as I can tell.

> If you don't make this stuff wrappercached, then it can't be an event target,
> because event.target will give you the event target and if you don't want
> expandos on it disappearing you have to wrappercache it....

Good point.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Hmm, the CC should be able to collect this stuff when the JSBindingFinalized function
> runs 

If the wrapper is preserved, then JSBindingFinalized will only be called when the object is being unlinked.  That is, only when the CC has already managed to collect it.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

AudioNodes connected directly or indirectly to the destination can't be collected by CC as long as the AudioContext stays alive. So I believe this will break our node-pruning strategy.

Basically, as long as there's an event listener attached *and* the event might fire in the future, we have to keep the AudioNode and its JS wrapper alive. While there's no listener or the event can't fire in the future, we want to allow the JS wrapper to be collected, at which point we can consider pruning the AudioNode.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think we need smaug in this discussion...

It sounds like what we really want to do is prune unreachable nodes, where unreachable is defined as comment 7... and the strong pointer from AudioContext does NOT count for reachability.  Right?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Sounds right (if I understand the meaning of strong pointer from AudioContext correctly) and sounds
somewhat similar to WebSocket which is kept alive as long as there can be
data coming from network and the object has the right event listeners. 
(WebSocket::UpdateMustKeepAlive() increases and decreases refcnt because of this,
http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp?rev=43128c241d3e&force=1&mark=1100-1105#1058 )

Comment 10

[(no longer active)]

(In reply to comment #9)
> Sounds right (if I understand the meaning of strong pointer from AudioContext
> correctly) and sounds
> somewhat similar to WebSocket which is kept alive as long as there can be
> data coming from network and the object has the right event listeners. 
> (WebSocket::UpdateMustKeepAlive() increases and decreases refcnt because of
> this,
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp?rev=43128c241d3e&force=1&mark=1100-1105#1058
> )

Hmm, I don't think we need to do anything like that in this case, since the nodes are already kept alive.  We just need to run some graph analysis code when the output for each node is finished, which sucks.  I'll try to think of a way to avoid this...

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about this:
-- Give nsDOMEventTargetHelper contain a bool mCanFireEventsInFuture (defaults to true obviously).
-- Give it another bool mWrapperOnlyNeededForEvents (defaults to false).
-- Provide setters for both.
-- When mWrapperOnlyNeededForEvents is true, and mCanFireEventsInFuture is false, stop nsWrapperCache from keeping the JS wrapper alive
-- Make AudioNodes other than DestinationNode set mWrapperOnlyNeededForEvents
-- Make AudioNodes set mCanFireEventsInFuture to false at the right times.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

So that would be somewhat similar to WebSocket. You keep the object alive until X, and after
that CC/GC just do their magic and unlink/delete the object.

I'm not familiar with the ownership model of AudioNodes. Assuming nothing outside an AudioNode
is supposed to keep it alive, let CC to decide when it should die (when there is no JS keeping it alive), and in Unlink kill the raw references there is for that particular AudioNode.

Comment 13

[(no longer active)]

Currently the Web Audio spec says that all audio nodes must be kept alive while the playback continues, which means that finishing playback should drop their reference count.  Also, the API is structured in a way that once you set up the node graph, the only node that you can find without explicitly holding a JS reference to it is the destination ("sink") node, and it's not possible to get to a node's inputs from the node itself in the API exposed to web content, which enabled us to use a near trick to remember when a JS binding has been finalized (see AudioNode::JSBindingFinalized) and destroy the node if we can.

With AudioNodes being EventTargets, it's technically possible for a node's wrapper to be resurrected once an event is dispatched, which complicates this ownership model.  This is already a problem for ScriptProcessorNode in WebKit, and we are trying to get the spec author to remove mentioning the reference counting that needs to happen for audio nodes in the spec, but I believe that roc's idea in comment 11 could work here...

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Why can't we just addref when playing and release when stopping and have strong reference to destination, but destination would not own input. And then addref for the time of "tail processing".

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Because as long as there is a JS wrapper a GainNode hasn't really "stopped" (since someone could add a new input to it).

Comment 16

[(no longer active)]

The trick in comment 11 is not enough since the event object can resurrect the wrapper through event.target.  It seems to me like we should just get the AudioNode lifetime section removed from the spec, and just make AudioNodes wrapper-cached and let the CC do its thing when needed.

Comment 17

[(no longer active)]

(I'll post to public-audio about this.)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I don't really see problem with the spec.
GC/CC is about (1) of https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html#lifetime-AudioNode

2, 3 ad 4 require artificial addref/release

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The language in the spec is a best superfluous. These lifetime rules should not remove nodes from the graph if that could change observable behavior (since then observable behavior would depend on the timing of JS GC, which is bad). Given these rules do not affect observable behavior, they are not needed in the spec. (However, some kind of performance note, describing how nodes that can't produce output will eventually be removed automatically so authors don't have to do it, would still be a good idea.)

So what is the plan here? I think on IRC you said "make everything CCed and wrappercached, remove JSBindingFinalized notifications, make mOutputNodes strong and mInputNodes weak, and make nodes that are playing even when they have no playing inputs keep a reference to themselves"?

I think that should work, but I've been wrong about this before, more than once :-).

Comment 20

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> The language in the spec is a best superfluous. These lifetime rules should
> not remove nodes from the graph if that could change observable behavior
> (since then observable behavior would depend on the timing of JS GC, which
> is bad). Given these rules do not affect observable behavior, they are not
> needed in the spec. (However, some kind of performance note, describing how
> nodes that can't produce output will eventually be removed automatically so
> authors don't have to do it, would still be a good idea.)

Right, and now the entire AudioNode lifetime section is marked as informative.

> So what is the plan here? I think on IRC you said "make everything CCed and
> wrappercached, remove JSBindingFinalized notifications, make mOutputNodes
> strong and mInputNodes weak, and make nodes that are playing even when they
> have no playing inputs keep a reference to themselves"?

Yeah, something like that.  I'm going to tackle this right now.

> I think that should work, but I've been wrong about this before, more than
> once :-).

Haha, yeah this stuff is complicated by I've managed to convince myself that the above is going to work.  We'll see.  ;-)

Comment 21

[(no longer active)]

Attachment: Part 1: Switch the ownership model of audio nodes to be based the cycle collector with wrapper caches

The idea here is that the 2nd part of this bug (which I have not written yet!) will just replace the nsWrapperCache and nsISupports inheritance with nsDOMEventTargetHelper.

Comment 22

[(no longer active)]

Bobby, if I win the push race here, you need to rebase your new node type on top of this to make it wrapper-cached.

Comment 23

[(no longer active)]

Attachment: Part 2: Make AudioNode an EventTarget

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 736567 [details] [diff] [review]
Part 1: Switch the ownership model of audio nodes to be based the cycle collector with wrapper caches

Review of attachment 736567 [details] [diff] [review]:
-----------------------------------------------------------------

At least DelayNodes need to be modified to also hold a self-reference while they have delayed stuff to play, right? Do any other nodes we implement have state that mean they can play while they have no inputs?

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +456,5 @@
>        NS_lround(endOffset*rate) - offsetTicks);
>    ns->SetInt32Parameter(AudioBufferSourceNodeEngine::SAMPLE_RATE, rate);
> +
> +  MOZ_ASSERT(!mPlayingRef, "We can only accept a successful start() call once");
> +  mPlayingRef = this;

We don't actually need this member, right? We could just use a bool to indicate whether a self-reference is held or not, plus some code in the destructor. We could even use a helper object like

class SelfReference {
public:
  void Take(T* t) { if (!mHeld) { mHeld = true; t->AddRef(); } }
  void Drop(T*) { if (mHeld) { mHeld = false; t->Release(); } }
  SelfReference() : mHeld(false) {}
  ~SelfReference() { NS_ASSERTION(!mHeld); }
private:
  bool mHeld;
};

Maybe it's overkill but I think I'd prefer that --- a tiny bit cheaper.

::: content/media/webaudio/AudioBufferSourceNode.h
@@ +92,5 @@
>    double mLoopStart;
>    double mLoopEnd;
>    bool mLoop;
>    bool mStartCalled;
>    nsRefPtr<AudioParam> mPlaybackRate;

This member should be up next to the other pointers.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 736831 [details] [diff] [review]
Part 2: Make AudioNode an EventTarget


> AudioNode::AudioNode(AudioContext* aContext)
>   : mContext(aContext)
> {
>   MOZ_ASSERT(aContext);
>+  nsDOMEventTargetHelper::Init(nullptr);
You want to call nsDOMEventTargetHelper::BindToOwner with correct window.

(I'll remove ::Init. it is useless)

Comment 26

[(no longer active)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Comment on attachment 736567 [details] [diff] [review]
> Part 1: Switch the ownership model of audio nodes to be based the cycle
> collector with wrapper caches
> 
> Review of attachment 736567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> At least DelayNodes need to be modified to also hold a self-reference while
> they have delayed stuff to play, right? Do any other nodes we implement have
> state that mean they can play while they have no inputs?

Yeah, you're right.  I don't think there is any other node that we need to handle like this for now.

Speaking of which, I think we might need to set *aFinished for DelayNode similar to AudioBufferSourceNode...

> ::: content/media/webaudio/AudioBufferSourceNode.cpp
> @@ +456,5 @@
> >        NS_lround(endOffset*rate) - offsetTicks);
> >    ns->SetInt32Parameter(AudioBufferSourceNodeEngine::SAMPLE_RATE, rate);
> > +
> > +  MOZ_ASSERT(!mPlayingRef, "We can only accept a successful start() call once");
> > +  mPlayingRef = this;
> 
> We don't actually need this member, right? We could just use a bool to
> indicate whether a self-reference is held or not, plus some code in the
> destructor. We could even use a helper object like
> 
> class SelfReference {
> public:
>   void Take(T* t) { if (!mHeld) { mHeld = true; t->AddRef(); } }
>   void Drop(T*) { if (mHeld) { mHeld = false; t->Release(); } }
>   SelfReference() : mHeld(false) {}
>   ~SelfReference() { NS_ASSERTION(!mHeld); }
> private:
>   bool mHeld;
> };
> 
> Maybe it's overkill but I think I'd prefer that --- a tiny bit cheaper.

OK.  BTW, the only reason that I am using a member variable is for easier debugging, otherwise I only need to call AddRef/Release.

Comment 27

[(no longer active)]

Attachment: Part 2: Make AudioNode an EventTarget

Comment 28

[(no longer active)]

Attachment: Part 1: Switch the ownership model of audio nodes to be based the cycle collector with wrapper caches

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 737190 [details] [diff] [review]
Part 1: Switch the ownership model of audio nodes to be based the cycle collector with wrapper caches

Review of attachment 737190 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AudioNodeStream.cpp
@@ +184,5 @@
> +  bool allInputsFinished = !!inputCount;
> +  for (uint32_t i = 0; i < inputCount; ++i) {
> +    if (!mInputs[i]->GetSource()->IsFinishedOnGraphThread()) {
> +      allInputsFinished = false;
> +      break;

Just return false here and drop the local variable

Comment 30

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dedbbce5235f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d549a8fc2eb

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dedbbce5235f
https://hg.mozilla.org/mozilla-central/rev/5d549a8fc2eb

Comment 33

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.