Bug 226478

Summary: Fix thread safety issues in WaveShaperProcessor
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web AudioAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, jer.noble, kondapallykalyan, peng.liu6, philipj, sam, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Chris Dumez 2021-05-31 18:53:08 PDT
Adopt thread safety analysis annotations in WaveShaperProcessor and fix bugs found by clang.
Comment 1 Chris Dumez 2021-05-31 19:01:03 PDT
Created attachment 430223 [details]
Patch
Comment 2 youenn fablet 2021-06-01 05:05:55 PDT
Comment on attachment 430223 [details]
Patch

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

> Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:96
> +    for (size_t i = 0; i < m_kernels.size(); ++i)

auto?

> Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:97
> +        static_cast<WaveShaperDSPKernel&>(*m_kernels[i]).process(source->channel(i)->data(), destination->channel(i)->mutableData(), framesToProcess);

Why do we need static_cast<WaveShaperDSPKernel&> now while we did not in the past?
Comment 3 Chris Dumez 2021-06-01 07:39:45 PDT
Comment on attachment 430223 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:96
>> +    for (size_t i = 0; i < m_kernels.size(); ++i)
> 
> auto?

What will be the type for auto i = 0? I want the type to be size_t to match kernels.size().

>> Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:97
>> +        static_cast<WaveShaperDSPKernel&>(*m_kernels[i]).process(source->channel(i)->data(), destination->channel(i)->mutableData(), framesToProcess);
> 
> Why do we need static_cast<WaveShaperDSPKernel&> now while we did not in the past?

No, this is just an optimization to avoid a virtual function call.
Comment 4 Chris Dumez 2021-06-01 07:46:44 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 430223 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430223&action=review
> 
> >> Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:96
> >> +    for (size_t i = 0; i < m_kernels.size(); ++i)
> > 
> > auto?
> 
> What will be the type for auto i = 0? I want the type to be size_t to match
> kernels.size().

auto i = 0; would resolve auto to int so it wouldn't do what I want.

> 
> >> Source/WebCore/Modules/webaudio/WaveShaperProcessor.cpp:97
> >> +        static_cast<WaveShaperDSPKernel&>(*m_kernels[i]).process(source->channel(i)->data(), destination->channel(i)->mutableData(), framesToProcess);
> > 
> > Why do we need static_cast<WaveShaperDSPKernel&> now while we did not in the past?
> 
> No, this is just an optimization to avoid a virtual function call.
Comment 5 EWS 2021-06-01 08:09:55 PDT
Committed r278307 (238344@main): <https://commits.webkit.org/238344@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430223 [details].
Comment 6 Radar WebKit Bug Importer 2021-06-01 08:10:20 PDT
<rdar://problem/78714897>