Bug 79202

Summary: Multi-Channel support in AudioBufferSourceNode
Product: WebKit Reporter: Wei James (wistoch) <james.wei>
Component: Web AudioAssignee: Wei James (wistoch) <james.wei>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 79017    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Wei James (wistoch) 2012-02-21 23:40:53 PST
Multi-Channel support in AudioBufferSourceNode
Comment 1 Wei James (wistoch) 2012-02-21 23:42:35 PST
Created attachment 128144 [details]
Patch
Comment 2 Wei James (wistoch) 2012-02-21 23:45:37 PST
the layout test in this patch depends on the js code mix-testing.js which is included in bug #79017.
Comment 3 WebKit Review Bot 2012-02-22 02:05:20 PST
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
Comment 4 Wei James (wistoch) 2012-02-23 06:17:35 PST
Created attachment 128473 [details]
Patch
Comment 5 Chris Rogers 2012-02-23 11:19:30 PST
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
Comment 6 Wei James (wistoch) 2012-02-23 23:18:16 PST
Created attachment 128664 [details]
Patch
Comment 7 Wei James (wistoch) 2012-02-23 23:19:37 PST
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
Comment 8 Chris Rogers 2012-02-27 17:12:39 PST
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)
Comment 9 Wei James (wistoch) 2012-02-27 18:29:43 PST
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.
Comment 10 Wei James (wistoch) 2012-02-27 18:30:09 PST
Created attachment 129158 [details]
Patch
Comment 11 Chris Rogers 2012-02-27 18:45:44 PST
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
Comment 12 Wei James (wistoch) 2012-02-27 18:56:00 PST
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
Comment 13 Chris Rogers 2012-02-27 18:56:47 PST
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;
Comment 14 Wei James (wistoch) 2012-02-27 19:04:54 PST
(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?
Comment 15 Chris Rogers 2012-02-27 19:13:14 PST
(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).
Comment 16 Wei James (wistoch) 2012-02-27 19:21:55 PST
(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.
Comment 17 Wei James (wistoch) 2012-02-27 19:23:53 PST
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'
Comment 18 Wei James (wistoch) 2012-02-27 19:30:21 PST
Created attachment 129170 [details]
Patch
Comment 19 Wei James (wistoch) 2012-02-27 19:32:37 PST
(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
Comment 20 Chris Rogers 2012-02-27 20:27:06 PST
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.
Comment 21 Wei James (wistoch) 2012-02-27 20:44:37 PST
Created attachment 129179 [details]
Patch
Comment 22 Wei James (wistoch) 2012-02-27 20:46:17 PST
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
Comment 23 WebKit Review Bot 2012-02-27 22:04:02 PST
Comment on attachment 129179 [details]
Patch

Clearing flags on attachment: 129179

Committed r109076: <http://trac.webkit.org/changeset/109076>
Comment 24 WebKit Review Bot 2012-02-27 22:04:16 PST
All reviewed patches have been landed.  Closing bug.