Bug 228589 - [Catalina][GPUP] Some API tests fail after GPU Process features are enabled
Summary: [Catalina][GPUP] Some API tests fail after GPU Process features are enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-29 09:38 PDT by Peng Liu
Modified: 2021-08-04 14:36 PDT (History)
16 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2021-07-29 09:45 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
[fast-cq] Patch (5.12 KB, patch)
2021-07-29 10:56 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (22.19 KB, patch)
2021-08-03 15:59 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (22.90 KB, patch)
2021-08-03 16:16 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (24.03 KB, patch)
2021-08-03 23:23 PDT, Peng Liu
jer.noble: review+
Details | Formatted Diff | Diff
Revise the patch based on Jer's comments (25.21 KB, patch)
2021-08-04 12:11 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 2021-07-29 09:38:52 PDT
https://ews-build.webkit.org/#/builders/3/builds/52604


    TestWebKitAPI.PreferredAudioBufferSize.AudioElement
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PreferredAudioBufferSize.mm:67
        Expected equality of these values:
          expectedAudioBufferSize
            Which is: 4096
          preferredAudioBufferSize()
            Which is: 512
        

    TestWebKitAPI.PreferredAudioBufferSize.AudioWithWebAudio
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PreferredAudioBufferSize.mm:67
        Expected equality of these values:
          expectedAudioBufferSize
            Which is: 128
          preferredAudioBufferSize()
            Which is: 512
        

    TestWebKitAPI.PreferredAudioBufferSize.VideoOnly
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PreferredAudioBufferSize.mm:67
        Expected equality of these values:
          expectedAudioBufferSize
            Which is: 4096
          preferredAudioBufferSize()
            Which is: 512
        

    TestWebKitAPI.PreferredAudioBufferSize.VideoWithAudio
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PreferredAudioBufferSize.mm:67
        Expected equality of these values:
          expectedAudioBufferSize
            Which is: 4096
          preferredAudioBufferSize()
            Which is: 512
        

    TestWebKitAPI.PreferredAudioBufferSize.VideoWithAudioAndWebAudio
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PreferredAudioBufferSize.mm:67
        Expected equality of these values:
          expectedAudioBufferSize
            Which is: 128
          preferredAudioBufferSize()
            Which is: 512
        

    TestWebKitAPI.PreferredAudioBufferSize.WebAudio
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/PreferredAudioBufferSize.mm:67
        Expected equality of these values:
          expectedAudioBufferSize
            Which is: 128
          preferredAudioBufferSize()
            Which is: 512
        

    TestWebKitAPI.WebKit.AudioBufferSize
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/AudioBufferSize.mm:95
        Expected: (bufferSizeWithWebAudio) < (bufferSizeCapturingAudio), actual: 512 vs 0
        

    TestWebKitAPI.WebKit2.CrashGPUProcessAfterApplyingConstraints
        
        /Volumes/Data/worker/macOS-Catalina-Release-Build-EWS/build/Tools/TestWebKitAPI/Tests/WebKit/GetUserMedia.mm:82
        Expected equality of these values:
          @"PASS"
            Which is: "PASS"
          [message body]
            Which is: "FAIL checkConstraints, width is not 320 but 640"
Comment 1 Peng Liu 2021-07-29 09:45:12 PDT
Created attachment 434529 [details]
Patch
Comment 2 Peng Liu 2021-07-29 10:56:19 PDT
Created attachment 434535 [details]
[fast-cq] Patch
Comment 3 EWS 2021-07-29 12:05:00 PDT
Committed r280439 (240077@main): <https://commits.webkit.org/240077@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434535 [details].
Comment 4 Radar WebKit Bug Importer 2021-07-29 12:06:05 PDT
<rdar://problem/81283879>
Comment 5 Peng Liu 2021-07-29 12:12:34 PDT
The bug is not resolved yet.
Comment 6 Peng Liu 2021-08-03 15:59:25 PDT
Created attachment 434869 [details]
WIP patch
Comment 7 Peng Liu 2021-08-03 16:16:41 PDT
Created attachment 434870 [details]
WIP patch
Comment 8 Peng Liu 2021-08-03 16:18:59 PDT
Bug 228759 and 228760 will track the fixes for the remaining test failures.
Comment 9 Peng Liu 2021-08-03 23:23:47 PDT
Created attachment 434887 [details]
Patch
Comment 10 Jer Noble 2021-08-04 10:12:31 PDT
Comment on attachment 434887 [details]
Patch

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

r=me, with nits:

> Source/WebCore/html/HTMLMediaElement.cpp:3813
> -void HTMLMediaElement::hardwareMutedStateDidChange(AudioSession* session)
> +void HTMLMediaElement::hardwareMutedStateDidChange(const AudioSession* session)

If we're changing the signature here, can we change this to be a `const AudioSession&` rather than a pointer?

> Source/WebCore/html/HTMLMediaElement.h:885
> -    void hardwareMutedStateDidChange(AudioSession*) final;
> +    void hardwareMutedStateDidChange(const AudioSession*) final;

Ditto.

> Source/WebCore/platform/audio/mac/AudioSessionMac.mm:140
> +void AudioSessionMac::handleSampleRateChange() const
> +{
> +    for (auto& observer : m_configurationChangeObservers)
> +        observer.sampleRateDidChange(this);
> +}

I _think_ you want to use m_configurationChangeObservers.forEach() or for(... : copyToVector(m_configurationChangeObservers)). The default iterator will skip empty observers, but won't remove them.

> Source/WebCore/platform/audio/mac/AudioSessionMac.mm:188
> +    for (auto& observer : m_configurationChangeObservers)
> +        observer.bufferSizeDidChange(this);

Ditto.

> Source/WebCore/platform/audio/mac/AudioSessionMac.mm:490
> +    for (auto& observer : m_configurationChangeObservers)
> +        observer.hardwareMutedStateDidChange(this);

Ditto.
Comment 11 Peng Liu 2021-08-04 12:11:50 PDT
Created attachment 434923 [details]
Revise the patch based on Jer's comments
Comment 12 Peng Liu 2021-08-04 12:14:54 PDT
Comment on attachment 434887 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:3813
>> +void HTMLMediaElement::hardwareMutedStateDidChange(const AudioSession* session)
> 
> If we're changing the signature here, can we change this to be a `const AudioSession&` rather than a pointer?

Yes! Done.

>> Source/WebCore/html/HTMLMediaElement.h:885
>> +    void hardwareMutedStateDidChange(const AudioSession*) final;
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/audio/mac/AudioSessionMac.mm:140
>> +}
> 
> I _think_ you want to use m_configurationChangeObservers.forEach() or for(... : copyToVector(m_configurationChangeObservers)). The default iterator will skip empty observers, but won't remove them.

Good point! Fixed.

>> Source/WebCore/platform/audio/mac/AudioSessionMac.mm:188
>> +        observer.bufferSizeDidChange(this);
> 
> Ditto.

Fixed.

>> Source/WebCore/platform/audio/mac/AudioSessionMac.mm:490
>> +        observer.hardwareMutedStateDidChange(this);
> 
> Ditto.

Fixed.

> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:194
> +    for (auto& proxy : m_proxies)

Need to use m_proxies.forEach() as well.

> Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.cpp:152
> +    for (auto& observer : m_configurationChangeObservers) {

Ditto.
Comment 13 EWS 2021-08-04 14:36:47 PDT
Committed r280664 (240270@main): <https://commits.webkit.org/240270@main>

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