Bug 47518 – Add DelayDSPKernel files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add DelayDSPKernel files

[Chris Rogers]

Created attachment 70495 [details]
Patch

[chris fleizach]

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]

Created attachment 70641 [details]
Patch

[Chris Rogers]

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]

Created attachment 70659 [details]
Patch

[Chris Rogers]

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]

(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]

Starting at line 57 of this patch

[Chris Rogers]

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]

Created attachment 70696 [details]
Patch

[chris fleizach]

Comment on attachment 70696 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 70696 [details]
Patch

Clearing flags on attachment: 70696

Committed r69746: <http://trac.webkit.org/changeset/69746>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.