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.

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

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

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

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

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.

This time with correct length for floatResponse.

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

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

Bug 921695 identifies other code with similar issues.

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

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.

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

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

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

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.

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

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.

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

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?

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