Closed Bug 908669 Opened 7 years ago Closed 7 years ago

8bit.js output is different in Chrome and Firefox

Categories

(Core :: Web Audio, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 + fixed
firefox26 + fixed
firefox27 + fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

(Whiteboard: [blocking-webaudio+])

Attachments

(7 files, 2 obsolete files)

Attached file testcase.tar.gz
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?
Summary: OscillatorNode uses the wrong clock domain → 8bit.js output is different in Chrome and Firefox
Duplicate of this bug: 908864
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.
I cannot reproduce the delay either.
Depends on: 909467
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
Well, yes, now the clock problem seems to be gone. Not sure why, though.
Blocks: 907318
Assignee: nobody → paul
Attached file testcase
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.
Attachment #798537 - Attachment is patch: false
Attachment #798537 - Attachment mime type: text/plain → text/html
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)
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.
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.
(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.
If you have BLIT working, that sounds good to me. Should be more efficient than my untuned PeriodicWave.
Flags: needinfo?(giles)
FWIW the discussion here is pretty well over my head, I trust you guys to make the right call here.  :-)
Flags: needinfo?(ehsan)
Attached file osc.c
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.
Make it so it actually compiles.
Attachment #802965 - Attachment is obsolete: true
Attached image beginning.png
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
Attached image end.png
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.
(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 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;
Some modifications in this version. I've added comment the part that changed
iirc.
Attachment #804531 - Flags: review?(giles)
Attachment #802966 - Attachment is obsolete: true
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+
Blocks: 916285
Blocks: 916287
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?
And backed out unrelated changes that crept in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7469a58a5f3d
Blocks: 865256
https://hg.mozilla.org/mozilla-central/rev/86e17fd0298f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #804531 - Flags: approval-mozilla-beta?
Attachment #804531 - Flags: approval-mozilla-beta+
Attachment #804531 - Flags: approval-mozilla-aurora?
Attachment #804531 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 907318
Depends on: 923106
You need to log in before you can comment on or make changes to this bug.