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

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[(no longer active)]

Attachment: Part 1: Factor out PlayingRefChanged from DelayNodeEngine

Comment 2

[(no longer active)]

Attachment: Part 2: Keep the ConvolverNode alive while its reverb buffers are being exhausted

Comment 3

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

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.

Comment 4

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

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.

Comment 5

[(no longer active)]

Yeah, I guess you're right about the race condition.  Paul, can you please look into implementing comment 4?

Comment 6

[(no longer active)]

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.

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7151ab8ff70c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e815a5ca50d

Comment 9

[(no longer active)]

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?

Comment 10

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

I'll take this since I need it fixed to land my neutering patch...

Comment 11

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

Attachment: part 2 v2

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.

Comment 12

[(no longer active)]

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)

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3cbb0dd69f
https://hg.mozilla.org/integration/mozilla-inbound/rev/987fd0f14e76

Comment 14

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f12e9f364c3a
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6e678faa38c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae3cbb0dd69f
https://hg.mozilla.org/mozilla-central/rev/987fd0f14e76

Comment 16

[u279076]

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.