898291 - PannerNode with HRTF should continue producing output a short time after it is GCed

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

HR impulse responses for convolution are 256 samples at 44.1 kHz.  That means
there should be up to 5.8 ms extra output after input stops.  There could be a
noticeable click if the output shuts off quickly near the end of a block.

PlayingRefChangeHandler was designed to help solve this.

Comment 1

[(no longer active)]

FWIW I'd like us to land this fix at the same time as bug 865241, since otherwise the pannernode output can be chopped.

Comment 2

[Karl Tomlinson (:karlt)]

The patches in bug 865241 now have the PannerNodeEngine keep playing sound after the input becomes null, but there is still the chance of the output being chopped early if GC happens at the wrong time.

PlayingRefChangeHandler is not the full solution here because the Engine can start playing and send an ADDREF to the main thread, but the Node may no longer be alive to receive it.

Comment 3

[(no longer active)]

(In reply to comment #2)
> The patches in bug 865241 now have the PannerNodeEngine keep playing sound
> after the input becomes null, but there is still the chance of the output being
> chopped early if GC happens at the wrong time.
> 
> PlayingRefChangeHandler is not the full solution here because the Engine can
> start playing and send an ADDREF to the main thread, but the Node may no longer
> be alive to receive it.

Yeah, that's sort of a general problem which we need to address at some point.  I believe that this aggressively, so you can fix this problem by using PlayingRefChangeHandler and file a follow-up bug to address the higher level problem.

Comment 4

[(no longer active)]

Karl, can you work on this before the next uplift please?

Comment 5

[Karl Tomlinson (:karlt)]

I don't think this needs to block webaudio.

Retitling because the issue was mostly addressed in bug 865241.

A webaudio client will usually keep a reference to a PannerNode, because it can't be panned without a reference.

Even if the client does not keep a reference, the likelihood of the Node being GC'd and that propagating to the MSG thread to delete the Engine before the output latency expires seems low.

Given this, it doesn't seem worthwhile rushing a partial fix with PlayingRefChangeHandler because we haven't worked out how to implement a complete solution.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> I believe that this aggressively,

I didn't understand this comment.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #5)
> A webaudio client will usually keep a reference to a PannerNode, because it
> can't be panned without a reference.

That's not entirely true, because it can be panned just by methods on the AudioListener.  Still it seems the issues here rarely occur.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> PlayingRefChangeHandler is not the full solution here because the Engine can
> start playing and send an ADDREF to the main thread, but the Node may no
> longer be alive to receive it.

Spoke with roc about this.

The Engine cannot take a reference to the Node, even with a NodeMutex, because
the Node is cycle collected on the main thread, adjusting the ref count
without taking the NodeMutex.

We can take the self reference from the Node when an input is added.  ADDREF
should not be happening from an event.  The Engine would not send RELEASE
until the last input has been removed and buffered samples have been output.

With that approach cycles would keep themselves alive.  This is perhaps what we
want if the cycle is connected to an AudioDestinationNode and we want to treat
repeating cycles like looping source nodes.  Doing a more thorough analysis to
determine whether there is energy in the cycle, and whether an
AudioDestinationNode is connected may be possible (bug 897796).  The self
reference could be triggered off such a system, but that's probably more
complication than we want to implemenent right now.

A variation we discussed was taking the self reference when the last input is
removed.  This would not keep cycles alive.

Comment 8

[(no longer active)]

The reason why I think we should block on this is that the behavior here depends on when we run GC/CC which is not exposed to content.  This means that in some cases, the audio output of the application might be chopped and in other cases it will be fine.  This inconsistency is really bad, especially since the workaround is not obvious.

Comment 9

[Karl Tomlinson (:karlt)]

Now that I have a plan, I'll see what I can do, but I need to fix bug 910171 first.   I may fix this together with bug 910174, which is also more important than this bug because longer delays are less likely to expire before the Engine is destroyed.  I'm not yet confident that I'll have this finished in two weeks time.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: mochitest

Getting a test to fail required some engineering, but it fails reasonably reliably on desktop platforms.  On Android, it seems that passing connect messages to the graph thread takes too long to time a reliable failure.

https://tbpl.mozilla.org/?tree=Try&rev=778204c08526

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: erase azimuth/elevation history when reset()ing HRTFPanner

so that old cross-fade info does not distort new sound when a PannerNode finishes and restarts.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: remove unused PannerNodeEngine model type variables

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: skip HRTF panner processing when input has been null long enough for output to be null

This depends on patches in bug 923301, so wait for reviews to come in that bug if you like.

Comment 14

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=5072a8001d85
https://tbpl.mozilla.org/?tree=Try&rev=a619d72b85f8

Comment 15

[Karl Tomlinson (:karlt)]

Landed with an update to use DispatchToMainThreadAfterStreamStateUpdate() for the PlayingRefChangeHandlers consistent with attachment 820836 [details] [diff] [review]:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b47c1bf96830
https://hg.mozilla.org/integration/mozilla-inbound/rev/c91ae6b8208c
https://hg.mozilla.org/integration/mozilla-inbound/rev/52abf1613764
https://hg.mozilla.org/integration/mozilla-inbound/rev/578c80c21547

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b47c1bf96830
https://hg.mozilla.org/mozilla-central/rev/c91ae6b8208c
https://hg.mozilla.org/mozilla-central/rev/52abf1613764
https://hg.mozilla.org/mozilla-central/rev/578c80c21547

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 815781 [details] [diff] [review]
skip HRTF panner processing when input has been null long enough for output to be null

Please consider this a request for changesets c91ae6b8208c 52abf1613764 and 578c80c21547.  The test changeset b47c1bf96830 cannot land on Beta as is because it uses AudioBuffer.copyFromChannel which is only available in 27.
These patches also require bug 914016, bug 923301, and (as a subsequent landing) bug 933156 on Beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Web Audio HRTF panner bug 865241

User impact if declined: 

A significant perform issue has been identified in bug 923319 and 929595 that can degrade some Web Audio demos to the point of being a crunchy mess.
The demo in bug 929595 has been modified to mostly workaround the issue, but there likely will be other situations affected.

The patches here and in dependent bugs allow unnecessary computation to be skipped, which can make the difference between success and complete failure in some demos on many systems.

Testing completed (on m-c, etc.): on m-c Aurora, in testsuite.

Risk to taking this patch (and alternatives if risky): 
There is a moderate amount of code changed here and so moderate risk but limited to Web Audio.

We believe that the significant performance improvement is worth making these changes.

String or IDL/UUID changes made by this patch: none.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d8a337af4346
https://hg.mozilla.org/releases/mozilla-beta/rev/da6663c9edcd
https://hg.mozilla.org/releases/mozilla-beta/rev/8be8c7920b11

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d8a337af4346
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/da6663c9edcd
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8be8c7920b11