Closed Bug 898291 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

(Whiteboard: [qa-])

Attachments

(4 files)

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.
FWIW I'd like us to land this fix at the same time as bug 865241, since otherwise the pannernode output can be chopped.
Assignee: nobody → 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.
(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.
Karl, can you work on this before the next uplift please?
Flags: needinfo?(karlt)
Whiteboard: [blocking-webaudio+]
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.
Flags: needinfo?(karlt)
Summary: PannerNode with HRTF should continue producing output a short time after input stops → PannerNode with HRTF should continue producing output a short time after it is GCed
Whiteboard: [blocking-webaudio+] → [blocking-webaudio?]
(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.
Depends on: 910171
(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.
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.
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.
Depends on: 923301
Blocks: 923319
Attached patch mochitestSplinter Review
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
so that old cross-fade info does not distort new sound when a PannerNode finishes and restarts.
Attachment #815779 - Flags: review?(ehsan)
This depends on patches in bug 923301, so wait for reviews to come in that bug if you like.
Attachment #815781 - Flags: review?(ehsan)
Attachment #815779 - Flags: review?(ehsan) → review+
Attachment #815780 - Flags: review?(ehsan) → review+
Attachment #815781 - Flags: review?(ehsan) → review+
Blocks: 929595
Depends on: 933156
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.
Attachment #815781 - Flags: approval-mozilla-beta?
Attachment #815781 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [blocking-webaudio?] → [don't land on beta without approval for bug 933156]
Depends on: 931311
Whiteboard: [qa-]
Depends on: 939491
No longer depends on: 939491
You need to log in before you can comment on or make changes to this bug.