Bug 217921 – [Media in GPU Process] Some WebAudio layout tests generate strange noises

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Peng Liu]

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.

[Radar WebKit Bug Importer]

<rdar://problem/70457775>

[Peng Liu]

Created attachment 411879 [details]
Patch

[Eric Carlson]

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.

[Chris Dumez]

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.

[Peng Liu]

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.

[Peng Liu]

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.

[Peng Liu]

Created attachment 411899 [details]
Patch for landing

[EWS]

Committed r268758: <https://trac.webkit.org/changeset/268758>

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