Bug 79017 – Add multi channels support in AudioBus

• Component: Web Audio
• Status: RESOLVED FIXED

[Wei James (wistoch)]

Add multi channels support in AudioBus and AudioBufferSourceNode

[Wei James (wistoch)]

Created attachment 127780 [details]
Patch

[Chris Rogers]

Comment on attachment 127780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127780&action=review

Hi James, I have some initial comments and ideas for the layout tests.  I think the AudioBufferSourceNode portion of this patch could be separated out into a later patch, and just focus on the AudioBus parts and the tests for now.

> Source/WebCore/platform/audio/AudioBus.cpp:237
> +        // Handle 5.1 -> mono case, copy center channel into mono.

See comment about FIXME for 5.1 -> stereo -- we at least need a FIXME here
Luckily it shouldn't be a very common case...

> Source/WebCore/platform/audio/AudioBus.cpp:240
> +        // Default down mixing handling, just match the source channels with the first avaialbe destination channels. 

typo: avaialbe -> available

> Source/WebCore/platform/audio/AudioBus.cpp:241
> +        // 5.1 -> stereo case covered here. 

Please add a FIXME here because a good 5.1 -> stereo downmix will not simply throw away the other channels.
We're investigating what would be a good standard reference for the mixdown coefficients (how much to blend in the surround and mono channels...)
So could you please write up a new bug for this and put the bug URL in the FIXME comment here?

> Source/WebCore/platform/audio/AudioBus.cpp:245
> +        // Default up mixing handling, just match the destination channels with the first avaialbe source channels. 

typo: avaialbe

> Source/WebCore/platform/audio/AudioBus.cpp:249
> +        for (unsigned i = numberOfSourceChannels; i< numberOfDestinationChannels; ++i)

nit: spacing before '<'

> Source/WebCore/platform/audio/AudioBus.cpp:286
> +        // Handle 5.1 -> mono case, sum center channel into mono.

See comment about FIXME for 5.1 -> stereo -- we at least need a FIXME here
Luckily it shouldn't be a very common case...

> LayoutTests/webaudio/audiobuffersource-multi-channels.html:36
> +    context = new webkitAudioContext();

To really do proper testing here, we'll need to use an "OfflineAudioContext".  Your current approach is not testing that the up-mixing and down-mixing results are correct.

There are two main approaches to audio testing we've taken with the Web Audio API:

1) Render .wav files and compare with reference .wav files.  See, for example, "mixing.html" which is a simple example which just plays two sounds at the same time and makes sure they're mixed correctly.
"gain.html" is another example like this where you can actually listen to the expected rendered result in the "gain-expected.wav"
2) A lower-level approach we often use is to not generate .wav files, but instead inspect the rendered AudioBuffer PCM audio data directly in JavaScript and verify that it is correct.
For example, "delay.html" and "delay-testing.js" play a pure sin() wave through a DelayNode set to delay by an exact time, then verify that the rendered audio stream matches what we expect.

Either approach could make sense for testing up-mixing and down-mixing, but maybe approach (1) is the best.
We don't need to test all of the combinations, but we *should* at least test the important ones:

Up mixing:
1 -> 2
1 -> 5.1
2 -> 5.1

The OfflineAudioContext is created with the desired number of "destination" channels.

For example, to test this up-mixing, we could generate two test signals (directly in JavaScript -- see "delay.html" and "delay-testing.js" as an example):
mono: 1 second sin() wave @ 440Hz
stereo: 1 second, left channel sin() wave @ 440Hz, right channel @ 880Hz

Then the expected .wav files we could actually listen to (for the stereo case), or inspect in a wave editor 

Down mixing:
2 -> 1
5.1 -> 2

It's best to wait until we're actually doing the correct downmix before writing these tests and just leave the FIXME comments in AudioBus.cpp as I suggest

[Wei James (wistoch)]

Created attachment 128128 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 127780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127780&action=review

>> Source/WebCore/platform/audio/AudioBus.cpp:237
>> +        // Handle 5.1 -> mono case, copy center channel into mono.
> 
> See comment about FIXME for 5.1 -> stereo -- we at least need a FIXME here
> Luckily it shouldn't be a very common case...

added.

>> Source/WebCore/platform/audio/AudioBus.cpp:240
>> +        // Default down mixing handling, just match the source channels with the first avaialbe destination channels. 
> 
> typo: avaialbe -> available

fixed.

>> Source/WebCore/platform/audio/AudioBus.cpp:241
>> +        // 5.1 -> stereo case covered here. 
> 
> Please add a FIXME here because a good 5.1 -> stereo downmix will not simply throw away the other channels.
> We're investigating what would be a good standard reference for the mixdown coefficients (how much to blend in the surround and mono channels...)
> So could you please write up a new bug for this and put the bug URL in the FIXME comment here?

added.

>> Source/WebCore/platform/audio/AudioBus.cpp:245
>> +        // Default up mixing handling, just match the destination channels with the first avaialbe source channels. 
> 
> typo: avaialbe

fixed.

>> Source/WebCore/platform/audio/AudioBus.cpp:249
>> +        for (unsigned i = numberOfSourceChannels; i< numberOfDestinationChannels; ++i)
> 
> nit: spacing before '<'

fixed.

>> Source/WebCore/platform/audio/AudioBus.cpp:286
>> +        // Handle 5.1 -> mono case, sum center channel into mono.
> 
> See comment about FIXME for 5.1 -> stereo -- we at least need a FIXME here
> Luckily it shouldn't be a very common case...

added.

>> LayoutTests/webaudio/audiobuffersource-multi-channels.html:36
>> +    context = new webkitAudioContext();
> 
> To really do proper testing here, we'll need to use an "OfflineAudioContext".  Your current approach is not testing that the up-mixing and down-mixing results are correct.
> 
> There are two main approaches to audio testing we've taken with the Web Audio API:
> 
> 1) Render .wav files and compare with reference .wav files.  See, for example, "mixing.html" which is a simple example which just plays two sounds at the same time and makes sure they're mixed correctly.
> "gain.html" is another example like this where you can actually listen to the expected rendered result in the "gain-expected.wav"
> 2) A lower-level approach we often use is to not generate .wav files, but instead inspect the rendered AudioBuffer PCM audio data directly in JavaScript and verify that it is correct.
> For example, "delay.html" and "delay-testing.js" play a pure sin() wave through a DelayNode set to delay by an exact time, then verify that the rendered audio stream matches what we expect.
> 
> Either approach could make sense for testing up-mixing and down-mixing, but maybe approach (1) is the best.
> We don't need to test all of the combinations, but we *should* at least test the important ones:
> 
> Up mixing:
> 1 -> 2
> 1 -> 5.1
> 2 -> 5.1
> 
> The OfflineAudioContext is created with the desired number of "destination" channels.
> 
> For example, to test this up-mixing, we could generate two test signals (directly in JavaScript -- see "delay.html" and "delay-testing.js" as an example):
> mono: 1 second sin() wave @ 440Hz
> stereo: 1 second, left channel sin() wave @ 440Hz, right channel @ 880Hz
> 
> Then the expected .wav files we could actually listen to (for the stereo case), or inspect in a wave editor 
> 
> Down mixing:
> 2 -> 1
> 5.1 -> 2
> 
> It's best to wait until we're actually doing the correct downmix before writing these tests and just leave the FIXME comments in AudioBus.cpp as I suggest

I have removed AudioBufferSourceNode part and added three test cases with the approach (1). 

please review. thanks

[Chris Rogers]

Comment on attachment 128128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128128&action=review

Great tests!  I just have a few comments:

> LayoutTests/webaudio/resources/audio-testing.js:49
> +             writeInt16(sample, a, offset);

nit: indentation off by one space

> LayoutTests/webaudio/resources/mix-testing.js:4
> +var toneLengthSeconds = 2;

It seems a little bit wasteful to generate audio files which have two seconds of silence at the end.

How about making both of these values equal and shorten to 1 second which should be sufficient for testing purposes.

> LayoutTests/webaudio/resources/mix-testing.js:5
> +

Please add short comment here describing the relationship of the frequency of each channel.

> LayoutTests/webaudio/resources/mix-testing.js:6
> +function createToneBuffer(context, frequency, numberOfCycles, sampleRate, numberOfChannels) {

This is up to your judgement, but maybe it would be simpler to just pass in "duration" directly instead of "numberOfCycles".
I realize this code was probably taken from another test, where "numberOfCycles" might have been important - but for these mixing tests, maybe not needed...

[Raymond Toy]

Comment on attachment 128128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128128&action=review

> Source/WebCore/platform/audio/AudioBus.cpp:229
> +        // Handle mono -> 5.1 case, copy mono channel to center. 

Is copying the mono channel to center the right thing?  (I don't know.  I'm just asking.)  If not, please add a FIXME comment like you did on the other cases.

>> LayoutTests/webaudio/resources/mix-testing.js:6
>> +function createToneBuffer(context, frequency, numberOfCycles, sampleRate, numberOfChannels) {
> 
> This is up to your judgement, but maybe it would be simpler to just pass in "duration" directly instead of "numberOfCycles".
> I realize this code was probably taken from another test, where "numberOfCycles" might have been important - but for these mixing tests, maybe not needed...

Is the sampleRate parameter? The context has the sample rate.

[Chris Rogers]

(In reply to comment #6)
> (From update of attachment 128128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128128&action=review
> 
> > Source/WebCore/platform/audio/AudioBus.cpp:229
> > +        // Handle mono -> 5.1 case, copy mono channel to center. 
> 
> Is copying the mono channel to center the right thing?  (I don't know.  I'm just asking.)  If not, please add a FIXME comment like you did on the other cases.

This is currently what the specification says, so FIXME should not be necessary.

[Wei James (wistoch)]

Comment on attachment 128128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128128&action=review

>> LayoutTests/webaudio/resources/audio-testing.js:49
>> +             writeInt16(sample, a, offset);
> 
> nit: indentation off by one space

fixed. thanks

>> LayoutTests/webaudio/resources/mix-testing.js:4
>> +var toneLengthSeconds = 2;
> 
> It seems a little bit wasteful to generate audio files which have two seconds of silence at the end.
> 
> How about making both of these values equal and shorten to 1 second which should be sufficient for testing purposes.

fixed. thanks

>> LayoutTests/webaudio/resources/mix-testing.js:5
>> +
> 
> Please add short comment here describing the relationship of the frequency of each channel.

added. thanks

>>> LayoutTests/webaudio/resources/mix-testing.js:6
>>> +function createToneBuffer(context, frequency, numberOfCycles, sampleRate, numberOfChannels) {
>> 
>> This is up to your judgement, but maybe it would be simpler to just pass in "duration" directly instead of "numberOfCycles".
>> I realize this code was probably taken from another test, where "numberOfCycles" might have been important - but for these mixing tests, maybe not needed...
> 
> Is the sampleRate parameter? The context has the sample rate.

change the function to createToneBuffer(context, frequency, duration, numberOfChannels)

[Wei James (wistoch)]

Created attachment 128356 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128356 [details]
Patch

Clearing flags on attachment: 128356

Committed r108604: <http://trac.webkit.org/changeset/108604>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.