Bug 217921 - [Media in GPU Process] Some WebAudio layout tests generate strange noises
Summary: [Media in GPU Process] Some WebAudio layout tests generate strange noises
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-19 14:58 PDT by Peng Liu
Modified: 2020-10-20 13:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.05 KB, patch)
2020-10-20 10:01 PDT, Peng Liu
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (8.60 KB, patch)
2020-10-20 12:35 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

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