Closed Bug 890072 Opened 7 years ago Closed 7 years ago

Prevent the ConvolverNode from being destroyed while the reverb buffer is being exhausted

Categories

(Core :: Web Audio, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 + fixed
firefox25 + fixed

People

(Reporter: ehsan, Assigned: roc)

References

Details

(Whiteboard: [blocking-webaudio+][qa-])

Attachments

(2 files, 1 obsolete file)

No description provided.
Attachment #771041 - Flags: review?(roc) → review+
Comment on attachment 771042 [details] [diff] [review]
Part 2: Keep the ConvolverNode alive while its reverb buffers are being exhausted

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

OK, but I have one question: Can the following happen?

-- A ConvolverNode is created and some inputs are provided, and things run normally for a while.
-- All inputs removed from ConvolverNode
-- On audio thread, buffers drain and a PlayingRefChanged(RELEASE) is queued
-- On main thread, a new input node is added.
-- On audio thread, new input is received and a PlayingRefChanged(ADDREF) is queued
-- On main thread, input is removed.
-- On main thread, PlayingRefChanged(RELEASE) runs and mPlayingRef is dropped.
-- On main thread, cycle collection happens and the ConvolverNode goes away prematurely

Basically, it seems to me PlayingRefChanged events can race with main-thread stuff in a way that means the ADDREF comes too late to keep the node alive.
Attachment #771042 - Flags: review?(roc) → review+
I think the solution here probably requires using MediaStreamGraph::GetCurrentGraphUpdateIndex and related state. We need to keep track of the last graph update that added an input node to the ConvolverNode. Then in the PlayingRefChanged event, we can store the index of the last graph update that was processed. If that PlayingRefChanged event was fired before the adding of the input node was processed by the MSG, then it should be ignored.
Yeah, I guess you're right about the race condition.  Paul, can you please look into implementing comment 4?
Flags: needinfo?(paul)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed650741dc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff011e5b3655

These two patches need to land on Aurora, a=webaudio.
Keywords: checkin-needed
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][leave open]
This broke the build, because gcc with -fpermissive issues a warning which we treat as an error :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/4937e561e40d
https://tbpl.mozilla.org/php/getParsedLog.php?id=24914612&tree=Mozilla-Inbound#error0
Backed out again since apparently where it builds, it will leak in mochitest-1.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4afa33be1fce

I'm afraid I won't have enough time to work on this more in the near future.  Can somebody please pick it up?
Assignee: ehsan → nobody
I'll take this since I need it fixed to land my neutering patch...
Assignee: nobody → roc
Keywords: checkin-needed
Attached patch part 2 v2Splinter Review
The previous version of part 2 had some problems. In particular, if aInput.IsNull(), we never decremented mLeftOverData, which means we'd never fire the RELEASE message.
Attachment #771042 - Attachment is obsolete: true
Attachment #771291 - Flags: review?(ehsan)
Comment on attachment 771291 [details] [diff] [review]
part 2 v2

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

(Please rebase on top of bug 890023 which just landed on inbound)
Attachment #771291 - Flags: review?(ehsan) → review+
Flags: needinfo?(paul)
https://hg.mozilla.org/mozilla-central/rev/ae3cbb0dd69f
https://hg.mozilla.org/mozilla-central/rev/987fd0f14e76
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.
Whiteboard: [blocking-webaudio+] → [blocking-webaudio+][qa-]
You need to log in before you can comment on or make changes to this bug.