Make ConvolverNode conform to spec

Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec. This basically means mono source with mono response produces a
mono output.  All other supported configurations produce a stereo output.

Spec: https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations
and https://github.com/WebAudio/web-audio-api/pull/1176

BUG=639330
TEST=convolver-channel-modes.html

Review-Url: https://codereview.chromium.org/2732523003
Cr-Commit-Position: refs/heads/master@{#461140}
Committed: https://chromium.googlesource.com/chromium/src/+/44c111b0f7bd9a9bb044c527162bb5bc7e6c0250

[Raymond Toy, 2017-03-20 20:21:20]

Description was changed from

==========
Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec.

Spec:
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations

BUG=639330
TEST=convolver-channel-modes.html
==========

to

==========
Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec. This basically means mono source with mono response produces a
mono output.  All other supported configurations produce a stereo output.

Spec:
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations

BUG=639330
TEST=convolver-channel-modes.html
==========

[Raymond Toy, 2017-03-24 19:25:19]

Description was changed from

==========
Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec. This basically means mono source with mono response produces a
mono output.  All other supported configurations produce a stereo output.

Spec:
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations

BUG=639330
TEST=convolver-channel-modes.html
==========

to

==========
Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec. This basically means mono source with mono response produces a
mono output.  All other supported configurations produce a stereo output.

Spec:
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations
and https://github.com/WebAudio/web-audio-api/pull/1176

BUG=639330
TEST=convolver-channel-modes.html
==========

[Raymond Toy, 2017-03-24 19:26:02]

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

[Raymond Toy, 2017-03-24 19:26:02]

PTAL

[hongchan, 2017-03-28 16:44:35]

Looks good overall. I think the variable in Reverb.cpp could be better names,
but I leave this up to you.

lgtm

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-cascade.html
(right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-cascade.html:21:
{label: 'test', description: 'Cascaded mono convolvers'},
test => cascade-mono

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-response-2-chan.html
(right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-response-2-chan.html:15:
//
Remove comment tag '//' for this line.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-response-2-chan.html:143:

Why two blank lines?

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-response-4-chan.html
(right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-response-4-chan.html:15:
//
Ditto. Remove the comment tag.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/Convolver/convolver-response-4-chan.html:143:

Ditto. Two blank lines.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:51: addOutput(1);
Why 1? The default channel value is 1 and then can be changed later? Can we have
a comment why this starts from 1? The line below it says the channel count is 2.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:187: if
(channelCount != 2) {
If this cannot be changed, why did we start from 1?

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:220: unsigned
numReverbChannels = m_buffer->numberOfChannels();
numReverbChannels => numberOfReverbChannels

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:221: unsigned
numOutputsDesired =
Ditto. I think it's better to spell out as much as possible.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/Reverb.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:125: // processing
nit: a missing period.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:126: size_t
numResponseChannels = impulseResponseBuffer->numberOfChannels();
This can be unsigned.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:128:
m_convolvers.reserveCapacity(numConvolvers);
Is |numConvolvers| properly named? Isn't it more of |numberOfConvolverChannels|?

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:179: size_t
numReverbChannels = m_numberOfChannels;
I can't make a clear distinction between numReverbChannel, numConvolver,
numberOfChannel. We might want to rethink about these names.

[Raymond Toy, 2017-03-28 17:34:49]

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:51: addOutput(1);
On 2017/03/28 16:44:35, hongchan wrote:
> Why 1? The default channel value is 1 and then can be changed later? Can we
have
> a comment why this starts from 1? The line below it says the channel count is
2.

We start the convolver with 1 output channel.  m_channelCount is really
unrelated because it's the channelCount that goes into the mixing rules for the
input to the convolver.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:187: if
(channelCount != 2) {
On 2017/03/28 16:44:35, hongchan wrote:
> If this cannot be changed, why did we start from 1?

m_channelCount is initialized to 2.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:220: unsigned
numReverbChannels = m_buffer->numberOfChannels();
On 2017/03/28 16:44:35, hongchan wrote:
> numReverbChannels => numberOfReverbChannels

I was following the style in other parts of this file, but
numberOfReverbChannels is better.

[Raymond Toy, 2017-03-28 18:17:21]

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/Reverb.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:128:
m_convolvers.reserveCapacity(numConvolvers);
On 2017/03/28 16:44:35, hongchan wrote:
> Is |numConvolvers| properly named? Isn't it more of
|numberOfConvolverChannels|?

numberOfConvolverChannels is confusing too since the number of responses isn't
the same as this.

numberOfConvolvers is probably closest since it's the size of m_convolvers?

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:179: size_t
numReverbChannels = m_numberOfChannels;
On 2017/03/28 16:44:35, hongchan wrote:
> I can't make a clear distinction between numReverbChannel, numConvolver,
> numberOfChannel. We might want to rethink about these names.

numReverbChannels is the number of channels in the reverb buffer.
numReverbChannels should be renamed to numberOfResponseChannels. (Probably want
to rename m_numberOfChannels to m_numberOfResponseChannels?)

numConvolver is the length of m_convolvers.

I'll rename some of these.

[Raymond Toy, 2017-03-28 18:26:50]

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/Reverb.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:126: size_t
numResponseChannels = impulseResponseBuffer->numberOfChannels();
On 2017/03/28 16:44:35, hongchan wrote:
> This can be unsigned.

This requires changing various other bits of code because it's operated on with
size_t objects too.

[Raymond Toy, 2017-03-28 18:27:22]

PTAL at the updates that includes renaming some things.

[hongchan, 2017-03-28 19:01:44]

Still lgtm. There are some nits, but I'll leave them up to you.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:51: addOutput(1);
On 2017/03/28 17:34:49, Raymond Toy wrote:
> On 2017/03/28 16:44:35, hongchan wrote:
> > Why 1? The default channel value is 1 and then can be changed later? Can we
> have
> > a comment why this starts from 1? The line below it says the channel count
is
> 2.
> 
> We start the convolver with 1 output channel.  m_channelCount is really
> unrelated because it's the channelCount that goes into the mixing rules for
the
> input to the convolver.

Acknowledged.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/ConvolverNode.cpp:187: if
(channelCount != 2) {
On 2017/03/28 17:34:49, Raymond Toy wrote:
> On 2017/03/28 16:44:35, hongchan wrote:
> > If this cannot be changed, why did we start from 1?
> 
> m_channelCount is initialized to 2.

Acknowledged.

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/Reverb.cpp (right):

https://codereview.chromium.org/2732523003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:179: size_t
numReverbChannels = m_numberOfChannels;
On 2017/03/28 18:17:20, Raymond Toy wrote:
> On 2017/03/28 16:44:35, hongchan wrote:
> > I can't make a clear distinction between numReverbChannel, numConvolver,
> > numberOfChannel. We might want to rethink about these names.
> 
> numReverbChannels is the number of channels in the reverb buffer.
> numReverbChannels should be renamed to numberOfResponseChannels. (Probably
want
> to rename m_numberOfChannels to m_numberOfResponseChannels?)
> 
> numConvolver is the length of m_convolvers.

Yeah, this name seems to be problematic.

> 
> I'll rename some of these.

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/Reverb.cpp (right):

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:126: size_t
numResponseChannels = impulseResponseBuffer->numberOfChannels();
How this |numResponseChannels| is different from |numberOfResponseChannels|?

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:127: size_t numConvolvers =
std::max(numResponseChannels, static_cast<size_t>(2));
If |numResponseChannel| is unsigned we don't need the cast for the number 2.

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:128:
m_convolvers.reserveCapacity(numConvolvers);
Then we can do .reserveCapacity(static_cast<size_t>(numConvovlers)) here.

The reason behind is:
- The channel count is generally unsigned.
- The frame size or index is generally size_t.
- "Capacity" is considered to be the latter from my perspective.

This is more of nit - so I'll leave it up to you.

[Raymond Toy, 2017-03-28 20:18:03]

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/Reverb.cpp (right):

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:126: size_t
numResponseChannels = impulseResponseBuffer->numberOfChannels();
On 2017/03/28 19:01:44, hongchan wrote:
> How this |numResponseChannels| is different from |numberOfResponseChannels|?

Good point. I don't think there is anymore and numberOfResponseChannels should
be removed. It can be derived from impulseResponseBuffer.

https://codereview.chromium.org/2732523003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/Reverb.cpp:128:
m_convolvers.reserveCapacity(numConvolvers);
On 2017/03/28 19:01:44, hongchan wrote:
> Then we can do .reserveCapacity(static_cast<size_t>(numConvovlers)) here.
> 
> The reason behind is:
> - The channel count is generally unsigned.
> - The frame size or index is generally size_t.
> - "Capacity" is considered to be the latter from my perspective.
> 
> This is more of nit - so I'll leave it up to you.

This is ok. Just means we also need more casting in line 133 below. Or more
casting in line 131.

[Raymond Toy, 2017-03-30 19:43:08]

PTAL at the last change, removing unneeded parameters

[hongchan, 2017-03-30 19:50:23]

lgtm

[Raymond Toy, 2017-03-31 14:38:28]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2017-03-31 14:38:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 16:20:24]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490971108622120,
"parent_rev": "3e4cfb7160d81365035e927a8a1b63676f718e9e", "commit_rev":
"44c111b0f7bd9a9bb044c527162bb5bc7e6c0250"}

[commit-bot: I haz the power, 2017-03-31 16:21:13]

Description was changed from

==========
Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec. This basically means mono source with mono response produces a
mono output.  All other supported configurations produce a stereo output.

Spec:
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations
and https://github.com/WebAudio/web-audio-api/pull/1176

BUG=639330
TEST=convolver-channel-modes.html
==========

to

==========
Make ConvolverNode conform to spec

Update ConvolverNode to handle inputs and responses according to the
spec. This basically means mono source with mono response produces a
mono output.  All other supported configurations produce a stereo output.

Spec:
https://webaudio.github.io/web-audio-api/#Convolution-channel-configurations
and https://github.com/WebAudio/web-audio-api/pull/1176

BUG=639330
TEST=convolver-channel-modes.html

Review-Url: https://codereview.chromium.org/2732523003
Cr-Commit-Position: refs/heads/master@{#461140}
Committed:
https://chromium.googlesource.com/chromium/src/+/44c111b0f7bd9a9bb044c527162b...
==========

[commit-bot: I haz the power, 2017-03-31 16:21:14]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/44c111b0f7bd9a9bb044c527162b...