Bug 79202 – Multi-Channel support in AudioBufferSourceNode

• Component: Web Audio
• Status: RESOLVED FIXED

[Wei James (wistoch)]

Multi-Channel support in AudioBufferSourceNode

[Wei James (wistoch)]

Created attachment 128144 [details]
Patch

[Wei James (wistoch)]

the layout test in this patch depends on the js code mix-testing.js which is included in bug #79017.

[WebKit Review Bot]

Comment on attachment 128144 [details]
Patch

Attachment 128144 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11561427

New failing tests:
webaudio/audiobuffersource-multi-channels.html
webaudio/audiobuffersource-channels.html

[Wei James (wistoch)]

Created attachment 128473 [details]
Patch

[Chris Rogers]

Comment on attachment 128473 [details]
Patch

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

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:314
> +                float* source = m_buffer->getChannelData(i)->data();

313:314 is the part I'm a little bit worried about, since now we need to call these methods inside the inner loop to get the pointers.

Can you please have a look at Raymond's patch where he had a similar situation and was able to use OwnArrayPtr with the two member variables:
m_sourceChannels
m_destinationChannels

then it's easy to convert these to float* sourceChannels[], etc. and use as a simply array of pointers

[Wei James (wistoch)]

Created attachment 128664 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 128473 [details]
Patch

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

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:314
>> +                float* source = m_buffer->getChannelData(i)->data();
> 
> 313:314 is the part I'm a little bit worried about, since now we need to call these methods inside the inner loop to get the pointers.
> 
> Can you please have a look at Raymond's patch where he had a similar situation and was able to use OwnArrayPtr with the two member variables:
> m_sourceChannels
> m_destinationChannels
> 
> then it's easy to convert these to float* sourceChannels[], etc. and use as a simply array of pointers

fixed. thanks

[Chris Rogers]

Comment on attachment 128664 [details]
Patch

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

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:218
> +    OwnArrayPtr<float*> destinationChannels = adoptArrayPtr(new float* [numberOfChannels]);

Instead of allocating and re-allocating every time, create member variables for these which are initialized to the correct size and re-initialized to a new size (if necessary) in setBuffer().

This would be similar to what we do in DynamicsCompressorNode:
http://trac.webkit.org/changeset/108538

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:-227
> -        memset(destinationL, 0, sizeof(float) * destinationFrameOffset);

If destinationL and destinationR are no longer used, then please remove all other references to them (like around lines 204)

[Wei James (wistoch)]

Comment on attachment 128664 [details]
Patch

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

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:218
>> +    OwnArrayPtr<float*> destinationChannels = adoptArrayPtr(new float* [numberOfChannels]);
> 
> Instead of allocating and re-allocating every time, create member variables for these which are initialized to the correct size and re-initialized to a new size (if necessary) in setBuffer().
> 
> This would be similar to what we do in DynamicsCompressorNode:
> http://trac.webkit.org/changeset/108538

Fixed. thanks

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:-227
>> -        memset(destinationL, 0, sizeof(float) * destinationFrameOffset);
> 
> If destinationL and destinationR are no longer used, then please remove all other references to them (like around lines 204)

the line already removed.

[Wei James (wistoch)]

Created attachment 129158 [details]
Patch

[Chris Rogers]

Comment on attachment 129158 [details]
Patch

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

Looks great overall.  Just some minor comments...

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:198
> +    bool channelCountGood = numberOfChannels && numberOfChannels == busNumberOfChannels;

Can we add some checking for m_sourceChannels.size() and m_destinationChannels.size() too?

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:316
> +                *(destination + writeIndex) = narrowPrecisionToFloat(sample);

*(destination + writeIndex)     ->   destination[writeIndex]

> Source/WebCore/webaudio/AudioBufferSourceNode.h:-90
> -    inline bool renderSilenceAndFinishIfNotLooping(float* destinationL, float* destinationR, size_t framesToProcess);

Maybe add a simple comment describing the "index" param

[Wei James (wistoch)]

Comment on attachment 129158 [details]
Patch

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

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:198
>> +    bool channelCountGood = numberOfChannels && numberOfChannels == busNumberOfChannels;
> 
> Can we add some checking for m_sourceChannels.size() and m_destinationChannels.size() too?

added. thanks

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:316
>> +                *(destination + writeIndex) = narrowPrecisionToFloat(sample);
> 
> *(destination + writeIndex)     ->   destination[writeIndex]

fixed. thanks

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:-90
>> -    inline bool renderSilenceAndFinishIfNotLooping(float* destinationL, float* destinationR, size_t framesToProcess);
> 
> Maybe add a simple comment describing the "index" param

added " // Render silence starting from "index" frame in AudioBus." 
thanks

[Chris Rogers]

I also checked the .wav test file "audiobuffer-multi-channels-expected.wav" and noticed that it's two seconds long with one second of silence at the end.  Also the "up-mixing" .wav files have this problem.

Seems like these two lines in mix-testing.js:

var renderLengthSeconds = 2;
var toneLengthSeconds = 1;

[Wei James (wistoch)]

(In reply to comment #13)
> I also checked the .wav test file "audiobuffer-multi-channels-expected.wav" and noticed that it's two seconds long with one second of silence at the end.  Also the "up-mixing" .wav files have this problem.
> 
> Seems like these two lines in mix-testing.js:
> 
> var renderLengthSeconds = 2;
> var toneLengthSeconds = 1;

so you mean we don't need the silence at the end?

[Chris Rogers]

(In reply to comment #14)
> (In reply to comment #13)
> > I also checked the .wav test file "audiobuffer-multi-channels-expected.wav" and noticed that it's two seconds long with one second of silence at the end.  Also the "up-mixing" .wav files have this problem.
> > 
> > Seems like these two lines in mix-testing.js:
> > 
> > var renderLengthSeconds = 2;
> > var toneLengthSeconds = 1;
> 
> so you mean we don't need the silence at the end?

I don't think we need the silence at the end, do we?  It seems wasteful and better to just create test .wav files of 1 second duration with no silence

By the way, thanks for this patch!  I just applied it and tested, and it seems fine (other than the nit about test files).

[Wei James (wistoch)]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > I also checked the .wav test file "audiobuffer-multi-channels-expected.wav" and noticed that it's two seconds long with one second of silence at the end.  Also the "up-mixing" .wav files have this problem.
> > > 
> > > Seems like these two lines in mix-testing.js:
> > > 
> > > var renderLengthSeconds = 2;
> > > var toneLengthSeconds = 1;
> > 
> > so you mean we don't need the silence at the end?
> 
> I don't think we need the silence at the end, do we?  It seems wasteful and better to just create test .wav files of 1 second duration with no silence
> 
> By the way, thanks for this patch!  I just applied it and tested, and it seems fine (other than the nit about test files).

I see. I will modify all the cases. they are borrowed from other layout test cases where there are silence at the end. they may be meaningful for those cases while meaningless here. 

thanks. 

and I met a build issue when adding m_sourceChannels.size(). I checked the source code of OwnArrayPtr and no size() defined and there is no easy way to get the size of it. Since we created both m_sourceChannels and m_destionationChannels at the same place with the same parameter. it is assumed to be the same always.

[Wei James (wistoch)]

also I will fix the MAC build error and then push the patch again. thanks 

cc1plus: warnings being treated as errors
/Volumes/Data/WebKit/Source/WebCore/webaudio/AudioBufferSourceNode.cpp:166: warning: unused parameter 'bus'

[Wei James (wistoch)]

Created attachment 129170 [details]
Patch

[Wei James (wistoch)]

(In reply to comment #18)
> Created an attachment (id=129170) [details]
> Patch

I will fix all the silence issues in test cases in another patch. thanks

[Chris Rogers]

Comment on attachment 129170 [details]
Patch

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

James, sorry for the last minute change, but I noticed an in-efficiency in the inner loop I think we should avoid:

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:310
> +                const float* source = m_sourceChannels[i];

It's inefficient to use member variables m_destinationChannels and m_sourceChannels in the inner loop.  Some extra pointer derefs are needed because of it.

Please create and use local variables "destinationChannels" and "sourceChannels" before the while() loop using m_destinationChannels.get() and  m_sourceChannels.get()

This is kind of similar to how DynamicsCompressorKernel::process() is called using the sourceChannels and destinationChannels params.

[Wei James (wistoch)]

Created attachment 129179 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 129170 [details]
Patch

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

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:310
>> +                const float* source = m_sourceChannels[i];
> 
> It's inefficient to use member variables m_destinationChannels and m_sourceChannels in the inner loop.  Some extra pointer derefs are needed because of it.
> 
> Please create and use local variables "destinationChannels" and "sourceChannels" before the while() loop using m_destinationChannels.get() and  m_sourceChannels.get()
> 
> This is kind of similar to how DynamicsCompressorKernel::process() is called using the sourceChannels and destinationChannels params.

thanks a lot. 

this is fixed. thanks

[WebKit Review Bot]

Comment on attachment 129179 [details]
Patch

Clearing flags on attachment: 129179

Committed r109076: <http://trac.webkit.org/changeset/109076>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.