Bug 47518

Summary: Add DelayDSPKernel files
Product: WebKit Reporter: Chris Rogers <crogers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, cmarrin, commit-queue, dglazkov, eric.carlson, jamesr, jer.noble, kbr, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Chris Rogers
Reported 2010-10-11 16:51:24 PDT
Add DelayDSPKernel files
Attachments
Patch (7.93 KB, patch)
2010-10-11 16:52 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Patch (8.24 KB, patch)
2010-10-13 11:47 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Patch (8.26 KB, patch)
2010-10-13 14:20 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Patch (8.38 KB, patch)
2010-10-13 18:01 PDT, Chris Rogers 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Rogers
Comment 1 2010-10-11 16:52:26 PDT
Created attachment 70495 [details] Patch
chris fleizach
Comment 2 2010-10-13 00:58:58 PDT
Comment on attachment 70495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70495&action=review > WebCore/webaudio/DelayDSPKernel.cpp:43 > + , m_buffer(static_cast<size_t>(processor->sampleRate() * DefaultMaxDelayTime)) do you need to worry about the case where someone calls DelayDSPKernel(0) > WebCore/webaudio/DelayDSPKernel.h:46 > + void setDelayFrames(double numberOfFrames); this implementation looks like it should be in the header (it's one line in the cpp file) > WebCore/webaudio/DelayDSPKernel.h:48 > +protected: why protected and not private
Chris Rogers
Comment 3 2010-10-13 11:47:07 PDT
Created attachment 70641 [details] Patch
Chris Rogers
Comment 4 2010-10-13 11:48:15 PDT
Comment on attachment 70495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70495&action=review >> WebCore/webaudio/DelayDSPKernel.cpp:43 >> + , m_buffer(static_cast<size_t>(processor->sampleRate() * DefaultMaxDelayTime)) > > do you need to worry about the case where someone calls DelayDSPKernel(0) Yes, good point. I've added extra checks for negative and zero values in this constructor. >> WebCore/webaudio/DelayDSPKernel.h:46 >> + void setDelayFrames(double numberOfFrames); > > this implementation looks like it should be in the header (it's one line in the cpp file) FIXED >> WebCore/webaudio/DelayDSPKernel.h:48 >> +protected: > > why protected and not private FIXED
Chris Rogers
Comment 5 2010-10-13 14:20:45 PDT
Created attachment 70659 [details] Patch
Chris Rogers
Comment 6 2010-10-13 14:22:18 PDT
The last re-factoring introduced a new bug causing an uninitialized array - uploaded one more patch calling: m_buffer.zero() after m_buffer.resize()
chris fleizach
Comment 7 2010-10-13 17:13:11 PDT
(In reply to comment #4) > (From update of attachment 70495 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=70495&action=review > > >> WebCore/webaudio/DelayDSPKernel.cpp:43 > >> + , m_buffer(static_cast<size_t>(processor->sampleRate() * DefaultMaxDelayTime)) > > > > do you need to worry about the case where someone calls DelayDSPKernel(0) > > Yes, good point. I've added extra checks for negative and zero values in this constructor. > In both patches I don't see the extra checks.. Can you point those out
Chris Rogers
Comment 8 2010-10-13 17:50:28 PDT
Starting at line 57 of this patch
Chris Rogers
Comment 9 2010-10-13 17:52:06 PDT
Sorry, Chris, I now see you meant "NULL" for the pointer and not zero for the other constructor. I'll upload a new patch...
Chris Rogers
Comment 10 2010-10-13 18:01:27 PDT
Created attachment 70696 [details] Patch
chris fleizach
Comment 11 2010-10-13 18:11:15 PDT
Comment on attachment 70696 [details] Patch r=me
WebKit Commit Bot
Comment 12 2010-10-14 00:10:44 PDT
Comment on attachment 70696 [details] Patch Clearing flags on attachment: 70696 Committed r69746: <http://trac.webkit.org/changeset/69746>
WebKit Commit Bot
Comment 13 2010-10-14 00:10:50 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.