916285 - BLIT oscillators don't handle negative frequencies

Status: RESOLVED FIXED

Description

[Ralph Giles (:rillian) | needinfo me]

The spec doesn't say an Oscillator frequency must be positive, and Chrome's implementation handles this. our BLIT implementation does not.

In theory this is useful for time-inverting the sawtooth, but we should at least abs the frequency for the symmetric waveforms. Or amend the spec to disallow this and throw.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Make OscillatorNode handle negative frequencies. r=

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8530997 [details] [diff] [review]
Make OscillatorNode handle negative frequencies. r=

This seems to be obsoleted by changes in bug 1106649.

>+    mInversePhase = frequency > 0 ? 1.0f : -1.0f;

>-      aOutput[i] = sin(mPhase);
>+      aOutput[i] = mInversePhase * sin(mPhase);

I think this change would introduce a discontinuity when the frequency passes through zero.

>+    var context = new OfflineAudioContext(1, 256, 44100);
>+    var osc = context.createOscillator();
>+
>+    osc.frequency.value = -440;
>+    osc.type = t;
>+
>+    osc.connect(context.destination);
>+    osc.start();
>+    context.startRendering().then(function(buffer) {
>+      var samples = buffer.getChannelData(0);
>+      ok(samples[128] < 0., "Phase should be inverted when using a " + t + " waveform");

A period is 100.227 samples, and so 128 is about 1.277 phases, so yes, this should be negative, but it wasn't immediately obvious to me.  Can you add a comment to explain, or change numbers to make this more obvious, please?

Comment 3

[Paul Adenot (:padenot)]

Attachment: Make OscillatorNode handle negative frequencies. r=

Yeah the previous patch was terribly wrong, sorry.

Attached is a screenshot of a sweep from +440Hz to -440Hz, that has been taken
with a build that had the band limited wave table patch, plus the PeriodicWave
optimization patch.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Make OscillatorNode handle negative frequencies. r=

I forgot to fold the patch that contained the test clarification.

Comment 5

[Paul Adenot (:padenot)]

Attachment: waveforms

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8534983 [details] [diff] [review]
Make OscillatorNode handle negative frequencies. r=

Is this still needed with the changes in bug 1106649?

The only thing I see needing fixing for Sine is the phase wrapping to prevent the phase becoming very large, but I don't think attachment 8534983 [details] [diff] [review] addresses that.

Is this fixing something when using band limited wave tables?
I don't understand what makes mPhase advance backwards for -ve frequencies in ComputeCustom when mFinalFrequency is +ve?

>+      // This samples the wave form in the middle of the first period, the value
>+      // should be negative.
>+      ok(samples[Math.floor(44100 / 440 / 2)] < 0., "Phase should be inverted when using a " + t + " waveform");

Don't we want /2 replaced with /4 for 1/4 of the way through the first period?
I'd expect zero at the middle.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Make OscillatorNode handle negative frequencies. r=

This adds phase wrapping for the sine, and fixes the test (I probably got lucky
with rounding, because it was green).

This patch is only needed for the sine, the PeriodicWave (and therefore, with
the other patches applied, square, sawtooth, triangle) was behaving normally.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #7)
> This adds phase wrapping for the sine, and fixes the test (I probably got
> lucky with rounding, because it was green).
> 
> This patch is only needed for the sine, the PeriodicWave (and therefore, with
> the other patches applied, square, sawtooth, triangle) was behaving normally.

What else, other than phase wrapping, needed fixing for sine?
i.e. how was the behavior wrong before?

(In reply to Karl Tomlinson (back Dec 22 :karlt) from comment #6)
> I don't understand what makes mPhase advance backwards for -ve frequencies
> in ComputeCustom when mFinalFrequency is +ve?

Can you explain this, please?

Comment 9

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (back Dec 22 :karlt) from comment #8)
> (In reply to Paul Adenot (:padenot) from comment #7)
> > This adds phase wrapping for the sine, and fixes the test (I probably got
> > lucky with rounding, because it was green).
> > 
> > This patch is only needed for the sine, the PeriodicWave (and therefore, with
> > the other patches applied, square, sawtooth, triangle) was behaving normally.
> 
> What else, other than phase wrapping, needed fixing for sine?
> i.e. how was the behavior wrong before?

The problem was just the phase wrapping. I think I confused myself because I wrote this before deciding to change everything to use wave tables, and I thought it was also needed for the Sine, that does not use BLIT.

I've updated the patch to only contain the phase wrapping, and reordered the patch set (with the wave table and the ComputeCustom optimization) so that this patch comes after the wave table patch, so the test passes.

> 
> (In reply to Karl Tomlinson (back Dec 22 :karlt) from comment #6)
> > I don't understand what makes mPhase advance backwards for -ve frequencies
> > in ComputeCustom when mFinalFrequency is +ve?
> 
> Can you explain this, please?

mFinalFrequency is negative here:

>       mPhase =
>        j1 + sampleInterpolationFactor + basePhaseIncrement * mFinalFrequency;

so it goes backward, because `basePhaseIncrement` cannot be negative (it's the length of an array).

Comment 10

[Paul Adenot (:padenot)]

Attachment: Make OscillatorNode handle negative frequencies. r=

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8536594 [details] [diff] [review]
Make OscillatorNode handle negative frequencies. r=

>   void IncrementPhase()
>   {
>     mPhase += mPhaseIncrement;
>-    if (mPhase > mPhaseWrap) {
>+    if (std::abs(mPhase) > std::abs(mPhaseWrap)) {
>       mPhase -= mPhaseWrap;
>     }

Yes, I think it can be almost as simple as this, but mPhaseWrap is now always positive (in fact constant at 2 * M_PI and so need not be a member variable), and so when mPhaseIncrement is negative and mPhase is < -2ᴨ we don't want to subtract 2ᴨ again.

What do you think about something like the following?:

if (mPhase > 2 * M_PI) {
   mPhase -= 2 * M_PI;
} else if (mPhase < -2 * M_PI) {
   mPhase += 2 * M_PI;
}

Comment 12

[Paul Adenot (:padenot)]

Attachment: Make OscillatorNode handle negative frequencies. r=

Indeed, that should do it.

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8541244 [details] [diff] [review]
Make OscillatorNode handle negative frequencies. r=

>+    } else if (mPhaseIncrement < -2 * M_PI) {

r+ if you change mPhaseIncrement to mPhase here.

Comment 14

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99d7aacbdd3b

Comment 15

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33f5ff9d956

Comment 16

[Paul Adenot (:padenot)]

backed out for opt bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/58388db74dfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b91d597b54

Comment 17

[Paul Adenot (:padenot)]

relanded, two lines were inverted during rebase apparently.

https://hg.mozilla.org/integration/mozilla-inbound/rev/011c2f2f5899
https://hg.mozilla.org/integration/mozilla-inbound/rev/43e2d930ba6f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Something in this push made test_periodicWave.html permafail. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/72d7ae169b09

https://treeherder.mozilla.org/logviewer.html#?job_id=5080110&repo=mozilla-inbound

Comment 19

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/186c02fdb221

Comment 20

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d51d20fd97

Comment 21

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/186c02fdb221
https://hg.mozilla.org/mozilla-central/rev/b8d51d20fd97