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

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: roc)

Tracking

Trunk
mozilla25
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24+ fixed, firefox25+ fixed)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

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]
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
Posted 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
Last Resolved: 6 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.