Allow larger arrays up to size 8192 for PeriodicWave

Allow larger arrays up to size 8192 for PeriodicWave

Larger arrays are allowed, and no errors are thrown if the array is
larger than what the implementation would use.

Also took this opportunity to slightly optimize the computation of the
input signal and the culling of the extra partials.

And finally fixed a bug where the 0-th element of the imaginary array
was not always being ignored as required.  This is tested
osc-low-freq.html.

BUG=496301
TEST=osc-low-freq.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199389

[Raymond Toy, 2015-07-22 18:56:14]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2015-07-22 18:56:15]

PTAL.

[hongchan, 2015-07-22 21:01:20]

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
File LayoutTests/webaudio/osc-low-freq.html (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:19: var highSampleRate = 192000;
This is unused.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:25: // 2048 so the lowest frequency is
24000/2048 = 11.78 Hz.
Partials are not linearly arranged over the frequency range, so not sure if
11.78Hz is the right value. I might ask you more explanation in person.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:38: function checkSineResult(result,
freq, rate) {
Perhaps would |checkCosineResult| fit better for the task? Also can we rename
|rate|?

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:39: // Compute the SNR between the actual
result and expected cosine wave
How about moving this to the line 37? Also a period at the end would be nice.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:78:
context.startRendering().then(function (b) {
Let's use |buffer| instead of |b|.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
File LayoutTests/webaudio/periodicwave-lengths.html (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:35: {name : "192khz-test-1",
sampleRate : 192000, bigWave : 8192, smallWave : 2048, verifier :
resultShouldBeNonZero},
nit: JSON style. A space after/before the curly brace. A space only after the
colon, not before. One item at a line.

{ 
  name: "192khz-test-1",
  sampleRate: 192000,
  bigWave: 8192,
  smallWave: 2048,
  verifier: resultShouldBeNonZero
}

It would be a bit nicer to read.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:79: function prefixMessage
(sampleRate, bigLength, smallLength) {
optional nit: Doesn't look like it's prefixing the message. How about
'assembleMessage' or 'generateMessage'?

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:87: function
resultShouldBeZero(sampleRate, bigLength, smallLength) {
I am pretty sure you've already explored the option of using |Should| utilities
before inventing this verification function. Can I ask you why you decided to
ditch the |Should| utility here?

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:170: function
isBufferZero(buffer) {
This is identical to:
Should('myChannelData', f32array).beConstantValueOf(0);

But I guess you only needed the boolean value, not an actual failure or pass.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/re...
File LayoutTests/webaudio/resources/audio-testing.js (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/re...
LayoutTests/webaudio/resources/audio-testing.js:437: // "PASS SNR exceeds 100"
We previously agreed to add the example message from the failure.

[Raymond Toy, 2015-07-22 22:17:48]

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
File LayoutTests/webaudio/periodicwave-lengths.html (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:87: function
resultShouldBeZero(sampleRate, bigLength, smallLength) {
On 2015/07/22 21:01:19, hoch wrote:
> I am pretty sure you've already explored the option of using |Should|
utilities
> before inventing this verification function. Can I ask you why you decided to
> ditch the |Should| utility here?

I initially used Should.beConstantValueOf in resultShouldBeZero, but there was
no corresponding function to use in resultShouldNotBeZero.  So, for consistency,
I just used my own.

I suppose I could add a Should.notBeConstantValueOf.

WDYT?

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:170: function
isBufferZero(buffer) {
On 2015/07/22 21:01:19, hoch wrote:
> This is identical to:
> Should('myChannelData', f32array).beConstantValueOf(0);
> 
> But I guess you only needed the boolean value, not an actual failure or pass.

Exactly. In some cases all zeros is right but in others a non-zero value is
right.

[hongchan, 2015-07-22 22:30:32]

LGTM with nits.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
File LayoutTests/webaudio/periodicwave-lengths.html (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:87: function
resultShouldBeZero(sampleRate, bigLength, smallLength) {
On 2015/07/22 22:17:48, Raymond Toy wrote:
> On 2015/07/22 21:01:19, hoch wrote:
> > I am pretty sure you've already explored the option of using |Should|
> utilities
> > before inventing this verification function. Can I ask you why you decided
to
> > ditch the |Should| utility here?
> 
> I initially used Should.beConstantValueOf in resultShouldBeZero, but there was
> no corresponding function to use in resultShouldNotBeZero.  So, for
consistency,
> I just used my own.
> 
> I suppose I could add a Should.notBeConstantValueOf.
> 
> WDYT?

Yes, I agree this is good enough for now. However, I think it is beneficial to
have various test utilities with different criteria for the future and that
should be another CL.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:170: function
isBufferZero(buffer) {
On 2015/07/22 22:17:48, Raymond Toy wrote:
> On 2015/07/22 21:01:19, hoch wrote:
> > This is identical to:
> > Should('myChannelData', f32array).beConstantValueOf(0);
> > 
> > But I guess you only needed the boolean value, not an actual failure or
pass.
> 
> Exactly. In some cases all zeros is right but in others a non-zero value is
> right.

Acknowledged.

https://codereview.chromium.org/1180613007/diff/80001/Source/modules/webaudio...
File Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1180613007/diff/80001/Source/modules/webaudio...
Source/modules/webaudio/PeriodicWave.cpp:213:
m_bandLimitedTables.append(table.release());
I am aware that it is not a critical or relevant change in this CL, but do we
need adoptPtr/OwnPtr/.release() thing here?
I believe PeriodicWave is covered by Oilpan already. (GarbageCollectedFinalized)

It seems that the code here is creating an object and pass the ownership to
|m_bandLimitedtables|.

[Raymond Toy, 2015-07-23 15:33:09]

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
File LayoutTests/webaudio/osc-low-freq.html (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:19: var highSampleRate = 192000;
On 2015/07/22 21:01:19, hoch wrote:
> This is unused.

Done.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:25: // 2048 so the lowest frequency is
24000/2048 = 11.78 Hz.
On 2015/07/22 21:01:19, hoch wrote:
> Partials are not linearly arranged over the frequency range, so not sure if
> 11.78Hz is the right value. I might ask you more explanation in person.

I've rewritten the comment. Let me if it's better.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:38: function checkSineResult(result,
freq, rate) {
On 2015/07/22 21:01:19, hoch wrote:
> Perhaps would |checkCosineResult| fit better for the task? Also can we rename
> |rate|?

Done.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/os...
LayoutTests/webaudio/osc-low-freq.html:39: // Compute the SNR between the actual
result and expected cosine wave
On 2015/07/22 21:01:19, hoch wrote:
> How about moving this to the line 37? Also a period at the end would be nice.

Done.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
File LayoutTests/webaudio/periodicwave-lengths.html (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:35: {name : "192khz-test-1",
sampleRate : 192000, bigWave : 8192, smallWave : 2048, verifier :
resultShouldBeNonZero},
On 2015/07/22 21:01:19, hoch wrote:
> nit: JSON style. A space after/before the curly brace. A space only after the
> colon, not before. One item at a line.
> 
> { 
>   name: "192khz-test-1",
>   sampleRate: 192000,
>   bigWave: 8192,
>   smallWave: 2048,
>   verifier: resultShouldBeNonZero
> }
> 
> It would be a bit nicer to read.

Done.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/pe...
LayoutTests/webaudio/periodicwave-lengths.html:79: function prefixMessage
(sampleRate, bigLength, smallLength) {
On 2015/07/22 21:01:19, hoch wrote:
> optional nit: Doesn't look like it's prefixing the message. How about
> 'assembleMessage' or 'generateMessage'?

Renamed to generatePrefix.

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/re...
File LayoutTests/webaudio/resources/audio-testing.js (right):

https://codereview.chromium.org/1180613007/diff/80001/LayoutTests/webaudio/re...
LayoutTests/webaudio/resources/audio-testing.js:437: // "PASS SNR exceeds 100"
On 2015/07/22 21:01:19, hoch wrote:
> We previously agreed to add the example message from the failure.

Done.

[Raymond Toy, 2015-07-23 18:23:05]

https://codereview.chromium.org/1180613007/diff/80001/Source/modules/webaudio...
File Source/modules/webaudio/PeriodicWave.cpp (right):

https://codereview.chromium.org/1180613007/diff/80001/Source/modules/webaudio...
Source/modules/webaudio/PeriodicWave.cpp:213:
m_bandLimitedTables.append(table.release());
On 2015/07/22 22:30:32, hoch wrote:
> I am aware that it is not a critical or relevant change in this CL, but do we
> need adoptPtr/OwnPtr/.release() thing here?
> I believe PeriodicWave is covered by Oilpan already.
(GarbageCollectedFinalized)
> 
> It seems that the code here is creating an object and pass the ownership to
> |m_bandLimitedtables|.

Despite what I said (offline), I don't want to do that here.  It should be a
separate CL to isolate the two independent changes.

[Raymond Toy, 2015-07-23 18:23:34]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2015-07-23 18:23:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/1180613007/#ps120001
(title: "Rebase and use Audit.runTasks()")

[commit-bot: I haz the power, 2015-07-23 18:23:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180613007/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1180613007/120001

[commit-bot: I haz the power, 2015-07-23 19:34:55]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199389