1171436 - Support at least 8192 elements for PeriodicWave

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Paul Adenot (:padenot)]

Spec change: https://github.com/WebAudio/web-audio-api/pull/538/commits, discussion: https://github.com/WebAudio/web-audio-api/issues/386

We also need to perform the optimization where we only allocate and compute partials that make sense for the context's sample rate.

Comment 1

[Dan Minor [:dminor]]

(In reply to Paul Adenot (:padenot) (at TPAC, slow reviews) from comment #0)
> Spec change: https://github.com/WebAudio/web-audio-api/pull/538/commits,
> discussion: https://github.com/WebAudio/web-audio-api/issues/386
> 
> We also need to perform the optimization where we only allocate and compute
> partials that make sense for the context's sample rate.

Hi Paul, I'd be interested in working on this. For the optimization mentioned, is the idea to choose an appropriate fft size based upon the context's sample rate, or is it something more involved? Thanks!

Comment 2

[Paul Adenot (:padenot)]

(In reply to Dan Minor [:dminor] from comment #1)
> Hi Paul, I'd be interested in working on this. For the optimization
> mentioned, is the idea to choose an appropriate fft size based upon the
> context's sample rate, or is it something more involved? Thanks!

Yep, the idea is that you only need to generate partials that make sense considering the Nyquist frequency. So for example when you have a sample rate of 44.1kHz, you don't want to generate partials at more than 44.1 / 2 = 22.05kHz.

Comment 3

[Karl Tomlinson (:karlt)]

The tricky part is that, when createBandLimitedTables() is called, we don't know what the fundamental frequency will be, and so we don't know which partials will be above Nyquist.

Part of the full optimization solution would be to create the tables lazily, when required, at which point the fundamental is known, but I'm guessing that might be quite a redesign.

Perhaps simpler (and probably a good thing to do anyway even if the lazy-creation approach will be added later) would be to make m_periodicWaveSize a function of numberOfComponents.

The linear interpolation currently in use means that m_periodicWaveSize >> numberOfComponents is required to render higher components reasonably, which we don't necessarily achieve right now.  Cubic interpolation would provide much better rendering with much smaller m_periodicWaveSize, but that need not be addressed for this bug.

Comment 4

[Dan Minor [:dminor]]

Thanks for the suggestions!

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1171436 - support at least 8192 elements for PeriodicWave r=padenot

Bug 1171436 - support at least 8192 elements for PeriodicWave r?padenot

This increases the maximum PeriodicWave size to 8192 and adds an optimization
to use 8192 elements only in the case where we receive more than 4096
components. In accordance with the spec, a maximum number of components is no
longer enforced.

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8683813 [details]
MozReview Request: Bug 1171436 - support at least 8192 elements for PeriodicWave r=padenot

https://reviewboard.mozilla.org/r/24435/#review22007

Thanks. Can you file a bug for the rest of Karl's suggestions ?

::: dom/media/webaudio/blink/PeriodicWave.cpp:104
(Diff revision 1)
> +    m_numberOfRanges=(unsigned)(3.0f*logf(m_periodicWaveSize)/logf(2.0f));

Spaces around `=`.

::: dom/media/webaudio/test/test_periodicWave.html:40
(Diff revision 1)
> -    ac.createPeriodicWave(new Float32Array(4097), new Float32Array(4097));
> +  ac.createPeriodicWave(new Float32Array(4097), new Float32Array(4097));

You can add an `expectNoException` call here.

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8683813 [details]
MozReview Request: Bug 1171436 - support at least 8192 elements for PeriodicWave r=padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24435/diff/1-2/

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8683813 [details]
MozReview Request: Bug 1171436 - support at least 8192 elements for PeriodicWave r=padenot

Sorry to ask for a re-review, but I realized this morning I had an off by one error that would result in 4096 being rounded up to 8192.

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8683813 [details]
MozReview Request: Bug 1171436 - support at least 8192 elements for PeriodicWave r=padenot

https://reviewboard.mozilla.org/r/24435/#review22041

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/378ce498787c

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/378ce498787c