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

RESOLVED FIXED in mozilla2.0

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Joe Turner, Unassigned)

Tracking

({regression})

Trunk
mozilla2.0
regression
Points:
---

Firefox Tracking Flags

(blocking2.0 Macaw+, status2.0 .1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey [fx4-rc-ridealong], URL)

Attachments

(5 attachments)

(Reporter)

Description

7 years ago
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

7 years ago
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.
Version: unspecified → Trunk
Component: General → Video/Audio
Product: Firefox → Core
QA Contact: general → video.audio
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
(Reporter)

Comment 4

7 years ago
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.
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
Created attachment 518914 [details] [diff] [review]
Fixes subarray of subarray
Attachment #518914 - Flags: review?(jorendorff)
Attachment #518914 - Flags: feedback?
Created attachment 518915 [details] [diff] [review]
webgl conformance test change for subarray of subarray
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.
David, how badly does this impact the audio data api?  Does this need to block?
Assignee: nobody → general
Blocks: 636078
blocking2.0: --- → ?
Component: Video/Audio → JavaScript Engine
QA Contact: video.audio → general
Keywords: regression
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?
blocking2.0: ? → ---
Component: JavaScript Engine → Video/Audio
Apologies, I reset some of Boris' changes when I updated the bug in an existing tab.
blocking2.0: --- → ?
Component: Video/Audio → JavaScript Engine
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 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.
Attachment #518914 - Flags: review+
Created attachment 519162 [details] [diff] [review]
JS test
Attachment #519162 - Flags: review?(async.processingjs)

Updated

7 years ago
Attachment #519162 - Flags: review?(async.processingjs) → review+
(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.
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.

Updated

7 years ago
OS: Linux → All
Hardware: x86 → All
blocking2.0: ? → .x+
Hardware: All → x86
Summary: Audio Data API crackly on post 27/2/11 builds → subarrays of subarrays of typed arrays have the wrong byte offset (affects audio API)
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [fx4-rc-ridealong]

Updated

7 years ago
Hardware: x86 → All
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. :( :( :(
Attachment #518914 - Flags: review?(jorendorff)
Attachment #518914 - Flags: review+
Attachment #518914 - Flags: feedback?
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)
blocking2.0: .x+ → Macaw
Created attachment 523439 [details] [diff] [review]
Patch-fix for 2.0

This is exactly the same push as occurred to TM (and is in m-c now).
Attachment #523439 - Flags: review+
Attachment #523439 - Flags: approval2.0?
Created attachment 523449 [details] [diff] [review]
Patch-test for 2.0

Again the same as the one that landed in m-c/TM, easy-peasy for 2.0.
Attachment #523449 - Flags: review+
Attachment #523449 - Flags: approval2.0?
Comment on attachment 523439 [details] [diff] [review]
Patch-fix for 2.0

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #523439 - Flags: approval2.0? → approval2.0+
Attachment #523449 - Flags: approval2.0? → approval2.0+

Comment 24

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