Closed Bug 1223520 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Web Audio, defect, P2)

42 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 --- affected
firefox44 --- fixed
firefox45 --- fixed
firefox-esr38 --- unaffected

People

(Reporter: jruderman, Assigned: karlt)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(4 files)

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

This assertion was added in bug 1220037.
Attached file stack
Assignee: nobody → karlt
Status: NEW → ASSIGNED
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.
Priority: -- → P2
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.
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.
Attachment #8686441 - Flags: review?(padenot)
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.
Attachment #8686444 - Flags: review?(padenot)
Blocks: 1157768
Keywords: regression
Attachment #8686444 - Flags: review?(padenot) → review+
Attachment #8686441 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/51287ab3d66a
https://hg.mozilla.org/mozilla-central/rev/cd88c2150e1e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Version: Trunk → 42 Branch
Flags: in-testsuite+
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
Attachment #8686444 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(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.
Flags: needinfo?(karlt)
(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 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.
Attachment #8686444 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.