912474 - test_pannerNodeChannelCount.html fails with NaNs on Android

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

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.

Comment 1

[Karl Tomlinson (:karlt)]

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

Comment 2

[Karl Tomlinson (:karlt)]

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

Comment 3

[Karl Tomlinson (:karlt)]

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

Comment 4

[(no longer active)]

(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?

Comment 5

[Jean-Marc Valin]

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.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: use speex_resampler_process_int on platforms where speex_resampler_process_float expects samples in the range +/-32767

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: 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.

Comment 8

[(no longer active)]

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

Comment 9

[Karl Tomlinson (:karlt)]

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

Comment 10

[Karl Tomlinson (:karlt)]

Bug 921695 identifies other code with similar issues.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/840d3d8c39a0

Comment 12

[Karl Tomlinson (:karlt)]

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.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 14

[Karl Tomlinson (:karlt)]

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.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 16

[Karl Tomlinson (:karlt)]

(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.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 18

[Karl Tomlinson (:karlt)]

(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.

Comment 19

[Alex Keybl [:akeybl]]

(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.

Comment 20

[Jason Smith [:jsmith]]

Moving from qawanted to verifyme to indicate this is a verification.

Comment 21

[Karl Tomlinson (:karlt)]

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.

Comment 22

[Jason Smith [:jsmith]]

(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.

Comment 23

[Karl Tomlinson (:karlt)]

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.

Comment 24

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c87b736b65fa
https://hg.mozilla.org/releases/mozilla-beta/rev/cfc8205efe6a

Comment 25

[Paul Yang [:pyang] (away)]

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?

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to Paul Yang: pyang: [e-mail] 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.