Closed
Bug 916285
Opened 10 years ago
Closed 8 years ago
BLIT oscillators don't handle negative frequencies
Categories
(Core :: Web Audio, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: rillian, Assigned: padenot)
References
()
Details
Attachments
(3 files, 4 obsolete files)
135.84 KB,
image/png
|
Details | |
3.39 KB,
patch
|
karlt
:
review-
|
Details | Diff | Splinter Review |
3.33 KB,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8530997 -
Flags: review?(karlt)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Comment 2•8 years ago
|
||
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-
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
I forgot to fold the patch that contained the test clarification.
Attachment #8534983 -
Flags: review?(karlt)
Assignee | ||
Updated•8 years ago
|
Attachment #8534969 -
Attachment is obsolete: true
Attachment #8534969 -
Flags: review?(karlt)
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8530997 -
Attachment is obsolete: true
Comment 6•8 years ago
|
||
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-
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8534983 -
Attachment is obsolete: true
Comment 8•8 years ago
|
||
(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?
Assignee | ||
Comment 9•8 years ago
|
||
(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).
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8536594 -
Flags: review?(karlt)
Assignee | ||
Updated•8 years ago
|
Attachment #8535566 -
Attachment is obsolete: true
Attachment #8535566 -
Flags: review?(karlt)
Comment 11•8 years ago
|
||
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-
Assignee | ||
Comment 12•8 years ago
|
||
Indeed, that should do it.
Attachment #8541244 -
Flags: review?(karlt)
Comment 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/99d7aacbdd3b
Assignee | ||
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33f5ff9d956
Assignee | ||
Comment 16•8 years ago
|
||
backed out for opt bustage https://hg.mozilla.org/integration/mozilla-inbound/rev/58388db74dfb https://hg.mozilla.org/integration/mozilla-inbound/rev/12b91d597b54
Assignee | ||
Comment 17•8 years ago
|
||
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•8 years ago
|
||
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
Assignee | ||
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/186c02fdb221
Assignee | ||
Comment 20•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8d51d20fd97
https://hg.mozilla.org/mozilla-central/rev/186c02fdb221 https://hg.mozilla.org/mozilla-central/rev/b8d51d20fd97
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•