WebAudio Assertion failure: isAzimuthGood and crash [@WebCore::HRTFPanner::pan]

RESOLVED FIXED in Firefox 25

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: posidron, Assigned: karlt)

Tracking

(Blocks 1 bug, {assertion, crash, testcase})

Trunk
mozilla26
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed)

Details

(Whiteboard: [blocking-webaudio-])

Attachments

(8 attachments, 2 obsolete attachments)

Reporter

Comment 1

6 years ago
Posted file callstack
Assignee

Comment 2

6 years ago
azimuth = -nan(0x400000) -> -nan(0x8000000000000)
Assignee: nobody → karlt
Assignee

Comment 3

6 years ago
This fixes the situation for the testcase in attachment, but there are some other edge cases in azimuth calculation that I want to test and deal with.
Assignee

Updated

6 years ago
Attachment #794553 - Attachment description: norm.overflow → avoid under and overflow in Normalize()

Comment 5

6 years ago
Are these patches ready for review?
Whiteboard: [blocking-webaudio-]
Assignee

Comment 6

6 years ago
I'll attach patches here.  They are not top priority because the optimized builds will safely produce null output, but I want to put these up while they're fresh in my mind.
Attachment #794553 - Attachment is obsolete: true
Attachment #796377 - Flags: review?(paul)
Assignee

Updated

6 years ago
Attachment #794555 - Flags: review?(paul)
Assignee

Comment 7

6 years ago
See the checkin comment.  This follows the approach in http://lists.w3.org/Archives/Public/public-audio/2013JulSep/0571.html for the situations undefined in the spec.

I'll come up with a tests.
Attachment #796381 - Flags: review?(paul)
Assignee

Comment 8

6 years ago
The azimuth calculation in the Web Audio spec becomes undefined in this
situation as it requires normalizing a zero vector.  The panning effect should
not depend on azimuth when the source is directly above the listener (because
position does not depend on azimuth), but the specified "equalpower" panning
model does depend on azimuth even at this elevation.  Setting azimuth to zero
produces the same result as if the normalized projection were replaced with a
zero vector.
Attachment #796382 - Flags: review?(paul)
Assignee

Updated

6 years ago
Attachment #796381 - Attachment is obsolete: true
Attachment #796381 - Flags: review?(paul)
Assignee

Comment 10

6 years ago
The main difference from v1 is in normalizing the supplied up vector, because both vectors in the cross-product need to be sufficiently smaller than the maximum finite value in order to know that overflow won't occur.
Attachment #796562 - Flags: review?(paul)
Assignee

Updated

6 years ago
Attachment #796382 - Attachment description: handle zero front-right plane projection without NaNs → part 4: handle zero front-right plane projection without NaNs
Attachment #794555 - Flags: review?(paul) → review+
Attachment #796377 - Flags: review?(paul) → review+
Comment on attachment 796382 [details] [diff] [review]
part 4: handle zero front-right plane projection without NaNs

Review of attachment 796382 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webaudio/PannerNode.cpp
@@ +407,5 @@
> +  if (projectedSource.IsZero()) {
> +    // source - listener direction is up or down.
> +    aAzimuth = 0.0;
> +    return;
> +  }

We are now effectively doing the same as WebKit/Blink (in a slightly different manner), who are implementing something different than the spec. I'll go and file a bug, and quickly change the spec if nobody agrees.

I was thinking it would be nice to have a code resembling what is in the spec, but your solution here is actually better than what is in the spec (since it computes less stuff for the same result). It probably does not matter too much.
Attachment #796382 - Flags: review?(paul) → review+
Attachment #796384 - Flags: review?(paul) → review+
Comment on attachment 796562 [details] [diff] [review]
part 3 v2: normalize orientation vectors early and keep existing state if directions are undefined

Review of attachment 796562 [details] [diff] [review]:
-----------------------------------------------------------------

I assume the tests we have in the tree have the listener and the source on the same plane, so we weren't triggering this?

It would be nice to be to have a testcase that exercise this code, and maybe give the testcase to other implementers so they can check whether their behaviour is correct. I seem to recall they just replace the NaNs/Infinity by zero and continue running the algorithm like nothing happened.

Adding the testcase to our crashtest suite in content/media/test/crashtests would also be nice and quick.
Attachment #796562 - Flags: review?(paul) → review+
Assignee

Comment 13

6 years ago
Forgot to remove this in part3.  Already done in the setter.
Attachment #798385 - Flags: review?(paul)
Assignee

Comment 14

6 years ago
(In reply to Paul Adenot (:padenot) from comment #12)
> I assume the tests we have in the tree have the listener and the source on
> the same plane, so we weren't triggering this?

Yes, panner-model-testing.js has source and listener on the same plane.
That means that the effect of directly above case is not tested.
I'm holding off writing output tests for out of plane cases for the moment because the equalpower model doesn't make much sense out of plane and perhaps that will change.  I have a crashtest for the directly above case.
 
> It would be nice to be to have a testcase that exercise this code, and maybe
> give the testcase to other implementers so they can check whether their
> behaviour is correct. I seem to recall they just replace the NaNs/Infinity
> by zero and continue running the algorithm like nothing happened.

The crashtest also exercises this code, but doesn't test that the effect uses the previous orientation.

https://tbpl.mozilla.org/?tree=Try&rev=80b6bad2f9e8
https://tbpl.mozilla.org/?tree=Try&rev=8501301cd906
OS: Mac OS X → All
Attachment #798385 - Flags: review?(paul) → review+
pushed to inbound and aurora with a=webaudio

https://hg.mozilla.org/integration/mozilla-inbound/rev/3171b6ad0055 (I noticed after pushing that your patches don't have names on them, so this one got pushed with my name)

https://hg.mozilla.org/releases/mozilla-aurora/rev/0177db952fd3
https://hg.mozilla.org/mozilla-central/rev/3171b6ad0055
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee

Comment 18

6 years ago
Other patches are still to land.
I ended up rewriting the crashtest because ScriptProcessorNode isn't (and can't be) as synchronous as I was expecting.
I added test_pannerNodeAbove.html with the equalpower model, but it was already passing before the patches here through min(180, max(-180, NaN) selecting azimuth = -180.

https://tbpl.mozilla.org/?tree=Try&rev=acd2e5851497
https://tbpl.mozilla.org/?tree=Try&rev=c2108eee4512
https://tbpl.mozilla.org/?tree=Try&rev=b318718d606d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

6 years ago
Whiteboard: [blocking-webaudio-] → [blocking-webaudio-][checkin-needed-aurora]
Assignee

Comment 20

6 years ago
In reply to Christoph Diehl [:cdiehl] from comment #1)
> Created attachment 793755 [details]
> callstack

This stack trace points to a crash after the assertion, which I can't explain nor reproduce.  Do you perhaps have a different testcase that fails here?
http://hg.mozilla.org/mozilla-central/annotate/4b5789932294/content/media/webaudio/blink/HRTFPanner.cpp#l224
Flags: needinfo?(cdiehl)
Reporter

Comment 23

6 years ago
(In reply to Karl Tomlinson (:karlt) from comment #20)
> In reply to Christoph Diehl [:cdiehl] from comment #1)
> > Created attachment 793755 [details]
> > callstack
> 
> This stack trace points to a crash after the assertion, which I can't
> explain nor reproduce.  Do you perhaps have a different testcase that fails
> here?
> http://hg.mozilla.org/mozilla-central/annotate/4b5789932294/content/media/
> webaudio/blink/HRTFPanner.cpp#l224

Hm, no I don't have. I will look out for it in the next runs.
Flags: needinfo?(cdiehl)
Reporter

Updated

6 years ago
See Also: → 927697

Comment 24

6 years ago
Comment on attachment 793754 [details]
testcase

Yes, floating point arguments passed to the APIs definitely change the branching decisions.
You need to log in before you can comment on or make changes to this bug.