[Webkit-unassigned] [Bug 74592] Optimize AudioBufferSourceNode process by avoiding interpolation when pitchRate==1

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=74592





--- Comment #4 from Wei James <james.wei at intel.com>  2011-12-17 01:39:58 PST ---
(From update of attachment 119391)
View in context: https://bugs.webkit.org/attachment.cgi?id=119391&action=review

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:255
>> +    if (!needInterpolation) {
> 
> needInterpolation isn't used anywhere else, so remove it, and change to if (pitchRate == 1).

got it. thanks.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:258
>> +        virtualReadIndex += framesToProcess;
> 
> virtualReadIndex isn't used in the if block below.  It would be nice to move this down near line 298 where it's actually used.

thanks. will modify it.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:261
>> +            memcpy(destinationL, sourceL + readIndex, sizeof(float) * framesToProcess);
> 
> I think sizeof(*sourceL) or sizeof(*destinationL) is better than sizeof(float).  Same applies to other memcpy places below.

ok.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:273
>> +            framesToProcess -= framesToEnd;
> 
> Maybe add ASSERT(framesToProcess >= 0) here?  Currently, this is always true, but this is an important invariant for the memcpy's below so we don't suddenly copy a huge number of samples to the destination.

got it.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:277
>> +                for (unsigned k = 0; k < rounds; ++k) {
> 
> Why not just use a while loop:
> 
> while (framesToProcess > 0) {
>  ...
> |
> 
> instead of the for loop with rounds?

if the deltaFrames is small enough. we have to copy them repeatly. it will be more than once.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:279
>> +                    destinationL += deltaFrames;
> 
> Is this right?  sourceL and startFrame are not modified in the for loop so we keep copying the same data from sourceL+startFrame to consecutive places in destinationL.

it is for the case that the deltaFrame is small enough. We have to copy the frames frome startFrame to endFrame more than once. we can not just wrap around once to get enought data. 

correct me if I misunderstanding the underlying algorithm.

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:298
>>          if (virtualReadIndex >= endFrame) {
> 
> Maybe move this into the else case of the if above (place near line 295)?  I think this condition can only happen in that case.

the case virtualReadIndex == endFrame will hit both blocks. in this case, we also need to finish the playing. correct me if I am wrong. thanks

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:300
>> +            virtualReadIndex -= ((virtualReadIndex - endFrame) / deltaFrames + 1) * deltaFrames;
> 
> Is this correct?  If virtualReadIndex - endFrame = 1, we get (1/deltaFrames + 1)*deltaFrames = deltaFrames + 1 (because virtualReadIndex is a float and ignoring roundoff).  I think we want to subtract just deltaFrames in this case.

the reason for using this statement is to resolve the issue that deltaFrame may be very small. Substracting deltaFrames once might still exceed endFrame. 

I will check the math again for float issue. thanks

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.