Closed Bug 910174 Opened 6 years ago Closed 6 years ago
Node output can be cut off early when it is GCed
1. Load the testcase in attachment 771655 [details]. 2. Click to play 3. Wait Expected results: PlayingRefChange code in DelayNodeEngine sends ADDREF to the node. Actual results: The DelayNodeEngine is destroyed without any ADDREF sent. IIUC this is because aStream->AllInputsFinished() doesn't return true. If this did ever return true, I wonder whether this could be after the DelayNode is destroyed anyway, which would be too late for the PlayingRefChange code to take a reference. Also, as mentioned in bug 898291 comment 2, the Node may be destroyed even before the first ProduceAudioBlock() call.
I wonder whether the Engine could take a (self-ish) reference to its owning Stream. We don't need to keep the Node alive, only the Engine, and its output streams. But I guess that would require MSG redesign to keep the downstream Engines alive and deal with cycles. And we still need a way to ensure that the self reference is dropped (bug 910171).
Why does AllInputsFinished() not return true? That seems like a bug that we should fix.
AudioBufferSourceNode can set aFinished in ProduceAudioBlock when it is done. I don't think many other nodes can do that because they may be reconnected to another source.
(In reply to comment #3) > AudioBufferSourceNode can set aFinished in ProduceAudioBlock when it is done. > I don't think many other nodes can do that because they may be reconnected to > another source. Yeah, that's true. Not all nodes actually know when they're finished, that is the information that the AudioContext has (for the offline case, that is.)
This makes the DelayNode logic similar to ConvolverNode. It removes the dependence on AllInputsFinished() which didn't return true for many input types. The DelayProcessor is no longer continuously reset (bug 921457) and the reference is now correctly added again when all inputs are finished and then new inputs are connected. This depends on the patch in bug 921675 because the DelayProcessor hasn't calculated the current delay time when sound is first received.
Attachment #811445 - Flags: review?(ehsan)
Comment on attachment 811445 [details] [diff] [review] add DelayNode's tail-time reference as soon as the engine receives sound [Approval Request Comment] Bug caused by (feature/regressing bug #): new feature: Web Audio DelayNode User impact if declined: Bug 921457 is the most prominent bug fixed here. That does not depend on GC running, but means that in some circumstances a DelayNode will not play sounds after its first sound is finished. The precise circumstances leading to the bug would be difficult for an author to diagnose. Testing completed (on m-c, etc.): on m-i, in testsuite Risk to taking this patch (and alternatives if risky): The patch only modifies behavior of Web Audio's DelayNode so risk is confined to that. String or IDL/UUID changes made by this patch: none This patch needs the patch in bug 921675.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 811445 [details] [diff] [review] add DelayNode's tail-time reference as soon as the engine receives sound low risk cleanup of a new web feature
(In reply to Karl Tomlinson (:karlt) from comment #0) > Also, as mentioned in bug 898291 comment 2, the Node may be destroyed even > before the first ProduceAudioBlock() call. Bug 923301 covers that.
You need to log in before you can comment on or make changes to this bug.