Bug 226478 - Fix thread safety issues in WaveShaperProcessor
Summary: Fix thread safety issues in WaveShaperProcessor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-31 18:53 PDT by Chris Dumez
Modified: 2021-06-01 08:10 PDT (History)
15 users (show)

See Also:


Attachments
Patch (15.43 KB, patch)
2021-05-31 19:01 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>