637643 - subarrays of subarrays of typed arrays have the wrong byte offset (affects audio API)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Joe Turner]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110301 Firefox/4.0b13pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110301 Firefox/4.0b13pre

Playing back simple synthesized sounds using the Audio Data API is laggy and crackly on nightly builds from 28/2/11 and 1/3/11.  Builds from the 27th and earlier do not have this problem.

Reproducible: Always

Steps to Reproduce:
1.Visit http://www.oampo.co.uk/labs/sine/ with nightly from 28/2/11 or 1/3/11
2.Listen to the audio output
Actual Results:  
I hear a sine wave, but it is crackly, and sounds like it is underflowing somewhere in the audio chain.

Expected Results:  
You should hear a clean sine tone.

Comment 1

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110302 Firefox/4.0b13pre

I can reproduce the issue you are talking about - the sound isn't clean as it is on Windows XP,7 or Mac OS with the latest trunk.

My question is why can't I playback the sound using other browsers(even FF 3.6.13), except Safari in Mac OS.

Comment 2

[David Humphrey (:humph)]

I get a bit of crackling on OSX, but only in the very beginning, then it's fine.  I'd start by looking at your buffering code and make sure you aren't missing some samples, or writing 0s.

George, this uses the new Audio API in Firefox 4, so no other browser can make the sound.

Comment 3

[David Humphrey (:humph)]

Unclear if this is related, but talking to Joe, he notes that Float32Arrays did change at the same time:

https://hg.mozilla.org/mozilla-central/rev/8323a963fd6c

I don't have access to the bug to know more details.

Comment 4

[Joe Turner]

A quick extra note to say that I've checked the Float32Arrays used as the output stream, and they show no discontinuities (i.e. changes in value > 0.1) even in the latest nightlies.  Also it might be worth noting that my code uses subarrays a lot for efficiency, and it sounds a little like the output may be reading the entire array, rather than just the subarray.

Comment 5

[Yury Delendik (:yury)]

subarray() does not work as expected:

var a = new Float32Array(10);
for(var i = 0; i < a.length; i++) {
  a[i] = i;
}
var b = a.subarray(5);
var c = b.subarray(1);

print(c[0]); // expected 6, actual 1

----
P.S. Joe, quick workaround for Audiolet: http://pastebin.mozilla.org/1150483

Comment 6

[Yury Delendik (:yury)]

Attachment: Fixes subarray of subarray

Comment 7

[Yury Delendik (:yury)]

Attachment: webgl conformance test change for subarray of subarray

Comment 8

[David Humphrey (:humph)]

We don't have that many users of Float32Array in the tree, and I'd like to see us add some pure js/ tests for this issue as well.  I'm looking at:

https://mxr.mozilla.org/mozilla-central/search?string=float32array&find=js%2Fsrc&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Jason, where is the right place to stick a test for this?  Thanks.

Comment 9

[Boris Zbarsky [:bzbarsky]]

David, how badly does this impact the audio data api?  Does this need to block?

Comment 10

[David Humphrey (:humph)]

It has the potential to break all audio api code that writes audio.  It's possible to work around it, but it won't be obvious to people what the issue is, since it will manifest itself in bad sound vs. an error.

If we could get this fix in, given that it's a regression, that would be ideal.  If writing audio means producing static filled buffers, it's not going to look good.  I'd say 'yes' to blocking, but that needs this patch reviewed, and some tests written.  Boris, maybe you can speak to my question in comment 8?

Comment 11

[David Humphrey (:humph)]

Apologies, I reset some of Boris' changes when I updated the bug in an existing tab.

Comment 12

[Boris Zbarsky [:bzbarsky]]

The right place to put such tests is probably js/src/tests/js1_8_5/regress but one of the JS folks should confirm that...

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 518914 [details] [diff] [review]
Fixes subarray of subarray

Yes, this is absolutely correct.  I'll leave the existing r? alone so you can get the second set of eyes, just to be safe.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: JS test

Comment 15

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/23e8f18fa89c
http://hg.mozilla.org/tracemonkey/rev/1ea2071135dd

Comment 16

[Jeff Walden [:Waldo]]

(I should also note that I added one extra doesn't-overflow sanity assertion to the patch before pushing it, since now it's not just begin*sizeof that can overflow, it's begin*sizeof + offset.)

(In reply to comment #10)
> It has the potential to break all audio api code that writes audio.  It's
> possible to work around it, but it won't be obvious to people what the issue
> is, since it will manifest itself in bad sound vs. an error.

Not all code that writes audio -- only code that writes audio by creating subarrays of subarrays.  I don't know how common that is, but it is definitely less common than taking subarrays of non-subarrays.  (Almost logically necessarily, in fact.)  I could imagine this being point-fixable.  I could also imagine it being the other way.


> If we could get this fix in, given that it's a regression, that would be ideal.

Yes, definitely.  Since jorendorff is supposed to be around today, I'll wait for his + before asking for approval to land.

Comment 17

[David Humphrey (:humph)]

Great, thanks for jumping on this, Jeff.

> > It has the potential to break all audio api code that writes audio.  It's
> > possible to work around it, but it won't be obvious to people what the issue
> > is, since it will manifest itself in bad sound vs. an error.
> 
> Not all code that writes audio -- only code that writes audio by creating
> subarrays of subarrays.  I don't know how common that is, but it is definitely
> less common than taking subarrays of non-subarrays.  (Almost logically
> necessarily, in fact.)  I could imagine this being point-fixable.  I could also
> imagine it being the other way.

It turns out to be fairly common the way that the buffering code works, since you ask for N samples to get written, and often that will mean N-M actually get written, since it won't accept more than it can write without buffering.  So you have to slice/subarray your way out of it, and make sure the buffer's tail gets written first on the next write.  You could do things with loops and create new arrays manually; but almost no one will do that.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 518914 [details] [diff] [review]
Fixes subarray of subarray

Absolutely correct. Sorry for the delay.

This is very sad. I wonder if the bug is what caused the Audio API to be crackly and glitchy in my own recreational experiments. :( :( :(

Comment 19

[Daniel Veditz [:dveditz]]

Want this on the mozilla2.0 branch, please put an approval2.0? request on the appropriate patch (preferably a new one with what actually landed)

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/23e8f18fa89c
http://hg.mozilla.org/mozilla-central/rev/1ea2071135dd

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Patch-fix for 2.0

This is exactly the same push as occurred to TM (and is in m-c now).

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Patch-test for 2.0

Again the same as the one that landed in m-c/TM, easy-peasy for 2.0.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 523439 [details] [diff] [review]
Patch-fix for 2.0

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 24

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110407 Firefox/4.2a1pre

The sound seems clear but when opening a new tab, I get a busy signal - the tone is interrupted. I don't think this is intended as previous Firefox builds do not display this behavior.

Comment 25

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-2.0/rev/707a1efe9832
http://hg.mozilla.org/releases/mozilla-2.0/rev/eb53226a3a99