Bug 74693 – Optimize with memcpy instead of copying frame by frame in Realtimeanalyser::doFFTAnalysis

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Wei James (wistoch)]

Optimize with memcpy instead of copying frame by frame in Realtimeanalyser::doFFTAnalysis

[Wei James (wistoch)]

Created attachment 119579 [details]
Patch

[Raymond Toy]

Comment on attachment 119579 [details]
Patch

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

> Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
> +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(float) * (fftSize - writeIndex));

I think sizeof(*tempP) is better than sizeof(float).

[Wei James (wistoch)]

Created attachment 119717 [details]
Patch

[Wei James (wistoch)]

(In reply to comment #2)
> (From update of attachment 119579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119579&action=review
> 
> > Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
> > +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(float) * (fftSize - writeIndex));
> 
> I think sizeof(*tempP) is better than sizeof(float).

thanks for the review. patch updated.

[Raymond Toy]

Comment on attachment 119717 [details]
Patch

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

> Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
> +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(*tempP) * (fftSize - writeIndex));

Does this handle wrapping the index as the original code did?  It seems to me that writeIndex (m_writeIndex) could be as large as InputBufferSize - 1, so the memcpy might be reading past the end of the inputBuffer.

[Wei James (wistoch)]

Comment on attachment 119717 [details]
Patch

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

>> Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
>> +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(*tempP) * (fftSize - writeIndex));
> 
> Does this handle wrapping the index as the original code did?  It seems to me that writeIndex (m_writeIndex) could be as large as InputBufferSize - 1, so the memcpy might be reading past the end of the inputBuffer.

this memcpy is under the if(writeIndex < fftSize) statement. the case you mentioned is under else statement. thanks

[Wei James (wistoch)]

(In reply to comment #6)
> (From update of attachment 119717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119717&action=review
> 
> >> Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
> >> +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(*tempP) * (fftSize - writeIndex));
> > 
> > Does this handle wrapping the index as the original code did?  It seems to me that writeIndex (m_writeIndex) could be as large as InputBufferSize - 1, so the memcpy might be reading past the end of the inputBuffer.
> 
> this memcpy is under the if(writeIndex < fftSize) statement. the case you mentioned is under else statement. thanks

raymond, does it answer your question? or do you think the patch still need revise? thanks

[Raymond Toy]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 119717 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=119717&action=review
> > 
> > >> Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
> > >> +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(*tempP) * (fftSize - writeIndex));
> > > 
> > > Does this handle wrapping the index as the original code did?  It seems to me that writeIndex (m_writeIndex) could be as large as InputBufferSize - 1, so the memcpy might be reading past the end of the inputBuffer.
> > 
> > this memcpy is under the if(writeIndex < fftSize) statement. the case you mentioned is under else statement. thanks
> 
> raymond, does it answer your question? or do you think the patch still need revise? thanks

Sorry.  It seems that my reply from yesterday got lost some how.  Yes, you've answered my question.  Wrapping is handled.  (I had convinced myself in the first patch that it was correct, but promptly forgot in the second patch review.)

[Wei James (wistoch)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 119717 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=119717&action=review
> > > 
> > > >> Source/WebCore/webaudio/RealtimeAnalyser.cpp:162
> > > >> +        memcpy(tempP, inputBuffer + writeIndex - fftSize + InputBufferSize, sizeof(*tempP) * (fftSize - writeIndex));
> > > > 
> > > > Does this handle wrapping the index as the original code did?  It seems to me that writeIndex (m_writeIndex) could be as large as InputBufferSize - 1, so the memcpy might be reading past the end of the inputBuffer.
> > > 
> > > this memcpy is under the if(writeIndex < fftSize) statement. the case you mentioned is under else statement. thanks
> > 
> > raymond, does it answer your question? or do you think the patch still need revise? thanks
> 
> Sorry.  It seems that my reply from yesterday got lost some how.  Yes, you've answered my question.  Wrapping is handled.  (I had convinced myself in the first patch that it was correct, but promptly forgot in the second patch review.)

thanks. :)

[Kenneth Russell]

If Chris Rogers gives an unofficial LGTM on this I'd be happy to r+ it.

[Chris Rogers]

Comment on attachment 119717 [details]
Patch

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

Looks good if you fix the ChangeLog

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This line should be taken out.

[Wei James (wistoch)]

Created attachment 121371 [details]
Patch

[Wei James (wistoch)]

(In reply to comment #11)
> (From update of attachment 119717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119717&action=review
> 
> Looks good if you fix the ChangeLog
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> This line should be taken out.

thanks, patch updated to remove this line. thanks

[Kenneth Russell]

Comment on attachment 121371 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 121371 [details]
Patch

Clearing flags on attachment: 121371

Committed r104265: <http://trac.webkit.org/changeset/104265>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.