Bug 154538 - Web Audio becomes distorted after sample rate changes
Summary: Web Audio becomes distorted after sample rate changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari 9
Hardware: iPhone / iPad iOS 9.2
: P2 Major
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-22 08:34 PST by Ashley Gullen
Modified: 2017-02-11 03:54 PST (History)
9 users (show)

See Also:


Attachments
Patch (7.88 KB, patch)
2016-02-25 17:50 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2016-02-25 19:32 PST, Jer Noble
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ashley Gullen 2016-02-22 08:34:26 PST
Repro URL: https://www.scirra.com/labs/bugs/iosdistort/

Steps to reproduce:
1. Visit the URL.
2. Observe the displayed audioContext.sampleRate. It should be 44100 Hz.
3. Press the button. A sound should play, and a regular sound starts playing.
4. Kill Safari (double press-and-hold home button, swipe away Safari from apps list)
5. Open Safari again
6. Safari restores the previous page
7. Press the button again
8. Observe sample rate and listen to audio quality

Observed result:
The audio quality is severely distorted and sounds terrible. The sample rate has also changed to 48000 Hz, which may be related.

Expected result:
Audio quality same as on first listen, and sample rate should probably stay at 44100 Hz.

This appears to be a device-specific issue. This does not reproduce on an iPad Air 2, but it does reproduce on an iPhone 4S, all running iOS 9.2.1.

This is causing significant problems for all Construct 2 content on iOS. See here: https://www.scirra.com/forum/ios-sound-problems-since-ios-9-2_t167412
According to user reports, it only happens on iPhones and not iPads, the webview is affected, and the problem is new as of iOS 9.2.

After the audio sounds distorted, closing the Safari tab and reopening it seems to restore the audio quality and the 44100 sample rate. However note this is not an option with Cordova apps. We have also found what seems to be a workaround we can apply from our engine: if on startup we create an AudioContext, immediately call close() on it, throw it away, then create a new AudioContext which we use for the rest of the app, it appears to work correctly. We will shortly be shipping this workaround for iOS devices, but this should be fixed as it affects all existing deployed web content.
Comment 1 Radar WebKit Bug Importer 2016-02-22 10:14:40 PST
<rdar://problem/24771292>
Comment 2 Jer Noble 2016-02-23 09:11:04 PST
Ashley,

WebKit uses AVAudioSession to determine the hardware sample rate currently supported by the system. See <https://developer.apple.com/library/ios/qa/qa1631/_index.html>:

"Important: Different hardware can have different capabilities. For example, the internal speaker on the iPhone 6S models only support a sample rate of 48kHz while previous iPhone models supported a collection of sample rates. If you assume current values will always be your preferred values and for example fill our your client format using the hardware format expecting 44.1kHz when the actual sample rate is 48kHz, your application can suffer problems like audio distortion with the further possibility of other failures."

So even if we were able to fix this problem (which we may not, or at least, not at the WebKit level), you will still have to deal with a 48kHz sample rate if your users are playing to an iPhone 6S internal speaker.
Comment 3 Jer Noble 2016-02-23 09:19:36 PST
That said, I do wonder if the audio hardware's sample rate is changing out from under us. The distortion might be explained if we believe at AudioContext creation time that the sample rate is 48k, and at render time it switches to 44.1k.
Comment 4 Ashley Gullen 2016-02-23 16:24:44 PST
Hi Jer, thanks for looking in to this. However I'm a little confused by your response. When you say "you will still have to deal with a 48kHz sample rate", are you addressing that to web developers? As far as I am aware, there is no other platform that requires us to have to care about the output sample rate, since in general any content at a different sample rate is just automatically resampled to the output sample rate if necessary. In fact the Web Audio API spec states regarding decodeAudioData: "Take the result, representing the decoded linear PCM audio data, and resample it to the sample-rate of the AudioContext if it is different from the sample-rate of audioData." So this appears to be an explicit requirement that Safari's implementation should be complying with.

If we have to do anything differently to handle this, firstly I think iOS would be unique in that regard, which creates a web compatibility issue. Secondly I am not even sure what we would do differently in terms of the Web Audio API? Are we supposed to throw away all our buffers and re-decode them or something? The problem happens on startup, so it's a mystery to me what we would need to do differently and under what circumstances we ought to do it.

If I were to hazard a guess as to what's really going on, I'd guess Safari mistakes the output sample rate for 48 KHz when it's really 44.1 KHz, and accordingly decodeAudioData is resampled to the wrong rate, or something like that.
Comment 5 Jer Noble 2016-02-24 10:36:06 PST
(In reply to comment #4)
> Hi Jer, thanks for looking in to this. However I'm a little confused by your
> response. When you say "you will still have to deal with a 48kHz sample
> rate", are you addressing that to web developers? As far as I am aware,
> there is no other platform that requires us to have to care about the output
> sample rate, since in general any content at a different sample rate is just
> automatically resampled to the output sample rate if necessary. In fact the
> Web Audio API spec states regarding decodeAudioData: "Take the result,
> representing the decoded linear PCM audio data, and resample it to the
> sample-rate of the AudioContext if it is different from the sample-rate of
> audioData." So this appears to be an explicit requirement that Safari's
> implementation should be complying with.

All I'm saying is that Web Audio developers can't assume that the output sample rate will always be 44.1kHz.  If they are manually creating and filling buffers assuming that the sample rate is 44.1kHz (when instead it's 48kHz), they could playback artifacts.  Say, for example, if they were implementing a grain-based player; they could conceivably have gaps between the grains if their sample-rate assumptions were invalid.

> If we have to do anything differently to handle this, firstly I think iOS
> would be unique in that regard, which creates a web compatibility issue.
> Secondly I am not even sure what we would do differently in terms of the Web
> Audio API? Are we supposed to throw away all our buffers and re-decode them
> or something? The problem happens on startup, so it's a mystery to me what
> we would need to do differently and under what circumstances we ought to do
> it.

I'm pointing out a situation where the hardware audio sample rate could change dynamically within the lifetime of an AudioContext. A iPhone user could unplug their headphones, the active route would change to the internal speaker, and the audio hardware would switch up from 44.1kHz to 48kHz.  There is API in the spec to signal such a rate change, so WebKit would have to resample the Web Audio output in that case.

> If I were to hazard a guess as to what's really going on, I'd guess Safari
> mistakes the output sample rate for 48 KHz when it's really 44.1 KHz, and
> accordingly decodeAudioData is resampled to the wrong rate, or something
> like that.

It very well may be, though I think it's more likely that, at the time the AudioContext was created, the hardware sample rate really was 48k, but that the hardware sample rate changed sometime after playback began.
Comment 6 Jer Noble 2016-02-24 10:46:54 PST
I ran your test case against an iPhone 6S running iOS 9.2, and observed the following behavior:

Playing to the internal speaker.
1. Load page, hit OK.
2. Sample rate is 48kHz, no distortion.
3. Plug in headphones.
4. Distortion observed over headphones.
5. Unplugged headphones (restoring playback to the internal speaker).
6. Distortion disappeared.

However:

Playing over headphones.
1. Load page, hit OK.
2. Sample rate is 44kHz, no distortion.
3. Unplug headphones.
4. Distortion observed over internal speaker.
5. Plug in headphones.
6. Distortion disappeared.

So this appears to validate the theory that the audio hardware sample rate is changing dynamically between 44.1kHz and 48kHz and causing the distortion.
Comment 7 Jer Noble 2016-02-24 10:57:40 PST
(In reply to comment #5)
> There is API in the spec to signal such a rate change

(Sorry, this should read "there is _not_ API in the spec to signal such a rate change".)
Comment 8 Ashley Gullen 2016-02-24 11:36:45 PST
(In reply to comment #5)
> All I'm saying is that Web Audio developers can't assume that the output
> sample rate will always be 44.1kHz.  If they are manually creating and
> filling buffers assuming that the sample rate is 44.1kHz (when instead it's
> 48kHz), they could playback artifacts.
I should point out Web Audio developers make no such assumptions - we don't choose the sample rate of decoded buffers, the AudioContext does. I suppose if anything, the spec itself is making the assumption the AudioContext keeps running at the same sample rate.

I think the spec requires that anything played at a different sample rate to the AudioContext be resampled, since you can call createBuffer() to create a buffer at a given sample rate (e.g. 22050) and play it, and it presumably is resampled to the AudioContext rate. If the AudioContext sample rate changes after creation, shouldn't this kick in for existing buffers?
Comment 9 Jer Noble 2016-02-24 12:20:20 PST
(In reply to comment #8)
> (In reply to comment #5)
> > All I'm saying is that Web Audio developers can't assume that the output
> > sample rate will always be 44.1kHz.  If they are manually creating and
> > filling buffers assuming that the sample rate is 44.1kHz (when instead it's
> > 48kHz), they could playback artifacts.
> I should point out Web Audio developers make no such assumptions - we don't
> choose the sample rate of decoded buffers, the AudioContext does. I suppose
> if anything, the spec itself is making the assumption the AudioContext keeps
> running at the same sample rate.

For decoded buffers yes. But some Web Audio authors do crazy stuff with custom decoders and generators; for them, they must pay attention to the output sample rate.

> I think the spec requires that anything played at a different sample rate to
> the AudioContext be resampled, since you can call createBuffer() to create a
> buffer at a given sample rate (e.g. 22050) and play it, and it presumably is
> resampled to the AudioContext rate. If the AudioContext sample rate changes
> after creation, shouldn't this kick in for existing buffers?

Yes, it should, and now I need to figure out why it's not. :)
Comment 10 Jer Noble 2016-02-24 15:54:24 PST
Aha! The sample rate change was a red herring. CoreAudio's output audio node includes a format converter that should handle resampling the output node's input from one sample rate to another. The real reason for the crazy distortion is because, when the output route changes from the internal speaker to headphones, not only does the sample rate change, but the output channel count changes as well.
Comment 11 Jer Noble 2016-02-25 17:49:09 PST
(In reply to comment #10)
> Aha! The sample rate change was a red herring. 

And that itself was a red herring. Turns out, the output unit will request fewer samples when moving from a higher sample rate to a lower one, and the Web Audio engine chokes in that case.
Comment 12 Jer Noble 2016-02-25 17:50:37 PST
Created attachment 272270 [details]
Patch
Comment 13 WebKit Commit Bot 2016-02-25 17:52:36 PST
Attachment 272270 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:241:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Jer Noble 2016-02-25 19:32:29 PST
Created attachment 272280 [details]
Patch
Comment 15 Darin Adler 2016-03-09 09:10:24 PST
Comment on attachment 272280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272280&action=review

Why no regression test?

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:203
> +static void assignAudioBuffersToBus(AudioBuffer* buffers, AudioBus* bus, UInt32 numberOfBuffers, UInt32 numberOfFrames, UInt32 frameOffset, UInt32 framesThisTime)

Bus here should be a reference rather than a pointer.

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:208
> +        float* memory = (float*)((char*)buffers[i].mData + byteOffset);

We normally prefer C++ casts to C style.

> Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:228
> +            m_firstSpareFrame = m_lastSpareFrame = 0;

Two separate lines please.
Comment 16 Jer Noble 2016-03-09 09:14:39 PST
(In reply to comment #15)
> Comment on attachment 272280 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272280&action=review
> 
> Why no regression test?

Two reasons: 1) we have no way to directly control the platform behavior (of requesting fewer or more samples per callback) and 2) the path we use for Web Audio regression tests does its rendering offline, so doesn't actually touch this code.

> > Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:203
> > +static void assignAudioBuffersToBus(AudioBuffer* buffers, AudioBus* bus, UInt32 numberOfBuffers, UInt32 numberOfFrames, UInt32 frameOffset, UInt32 framesThisTime)
> 
> Bus here should be a reference rather than a pointer.

Ok.

> > Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:208
> > +        float* memory = (float*)((char*)buffers[i].mData + byteOffset);
> 
> We normally prefer C++ casts to C style.

Ok.

> > Source/WebCore/platform/audio/ios/AudioDestinationIOS.cpp:228
> > +            m_firstSpareFrame = m_lastSpareFrame = 0;
> 
> Two separate lines please.

Ok.
Comment 17 Jer Noble 2016-03-11 11:35:36 PST
Committed r198035 <http://trac.webkit.org/changeset/198035>
Comment 19 Jer Noble 2016-05-03 15:54:16 PDT
(In reply to comment #18)
> Same issue? 
> <http://stackoverflow.com/questions/17892345/webkit-audio-distorts-on-ios-6-
> iphone-5-first-time-after-power-cycling>

Yes, very likely.
Comment 20 ae 2017-02-06 12:05:37 PST
This problem is still occuring in iOS 10.2.1 if there's an active WebAudio context, and then a <video> element with a video with 48 kHz sampling rate starts playing.

The WebAudio output is either distorted afterwards, or plays too fast (44.1 -> 48).
Comment 21 Jer Noble 2017-02-06 13:22:17 PST
(In reply to comment #20)
> This problem is still occuring in iOS 10.2.1 if there's an active WebAudio
> context, and then a <video> element with a video with 48 kHz sampling rate
> starts playing.
> 
> The WebAudio output is either distorted afterwards, or plays too fast (44.1
> -> 48).

Could you attach or link to a reproduction case? Then I'll take a look.
Comment 22 ae 2017-02-07 03:14:03 PST
Sorry this is happening in a hybrid app, so no simple way to make a testcase. But basically it's really simple:

- Initialize Web Audio API, play a sound from a buffer source
- Create a <video> element via JS with an MPEG4 file that has a 48 kHz audio stream, and play it
- Play the buffer source again. It's either distorted, plays too fast by 48/44.1, or not at all.
Comment 23 ae 2017-02-07 03:20:46 PST
What I can provide is the video file that causes the issue:

http://instinctive.de/prolevel/calibration-tutorial.mp4
Comment 24 Jon Lee 2017-02-07 10:41:21 PST
Jer, or ae@, can we file a new bug to track the issue?
Comment 25 ae 2017-02-09 15:02:45 PST
Should I do that, or someone else? By the way, I just noticed that closing the Web Audio context and re-opening it (after playing the <video> element) fixes the issue...
Comment 26 Darin Adler 2017-02-10 15:02:26 PST
(In reply to comment #25)
> Should I do that, or someone else?

If you are willing, yes, you should do it.
Comment 27 ae 2017-02-11 03:54:49 PST
I've constructed a minimal testcase for the video problem and submitted a new report:

https://bugs.webkit.org/show_bug.cgi?id=168165