1223520 - "Assertion failure: mOutputBuffer[halfSize].i == 0" with NaN buffer

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: mOutputBuffer[halfSize].i == 0, at FFTBlock.h:141

This assertion was added in bug 1220037.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Karl Tomlinson (:karlt)]

The non-zero value comes from the previous call to BufferComplexMultiply for
the same block.  Although the imaginary components of the factors are zero,
the imaginary component of the result is zero * NaN = NaN.

Comment 3

[Karl Tomlinson (:karlt)]

There is a bug here that did not exist with kiss_fft (without MOZ_LIBAV_FFT from bug 1157768):  If there is a finite impulse response buffer and a single NaN (or inf, I assume, because 0 * inf is NaN) in the input stream, then the output stream will continue to produce NaNs forever regardless of the input.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: finite period of influence test

This tests for the issue in comment 3, without depending on the any assertions
in the code.  It assumes bug 1224102 is fixed, but could be modified to
tolerate a larger period if that bug is not fixed.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: avoid leaking NaNs to and from the otherwise unused imaginary frequency components

The zeroth component is not removed from the BufferComplexMultiply() call so
as not to disrupt alignment.

The mOutputBuffer[halfSize].i assertions are removed because the code no
longer uses these components, and so their values are irrelevant.

Comment 6

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e91744c0bd7&filter-tier=1

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51287ab3d66a
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd88c2150e1e

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51287ab3d66a
https://hg.mozilla.org/mozilla-central/rev/cd88c2150e1e

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8686444 [details] [diff] [review]
avoid leaking NaNs to and from the otherwise unused imaginary frequency components

Approval Request Comment
[Feature/regressing bug #]: bug 1157768
[User impact if declined]:
If a single NaN (or inf, I assume) gets into the input stream, then the output stream will continue to produce NaNs forever regardless of the input.
There are possible reasons to suspect this might happen unintentionally (e.g. bug 957024), but I don't know how common these are.
If I knew of a real page with this problem, then I would request approval for 43 also.
[Describe test coverage new/current, TreeHerder]: new test
[Risks and why]: low. small patch.
[String/UUID change made/needed]: none

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Karl, given that we do not have a real page/audio feed with this kind of a problem, do you think we can let this one ride the trains? 

Also, just so I understand the end-user impact, are we saying that because of the bug if the input audio stream had a glitch (NaN value), we would never be able to recover from it and only play back garbage? I am trying to assess the severity and whether uplifting to Aurora is needed or not.

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Ritu Kothari (:ritu) from comment #10)
> Karl, given that we do not have a real page/audio feed with this kind of a
> problem, do you think we can let this one ride the trains? 

My fear is that this bug can exacerbate the effects of bug 957024, which was noticed by real developers, not just fuzz tests.

Our test coverage of this is much better now due to other regressions in 42, so we can have confidence in the patch.
 
> Also, just so I understand the end-user impact, are we saying that because
> of the bug if the input audio stream had a glitch (NaN value), we would
> never be able to recover from it and only play back garbage?

Yes, it would demonstrate as silence until page reload.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #11)
> (In reply to Ritu Kothari (:ritu) from comment #10)
> > Karl, given that we do not have a real page/audio feed with this kind of a
> > problem, do you think we can let this one ride the trains? 
> 
> My fear is that this bug can exacerbate the effects of bug 957024, which was
> noticed by real developers, not just fuzz tests.
> 
> Our test coverage of this is much better now due to other regressions in 42,
> so we can have confidence in the patch.
>  
> > Also, just so I understand the end-user impact, are we saying that because
> > of the bug if the input audio stream had a glitch (NaN value), we would
> > never be able to recover from it and only play back garbage?
> 
> Yes, it would demonstrate as silence until page reload.

Thanks! Given that we are still early in the Aurora cycle, I will take it in FF44. It's good to know that this has good test coverage and I am hoping that we will catch regressions sooner.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8686444 [details] [diff] [review]
avoid leaking NaNs to and from the otherwise unused imaginary frequency components

Patch seems safe to uplift to Aurora44.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/54e1a12b93e6
https://hg.mozilla.org/releases/mozilla-aurora/rev/68adba814ed0