Closed
Bug 908669
Opened 11 years ago
Closed 11 years ago
8bit.js output is different in Chrome and Firefox
Categories
(Core :: Web Audio, defect)
Core
Web Audio
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: padenot, Assigned: padenot)
References
Details
(Whiteboard: [blocking-webaudio+])
Attachments
(7 files, 2 obsolete files)
6.11 KB,
application/x-gzip
|
Details | |
2.80 KB,
text/html
|
Details | |
5.78 KB,
text/x-csrc
|
Details | |
91.73 KB,
image/png
|
Details | |
74.99 KB,
image/png
|
Details | |
14.66 KB,
patch
|
rillian
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
14.38 KB,
patch
|
Details | Diff | Splinter Review |
STR: - Open the attached testcase (mario-bros.html), in Chrome (this is from https://github.com/meenie/8bit.js, uploaded in bugzilla for posterity) - Press play - Listen to the tune, paying close attention to the timbre of the notes, and to time it takes to start, and the duration it runs. - Open the attached testcase in Firefox Expected: - The output should be similar as Chromes Actual results: - It takes a while to start - The timbre is not the same - It stops halfway I tried to diagnose a bit, and this is what I found: - We are using the wrong clock in ProduceAudioData: we want to clock of the context, because we are comparing to mStart and mStop, which are in the AudioContext.currentTime clock domain. So, we should either offset the clock with the value of AudioContext.currentTvim ime when start() was called, or directly use the AudioContext.currentTime clock. This causes (I think) the start time and the fact that it stops halfway - No idea for the timbre, but I know they use a square and a triangle in the demo. Maybe one of our waveform has a bug?
Assignee | ||
Updated•11 years ago
|
Summary: OscillatorNode uses the wrong clock domain → 8bit.js output is different in Chrome and Firefox
Comment 2•11 years ago
|
||
I can't reproduce this. The testcase works fine for me on linux, build from master as of this morning. The music starts as soon as I release the mouse button, and plays for 20 seconds, just like on chrome. Same with repeat plays and waiting some interval before clicking 'play'. I do hear a significant difference is timbre. I wonder if this is due to the difference in the waveform definitions.
Comment 3•11 years ago
|
||
I cannot reproduce the delay either.
Comment 4•11 years ago
|
||
I tried testing on mac, but get a crash loading the page. I filed bug 909467 for that. In the absence of steps-to-reproduce for the time-offset problem, I'll consider this just an issue of the timbre difference, which is probably waveform differences. Revisit once we have a story for https://www.w3.org/Bugs/Public/show_bug.cgi?id=17366
Assignee | ||
Comment 5•11 years ago
|
||
Well, yes, now the clock problem seems to be gone. Not sure why, though.
Updated•11 years ago
|
Assignee: nobody → paul
Assignee | ||
Comment 6•11 years ago
|
||
So, I found the major cause of the difference in output: we don't band limit our signal. For the sine, it's just fine, but for everything else, all the frequencies higher than 24kHz (48kHz / 2) fold back into the audible range, adding unwanted harmonics. See the attached testcase, which is the output of the FFT of different basic waveforms, and compare Chrome and Firefox. We seem to have micro glitches as well, but that's probably the same MSG problem as before.
Assignee | ||
Updated•11 years ago
|
Attachment #798537 -
Attachment is patch: false
Assignee | ||
Updated•11 years ago
|
Attachment #798537 -
Attachment mime type: text/plain → text/html
Assignee | ||
Comment 7•11 years ago
|
||
We have a couple options, here: - Do like WebKit/Blink do, and do an ifft using the setPeriodicWave code when available. - Implement everything but the sine using BLIT synthesis, so we get directly the band-limited signal in the time domain. I've forgotten most of the things I knew about that, but maybe I can squeeze it in for 25. - Use some other method I don't know about, but is simpler than BLIT synthesis. - Ship as-is, and fix later but it's not cool
Flags: needinfo?(giles)
Flags: needinfo?(ehsan)
Assignee | ||
Comment 8•11 years ago
|
||
So, I talked with derf who was conveniently at the Paris office today, I'll try to go with oversampling + lowpass. I'm a bit concerned about the noise floor, but maybe it will turn out fine.
Assignee | ||
Comment 9•11 years ago
|
||
In fact, this is a terrible idea, I've implemented it, and it does not sound great. I've got some very basic BLIT synthesis for a square wave that look (and sound) much better. I'll go with BLIT.
Comment 10•11 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #9) > In fact, this is a terrible idea, I've implemented it, and it does not sound Yes, after thinking it through I realized it wouldn't work, though I asked jmspeex the same question and got the same answer, so I feel a little better about that. > great. I've got some very basic BLIT synthesis for a square wave that look > (and sound) much better. I'll go with BLIT. This seems like a sensible approach.
Comment 11•11 years ago
|
||
If you have BLIT working, that sounds good to me. Should be more efficient than my untuned PeriodicWave.
Flags: needinfo?(giles)
Comment 12•11 years ago
|
||
FWIW the discussion here is pretty well over my head, I trust you guys to make the right call here. :-)
Flags: needinfo?(ehsan)
Assignee | ||
Comment 13•11 years ago
|
||
So, have all three waveforms (square, triangle, sawtooth) implemented using BLIT (outside gecko, in a little C program attached, that generate wav files), with the correct phase. The spectrum/waveforms are looking/sound nice, so I think we can go with that. The algorithm itself should be fast enough. We can always optimize it later (by caching or writing faster code). Amplitudes are not consistent across waveforms, but we can scale to reach what we want. I did not do it, since there are ongoing discussions about how to spec the amplitude in the WG. I'll work on putting it in Gecko, now, and refactor the BLIT algorithm and the DC blockers out so we have less code.
Assignee | ||
Comment 14•11 years ago
|
||
Assignee | ||
Comment 15•11 years ago
|
||
Make it so it actually compiles.
Assignee | ||
Updated•11 years ago
|
Attachment #802965 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
This patch works great (as in, it implements band-limited oscillators, that sound exactly like Webkit/Blink's), but for the triangle, I have a terrible DC offset at the beginning, most probably because I can't figure out the initial integration conditions (the initial value of mSquare and mTriangle for mPhase == M_PI / 2). We need the DCBlocker anyway (that limits the damage), but still, it would be nice to not have this offset at the start. Specifically, I'm not sure what the <=> symbol means in section 4 of [1] ("Square and Sawtooth-Wave Generation"), and I have the feeling that it is somewhat important. The thing we want to solve is in the "Triangle" paragraph. I think I want C6, that is supposed to be |k0 / period + C7|, and C7 is supposed to be "a function on BLIT(0)", which is not super helpful. If we don't have the initial integration condition in time for the uplift, we can hack around it by prerolling the oscillator (basically, letting the DCBlocker stabilize), while I dust off some maths knowledge I once had. [1]: https://ccrma.stanford.edu/~stilti/papers/blit.pdf
Assignee | ||
Comment 17•11 years ago
|
||
When the DCBlocker has blocked the DC offset, it looks way nicer. In practice, the triangle problem results in distortion in the output. Also, I haven't scaled the output yet, but it's easy enough, and we haven't agreed in the WG on how we want to do it.
Comment 18•11 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #16) > Specifically, I'm not sure what the <=> symbol means in section 4 of [1] 12:30 <@derf> It's an error in the PDF conversion. Get the PS version: ftp://sonenvir.at/Misc/papers/analogsynth/blit.ps It seems to be a minus sign ('-') in the postscript version.
Comment 19•11 years ago
|
||
Comment on attachment 802966 [details] [diff] [review] Use band-limited impulse trains (BLIT) for OscillatorNode. r= Review of attachment 802966 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/OscillatorNode.cpp @@ +40,5 @@ > + > +class DCBlocker > +{ > +public: > + // Those are sane defaut when the initial mPhase is zero 'These are sane defaults..." @@ +303,5 @@ > + // pure integrator here. > + mSquare += BipolarBLIT(); > + aOutput[i] = mSquare; > + // maybe we want to apply a gain, the wg has not decided yet > + //buffer[i] *= 1.5; aOutput[i] += 1.5;
Assignee | ||
Comment 20•11 years ago
|
||
Some modifications in this version. I've added comment the part that changed iirc.
Attachment #804531 -
Flags: review?(giles)
Assignee | ||
Updated•11 years ago
|
Attachment #802966 -
Attachment is obsolete: true
Comment 21•11 years ago
|
||
Comment on attachment 804531 [details] [diff] [review] Use band-limited impulse trains (BLIT) for OscillatorNode. r= Review of attachment 804531 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I didn't check the math in any detail, but it seems to work well enough. See nits and questions below. I did notice a few other issues, but they shouldn't hold up landing this; I'll file bugs for them. "imported patch osc" isn't much of a commit message body. ::: content/media/webaudio/OscillatorNode.cpp @@ +40,5 @@ > + > +class DCBlocker > +{ > +public: > + // These are sane defaut when the initial mPhase is zero //These are sane defaults... @@ +159,5 @@ > + // in the middle of the first upward slope. > + // XXX actually do the maths and put the right number here. > + mPhase = M_PI / 2; > + mSquare = 0.5; > + mTriangle = 0; 0.0 for consistency please. @@ +182,4 @@ > } > } > > + // Square and triangle are using bipolar band-limited impulse train, saw is ...using a bipolar... @@ +274,5 @@ > + { > + float blit; > + float denom = sin(mPhase); > + > + if (fabs(denom) <= std::numeric_limits<float>::epsilon()) { This seems to work fine, but I'd expect ~epsilon to be too small to get meaningful values out of sin. Is it because we're converting to double and back when we go through the sin() call? @@ +304,5 @@ > + // pure integrator here. > + mSquare += BipolarBLIT(); > + aOutput[i] = mSquare; > + // maybe we want to apply a gain, the wg has not decided yet > + //aOutput[i] *= 1.5; I would go ahead an scale the square, triangle and saw waves. 1.5 looks about right to make them both closer to full scale and to what chrome it doing. We should still figure out something appropriate in the wg but it's less surprising in the meantime.
Attachment #804531 -
Flags: review?(giles) → review+
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9524b1df278e
Assignee | ||
Comment 23•11 years ago
|
||
Comment on attachment 804531 [details] [diff] [review] Use band-limited impulse trains (BLIT) for OscillatorNode. r= [Approval Request Comment] Bug caused by (feature/regressing bug #): this is part of the last bits of feature work for WebAudio, planned to go in 25. I missed the uplift because the tree was closed all day (european time) yesterday. User impact if declined: Incorrect sound output when using OscillatorNode in WebAudio, resulting in difference in audio output between Chrome/Safari and Firefox. This is very noticeable. Testing completed (on m-c, etc.): Very extensive manual testing, valgrind-clean. Risk to taking this patch (and alternatives if risky): Not risky (this is just maths formulas change). Alternative is to ship WebAudio without and have people complaining on how the OscillatorNode output is better in Chrome/Safari. String or IDL/UUID changes made by this patch: none.
Attachment #804531 -
Flags: approval-mozilla-beta?
Attachment #804531 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 24•11 years ago
|
||
Fix warning-as-error problem on XP https://hg.mozilla.org/integration/mozilla-inbound/rev/7d17f2d129e3
Assignee | ||
Comment 25•11 years ago
|
||
And backed out unrelated changes that crept in. https://hg.mozilla.org/integration/mozilla-inbound/rev/7469a58a5f3d
Comment 26•11 years ago
|
||
The push this bug was in + followups have been backed out for crashes of form: https://tbpl.mozilla.org/php/getParsedLog.php?id=27977619&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=27976043&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/58dbd84ae828 https://hg.mozilla.org/integration/mozilla-inbound/rev/9524b1df278e https://hg.mozilla.org/integration/mozilla-inbound/rev/fb89f2090779 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe576415129e https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5d5e42f6c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/b221626331b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa38a1a2b06 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2aac8dc618 https://hg.mozilla.org/integration/mozilla-inbound/rev/ade49a801461 https://hg.mozilla.org/integration/mozilla-inbound/rev/4323508b3412
Assignee | ||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/86e17fd0298f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•11 years ago
|
status-firefox24:
--- → unaffected
status-firefox25:
--- → affected
status-firefox26:
--- → affected
status-firefox27:
--- → fixed
tracking-firefox25:
--- → +
tracking-firefox26:
--- → +
tracking-firefox27:
--- → +
Updated•11 years ago
|
Attachment #804531 -
Flags: approval-mozilla-beta?
Attachment #804531 -
Flags: approval-mozilla-beta+
Attachment #804531 -
Flags: approval-mozilla-aurora?
Attachment #804531 -
Flags: approval-mozilla-aurora+
Comment 30•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6cb1041ec95 https://hg.mozilla.org/releases/mozilla-beta/rev/b6af3891f9fc
You need to log in
before you can comment on or make changes to this bug.
Description
•