Bug 217921

Summary: [Media in GPU Process] Some WebAudio layout tests generate strange noises
Product: WebKit Reporter: Peng Liu <peng.liu6>
Component: Web AudioAssignee: Peng Liu <peng.liu6>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cdumez, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
eric.carlson: review+
Patch for landing none

Description Peng Liu 2020-10-19 14:58:18 PDT 
When running some WebAudio layout tests, we can hear strange noises, which does not happen before r268632. I can reproduce the issue by keep reloading
https://webaudioapi.com/samples/oscillator/

The issue only happens on Mac.
Comment 1 Radar WebKit Bug Importer 2020-10-19 14:58:53 PDT 
<rdar://problem/70457775>
Comment 2 Peng Liu 2020-10-20 10:01:32 PDT 
Created attachment 411879 [details]
Patch
Comment 3 Eric Carlson 2020-10-20 10:10:27 PDT 
Comment on attachment 411879 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:120
> +        if (m_isPlaying == isPlaying)
> +            return;

Is this necessary?

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:161
> +            uint64_t start = 0;
> +            uint64_t end = 0;
> +            m_ringBuffer->getCurrentFrameBounds(start, end);

getCurrentFrameBounds always updates start and end, so I don't think they need to be initialized.
Comment 4 Chris Dumez 2020-10-20 10:10:55 PDT 
Comment on attachment 411879 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:119
> +        if (m_isPlaying == isPlaying)

This check does not seem worth it for a simple boolean assignment.
Comment 5 Peng Liu 2020-10-20 10:20:54 PDT 
Comment on attachment 411879 [details]
Patch

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

>> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:119
>> +        if (m_isPlaying == isPlaying)
> 
> This check does not seem worth it for a simple boolean assignment.

Agree, will fix it. Thanks!

>> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:120
>> +            return;
> 
> Is this necessary?

No, it is not. :-)

>> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:161
>> +            m_ringBuffer->getCurrentFrameBounds(start, end);
> 
> getCurrentFrameBounds always updates start and end, so I don't think they need to be initialized.

Agree, will fix it.
Comment 6 Peng Liu 2020-10-20 12:26:42 PDT 
Comment on attachment 411879 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp:192
> +    Lock m_isPlayingLock;

Jer pointed out that the lock seems not necessary and it may block the main thread. So we discussed with Chris to confirm that.
I will remove this lock.
Comment 7 Peng Liu 2020-10-20 12:35:12 PDT 
Created attachment 411899 [details]
Patch for landing
Comment 8 EWS 2020-10-20 13:42:12 PDT 
Committed r268758: <https://trac.webkit.org/changeset/268758>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411899 [details].