+++ This bug was initially created as a clone of Bug #912474 +++ When speex_resampler_process_float() is compiled with FIXED_POINT, its floating point values should be scaled in the +/-32767 range. In WebAudio this currently affects AudioBufferSourceNode with buffer sample rates differing from the context sample rate, and WaveShaperNode with non-default oversample. http://hg.mozilla.org/mozilla-central/annotate/1a2d9a04ffb2/content/media/webaudio/AudioBufferSourceNode.cpp#l226 http://hg.mozilla.org/mozilla-central/annotate/1a2d9a04ffb2/content/media/webaudio/WaveShaperNode.cpp#l109 It's probably better to use speex_resampler_process_int on these platforms. Perhaps some existing template code can be shared and selected by AudioDataValue. http://hg.mozilla.org/mozilla-central/annotate/1a2d9a04ffb2/content/media/AudioNodeExternalInputStream.cpp#l82

(In reply to Karl Tomlinson (:karlt) from comment #0) > Perhaps some existing template code can be shared and selected by > AudioDataValue. Perhaps some SpeexResamplerProcess methods on WebAudioUtils would be useful, but regardless of the parameter types they should all work appropriately on all platforms, whether or not AudioDataValue is used. That means that at least some implementations will depend on compile-time switches such as MOZ_SAMPLE_TYPE_S16.

Comment on attachment 812463 [details] [diff] [review] Add convert functions. Looks great, thanks.

Comment on attachment 812464 [details] [diff] [review] Modify callers. Looks good to me, thanks. I don't know what the NS_ASSERTION(*aOut <= SPEEX_RESAMPLER_PROCESS_MAX_OUTPUT, "Bad aOut") was testing for. I don't know how |out| in ResampleChannelBuffer could be anything but SPEEX_RESAMPLER_PROCESS_MAX_OUTPUT. http://hg.mozilla.org/mozilla-central/annotate/cc4a3f3f899e/content/media/AudioNodeExternalInputStream.cpp#l130 feedback?roc to confirm there is no need to keep that assertion. >- AudioDataValue* resampledBuffer = new(fallible) AudioDataValue[channelCount * expectedOutSamples]; This made me wonder whether a fallible nsTArray might be necessary in SpeexResamplerProcess(), but ... >- speex_resampler_process_int(resampler, i, &bufferData[i * audioData->mFrames], &inSamples, >- &resampledBuffer[i * expectedOutSamples], >- &outSamples); ... the code wasn't checking for succesful allocation anyway, so switching to the infallible allocator, as in your patches, is definitely better than the existing code.

Comment on attachment 812464 [details] [diff] [review] Modify callers. Review of attachment 812464 [details] [diff] [review]: ----------------------------------------------------------------- that was just to catch buggy callers. It's not that important.

https://hg.mozilla.org/integration/b2g-inbound/rev/0744a7b07dea https://hg.mozilla.org/integration/b2g-inbound/rev/04e5314e0de6

https://hg.mozilla.org/mozilla-central/rev/0744a7b07dea https://hg.mozilla.org/mozilla-central/rev/04e5314e0de6

Given our android tests didn't catch this, it seems we don't have test coverage here.

Comment on attachment 812464 [details] [diff] [review] Modify callers. Please consider this a request for both changesets. [Approval Request Comment] Bug caused by (feature/regressing bug #): Web Audio User impact if declined: I'm requesting Beta approval here because this fixes an issue that would cause complete failure in some audio on Android and B2G. The issue affects the following features, but the first two are only affected when resampling is required, which would be the case if the input data is sampled at a different rate to what the AudioContext is used: * AudioBufferSourceNode, which is very common. * MediaStreamAudioSourceNode is used at http://greweb.me/zpeech/ for example * WaveShaperNode.oversample is a new feature in 26 from bug 875277. Testing completed (on m-c, etc.): on m-c, aurora Risk to taking this patch (and alternatives if risky): There is a risk to these features in Desktop platforms, and apparently we don't have automated test coverage for the individual paths. However, we do have test coverage in test_mediaDecoding.html for one path, which was working prior to these changes, and the patches here change code to share with that path. String or IDL/UUID changes made by this patch: none.

I suspect this bug prevent playback from http://dev.noteflight.com/scores/view/66d71f5197498141dc00d01fffed1d383795fa61?app=html5 on mobile devices.

Comment on attachment 812464 [details] [diff] [review] Modify callers. Approving for beta uplift to help with Android failure - please nominate for koi? with an explanation of the risk/reward here for B2G uplift consideration.

(In reply to lsblakk@mozilla.com [:lsblakk] from comment #12) > Approving for beta uplift to help with Android failure - please nominate for > koi? with an explanation of the risk/reward here for B2G uplift > consideration. beta is merging to b2g26 for the duration of this cycle, so this will end up there whether it has koi+ or not.

https://hg.mozilla.org/releases/mozilla-beta/rev/5e64fdaa360c https://hg.mozilla.org/releases/mozilla-beta/rev/cc12d97a80b8

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5e64fdaa360c https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/cc12d97a80b8

AudioBufferSourceNode resampling is now tested since https://hg.mozilla.org/integration/mozilla-inbound/rev/3296ae716782 I've marked bug 875277 in-testsuite? for WaveShaperNode.oversample. That leaves MediaStreamAudioSourceNode. Perhaps test_mediaStreamAudioSourceNodeResampling.html can be improved by comparing with a buffer from decodeAudioData() (see test_mediaDecoding.html). If audio.play() and createScriptProcessor() are called during the same event, then the alignment may be more predictable, but there will still be latency from the resampler to accommodate.