don't use FIXED_POINT speex_resampler_process_float() on (-1, 1) float samples

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: karlt, Assigned: jwwang)

Tracking

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

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

6 years ago
+++ 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
Reporter

Comment 1

6 years ago
(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.
Assignee

Comment 2

6 years ago
Attachment #812463 - Flags: review?(karlt)
Assignee

Comment 3

6 years ago
Attachment #812464 - Flags: review?(karlt)
Reporter

Comment 4

6 years ago
Comment on attachment 812463 [details] [diff] [review]
Add convert functions.

Looks great, thanks.
Attachment #812463 - Flags: review?(karlt) → review+
Reporter

Comment 5

6 years ago
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.
Attachment #812464 - Flags: review?(karlt)
Attachment #812464 - Flags: review+
Attachment #812464 - Flags: feedback?(roc)
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.
Attachment #812464 - Flags: feedback?(roc) → feedback+
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0744a7b07dea
https://hg.mozilla.org/mozilla-central/rev/04e5314e0de6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Reporter

Comment 9

6 years ago
Given our android tests didn't catch this, it seems we don't have test coverage here.
Flags: in-testsuite?
Reporter

Comment 10

6 years ago
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.
Attachment #812464 - Flags: approval-mozilla-beta?
Reporter

Comment 11

6 years ago
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.
Attachment #812464 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Reporter

Comment 16

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.