test_pannerNodeChannelCount.html fails with NaNs on Android

RESOLVED FIXED in Firefox 25

Status

()

Core
Web Audio
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({verifyme})

Trunk
mozilla27
ARM
Android
verifyme
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ fixed, firefox26+ fixed, b2g-v1.2 affected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

test_pannerNodeChannelCount.html is being added for bug 906966.

996 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_pannerNodeChannelCount.html | Found 1792 different samples, maxDifference: NaN, first bad index: 256 with source offset 0 and destination offset 0 - got 1792, expected 0
998 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_pannerNodeChannelCount.html | Found 1792 different samples, maxDifference: NaN, first bad index: 256 with source offset 0 and destination offset 0 - got 1792, expected 0
1007 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_pannerNodeChannelCount.html | Found 1792 different samples, maxDifference: NaN, first bad index: 256 with source offset 0 and destination offset 0 - got 1792, expected 0
1008 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_pannerNodeChannelCount.html | Found 1792 different samples, maxDifference: NaN, first bad index: 256 with source offset 0 and destination offset 0 - got 1792, expected 0

It does not fail on the 44.1 kHz offline context.
HRTF impulse responses are at 44.1 and so don't need resampling at that frequency.

It also does not fail on the Android 2.2 Tegra boxes, nor on other platforms.
(Assignee)

Comment 1

4 years ago
In speex_resampler_process_float, with FIXED_POINT defined, I don't see any use of 32768 when converting between int16_t and float, so I wonder whether speex_resampler_process_float is not intended for use with (-1, 1) float samples.

http://hg.mozilla.org/mozilla-central/annotate/e3785e299ab6/media/libspeex_resampler/src/resample.c#l953
(Assignee)

Comment 2

4 years ago
The best solution for HRTFElevation.cpp is to have a speex_resampler_process_int path because the incoming samples are int16_t, but I wonder there is at least one other speex_resampler_process_float callers with (-1, 1) float samples:
http://hg.mozilla.org/mozilla-central/annotate/e3785e299ab6/content/media/webaudio/WaveShaperNode.cpp#l109
(Assignee)

Updated

4 years ago
Blocks: 875277, 865241
(Assignee)

Comment 3

4 years ago
Intermittent on tegra-194:

failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=27373433&tree=Try
pass: https://tbpl.mozilla.org/php/getParsedLog.php?id=27364598&tree=Try
Summary: test_pannerNodeChannelCount.html fails with NaNs on Android 4.0 Panda test devices → test_pannerNodeChannelCount.html fails with NaNs on Android

Comment 4

4 years ago
(In reply to Karl Tomlinson (:karlt) from comment #1)
> In speex_resampler_process_float, with FIXED_POINT defined, I don't see any
> use of 32768 when converting between int16_t and float, so I wonder whether
> speex_resampler_process_float is not intended for use with (-1, 1) float
> samples.

o_O is that true?
Flags: needinfo?(jmvalin)
IIRC floating point values should be scaled in the +/-32767 range. At least when the code is compiled with FIXED_POINT. Otherwise, any scaling will work.
Flags: needinfo?(jmvalin)
(Assignee)

Updated

4 years ago
Assignee: nobody → karlt
(Assignee)

Comment 6

4 years ago
Created attachment 811022 [details] [diff] [review]
use speex_resampler_process_int on platforms where speex_resampler_process_float expects samples in the range +/-32767
Attachment #811022 - Flags: review?(ehsan)
(Assignee)

Updated

4 years ago
Attachment #811022 - Flags: review?(ehsan)
(Assignee)

Comment 7

4 years ago
Created attachment 811064 [details] [diff] [review]
use speex_resampler_process_int on platforms where speex_resampler_process_float expects samples in the range +/-32767 v1.1

This time with correct length for floatResponse.
Attachment #811022 - Attachment is obsolete: true
Attachment #811064 - Flags: review?(ehsan)

Comment 8

4 years ago
Comment on attachment 811064 [details] [diff] [review]
use speex_resampler_process_int on platforms where speex_resampler_process_float expects samples in the range +/-32767 v1.1

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

::: content/media/webaudio/blink/HRTFElevation.cpp
@@ +100,5 @@
> +
> +    // When libspeex_resampler is compiled with FIXED_POINT, samples in
> +    // speex_resampler_process_float are rounded directly to int16_t, which
> +    // only works well if the floats are in the range +/-32767.  On such
> +    // platforms its better to resample before converting to float anyway.

Nit: it's
Attachment #811064 - Flags: review?(ehsan) → review+
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/840d3d8c39a0

This bug is making PannerNode with default HRTF model, at best useless, and at worst corruptive to the whole graph on Android platforms including B2G.

pyang, can you try the following links on B2G and/or Android, with this fix, with headphones, please, to see whether they sound similar to Desktop builds?

http://people.mozilla.org/~karlt/panner-horizontal.html
http://people.mozilla.org/~karlt/panner-vertical.html
Blocks: 779297
tracking-firefox25: --- → ?
tracking-firefox26: --- → ?
Flags: needinfo?(pyang)
Flags: in-testsuite+
Keywords: qawanted
(Assignee)

Updated

4 years ago
No longer blocks: 875277
OS: Linux → Android
Hardware: x86_64 → ARM
(Assignee)

Updated

4 years ago
Blocks: 921695
(Assignee)

Comment 10

4 years ago
Bug 921695 identifies other code with similar issues.
https://hg.mozilla.org/mozilla-central/rev/840d3d8c39a0
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
(Assignee)

Comment 12

4 years ago
IIUC 26/Aurora 25/Beta are not relevant to B2G, so B2G v1.2 is the relevant target.
I don't know what the approval and landing processes are for that.

That leaves mobile/android as the only affected platform on 25 and 26, making this bug less significant for those code branches.
blocking-b2g: --- → koi?
status-b2g-v1.2: --- → affected
Comment on attachment 811064 [details] [diff] [review]
use speex_resampler_process_int on platforms where speex_resampler_process_float expects samples in the range +/-32767 v1.1

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

::: content/media/webaudio/blink/HRTFElevation.cpp
@@ +101,5 @@
> +    // When libspeex_resampler is compiled with FIXED_POINT, samples in
> +    // speex_resampler_process_float are rounded directly to int16_t, which
> +    // only works well if the floats are in the range +/-32767.  On such
> +    // platforms its better to resample before converting to float anyway.
> +#ifdef MOZ_SAMPLE_TYPE_S16

According to the comment above, wouldn't it be #ifdef FIXED_POINT? Or is there a connection between FIXED_POINT and MOZ_SAMPLE_TYPE_S16?
(Assignee)

Comment 14

4 years ago
FIXED_POINT is defined only in libspeex_resampler, but MOZ_SAMPLE_TYPE_S16 is defined on the same platforms, because it is used for the same purpose in Gecko.
(In reply to Karl Tomlinson (:karlt) from comment #2)
> The best solution for HRTFElevation.cpp is to have a
> speex_resampler_process_int path because the incoming samples are int16_t,
> but I wonder there is at least one other speex_resampler_process_float
> callers with (-1, 1) float samples:
> http://hg.mozilla.org/mozilla-central/annotate/e3785e299ab6/content/media/
> webaudio/WaveShaperNode.cpp#l109

Why do we need 2 code paths? It seems to me it is always correct to call speex_resampler_process_int first and then convert to floats no matter FIXED_POINT is defined or not.
(Assignee)

Comment 16

4 years ago
(In reply to jwwang from comment #15)
> Why do we need 2 code paths? It seems to me it is always correct to call
> speex_resampler_process_int first and then convert to floats no matter
> FIXED_POINT is defined or not.

That would be correct, but when FIXED_POINT and MOZ_SAMPLE_TYPE_S16 are not defined, speex_resampler_process_int is implemented by a conversion to float, resample, and conversion back to int.

In this code, where input is int and output is float, when FIXED_POINT is *not* defined, it is more efficient to convert to float and use speex_resampler_process_float.

When FIXED_POINT *is* defined, speex_resampler_process_float is implemented by conversion to int, resample and conversion back to float.  That also would not be efficient here.

When FIXED_POINT *is* defined, speex_resampler_process_float is not useful to web audio code because it has to transform samples to the +/-32767 range, and it can transform to int16_t at the same time as doing that.
(In reply to Karl Tomlinson (:karlt) from comment #14)
> FIXED_POINT is defined only in libspeex_resampler, but MOZ_SAMPLE_TYPE_S16
> is defined on the same platforms, because it is used for the same purpose in
> Gecko.

in /configure.in, we have
if test "$OS_TARGET" = "Android"; then
    MOZ_SAMPLE_TYPE_S16=1
    AC_DEFINE(MOZ_SAMPLE_TYPE_S16)
    AC_SUBST(MOZ_SAMPLE_TYPE_S16)

in /media/libspeex_resampler/src/Makefile.in, we have
ifeq ($(OS_TARGET),Android)
DEFINES += -DFIXED_POINT
else
DEFINES += -DFLOATING_POINT
endif

FIXED_POINT and MOZ_SAMPLE_TYPE_S16 are defined when OS_TARGET=Android.
I think it is more robust to test MOZ_SAMPLE_TYPE_S16 in Makefile.in whether to use FIXED_POINT.
(Assignee)

Comment 18

4 years ago
(In reply to jwwang from comment #17)
> I think it is more robust to test MOZ_SAMPLE_TYPE_S16 in Makefile.in whether
> to use FIXED_POINT.

Yes, that sounds good.
(In reply to Karl Tomlinson (:karlt) from comment #12)
> IIUC 26/Aurora 25/Beta are not relevant to B2G, so B2G v1.2 is the relevant
> target.
> I don't know what the approval and landing processes are for that.
> 
> That leaves mobile/android as the only affected platform on 25 and 26,
> making this bug less significant for those code branches.

Tracking for Android in FF25. Getting landed on mozilla-aurora will ensure this patch is taken in B2G 1.2. Please nominate for uplift.
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox26: --- → affected
tracking-firefox25: ? → +
tracking-firefox26: ? → +
Moving from qawanted to verifyme to indicate this is a verification.
Keywords: qawanted → verifyme
(Assignee)

Comment 21

4 years ago
Comment on attachment 811064 [details] [diff] [review]
use speex_resampler_process_int on platforms where speex_resampler_process_float expects samples in the range +/-32767 v1.1

I think we should take this on Aurora for the sake of B2G.

Re Beta, this bug only affects the mobile/android platform, which is a small proportion of our 26 users.  The bug means the PannerNode completely fails with this default HRTF model, killing output, but is probably easy to detect by authors if they test on this platform.  The author workaround is to use the equalpower panning model instead, which is probably more appropriate for mobile platforms because it uses less compute resource.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature HRTF panning bug 865241
User impact if declined: see leading paragraphs in this comment.
Testing completed (on m-c, etc.): on m-c, in test-suite
Risk to taking this patch (and alternatives if risky): 
Risk is limited to Web Audio's HRTF PannerNode.
Small risk to existing platforms, but code is not changed significantly and I have tested common scenarios.

String or IDL/UUID changes made by this patch: none.
Attachment #811064 - Flags: approval-mozilla-beta?
Attachment #811064 - Flags: approval-mozilla-aurora?
(In reply to Karl Tomlinson (:karlt) from comment #12)
> IIUC 26/Aurora 25/Beta are not relevant to B2G, so B2G v1.2 is the relevant
> target.
> I don't know what the approval and landing processes are for that.

Gecko 26 is the target branch for B2G 1.2. At this point in the release, getting Aurora approval = getting into B2G 1.2.

However, I don't understand what makes this a blocker for 1.2. Were you meaning just to indicate you wanted to ask for approval to land on 1.2 or are you actually making an argument here that this is stop-ship blocker for 1.2? Trying to figure out how to triage this.
(Assignee)

Comment 23

4 years ago
Thanks Jason and Alex for explaining the B2G 1.2 / Aurora / 26 situation.

I really just wanted to get this on the radar for B2G 1.2, and ask for approval to land there.  I was mis-assuming that blocking-b2g was like the tracking-firefox* flags.  This doesn't need to be a stop-ship blocker for 1.2, so I'm removing the blocking-b2g request.
blocking-b2g: koi? → ---

Updated

4 years ago
Attachment #811064 - Flags: approval-mozilla-beta?
Attachment #811064 - Flags: approval-mozilla-beta+
Attachment #811064 - Flags: approval-mozilla-aurora?
Attachment #811064 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 24

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c87b736b65fa
https://hg.mozilla.org/releases/mozilla-beta/rev/cfc8205efe6a
status-firefox25: affected → fixed
status-firefox26: affected → fixed

Comment 25

4 years ago
Verified with b2g pvt build

Gaia:     46466c559b4cbb1620fb831f3a922d66e1a336b7
Gecko:    ccee615a9fc9bb748836cfcbbdaef54a3971b3b0
BuildID   20131009060052
Version   27.0a1

panner can work on b2g master build as well as desktop

Karl, can you provide more detail step/expected result so we can do more verification?
Flags: needinfo?(pyang)
(Assignee)

Comment 26

4 years ago
(In reply to Paul Yang: pyang: pyang@mozilla.com from comment #25)
> panner can work on b2g master build as well as desktop

Thanks, Paul.

> Karl, can you provide more detail step/expected result so we can do more
> verification?

Mouse click on "Start" and move the mouse around in the red square.
Leave the other controls at their defaults.

When moving from left to right the sound should seem to move from left to right.

When moving up and down, the effect on / timbre of the sound should change.

This bug was about an issue that affected only mobile builds, so the key thing to verify is that mobile builds sound similar to desktop builds.
You need to log in before you can comment on or make changes to this bug.