BLIT oscillators don't handle negative frequencies

RESOLVED FIXED in mozilla37

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rillian, Assigned: padenot)

Tracking

Trunk
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 4 obsolete attachments)

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.
Priority: -- → P2
Attachment #8530997 - Flags: review?(karlt)
Assignee: nobody → padenot
Status: NEW → ASSIGNED
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?
Attachment #8530997 - Flags: review?(karlt) → review-
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.
Attachment #8534969 - Flags: review?(karlt)
I forgot to fold the patch that contained the test clarification.
Attachment #8534983 - Flags: review?(karlt)
Attachment #8534969 - Attachment is obsolete: true
Attachment #8534969 - Flags: review?(karlt)
Posted image waveforms
Attachment #8530997 - Attachment is obsolete: true
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.
Attachment #8534983 - Flags: review?(karlt) → review-
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.
Attachment #8535566 - Flags: review?(karlt)
Attachment #8534983 - Attachment is obsolete: true
(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?
(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).
Attachment #8535566 - Attachment is obsolete: true
Attachment #8535566 - Flags: review?(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;
}
Attachment #8536594 - Flags: review?(karlt) → review-
Indeed, that should do it.
Attachment #8541244 - Flags: review?(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.
Attachment #8541244 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/186c02fdb221
https://hg.mozilla.org/mozilla-central/rev/b8d51d20fd97
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.