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

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?

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...

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.)

(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.

> 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.

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.

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?

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 )

(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...

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.

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.

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...

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".

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

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.

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

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

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 :-).

(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. ;-)

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.

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 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 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)

(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 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

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

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

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