Multi-Channel support in AudioBufferSourceNode
Created attachment 128144 [details] Patch
the layout test in this patch depends on the js code mix-testing.js which is included in bug #79017.
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
Created attachment 128473 [details] Patch
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
Created attachment 128664 [details] Patch
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 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 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.
Created attachment 129158 [details] Patch
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 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
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;
(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?
(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).
(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.
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'
Created attachment 129170 [details] Patch
(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 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.
Created attachment 129179 [details] Patch
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 on attachment 129179 [details] Patch Clearing flags on attachment: 129179 Committed r109076: <http://trac.webkit.org/changeset/109076>
All reviewed patches have been landed. Closing bug.