Bug 228589 – [Catalina][GPUP] Some API tests fail after GPU Process features are enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Peng Liu]

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"

[Peng Liu]

Created attachment 434529 [details]
Patch

[Peng Liu]

Created attachment 434535 [details]
[fast-cq] Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/81283879>

[Peng Liu]

The bug is not resolved yet.

[Peng Liu]

Created attachment 434869 [details]
WIP patch

[Peng Liu]

Created attachment 434870 [details]
WIP patch

[Peng Liu]

Bug 228759 and 228760 will track the fixes for the remaining test failures.

[Peng Liu]

Created attachment 434887 [details]
Patch

[Jer Noble]

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.

[Peng Liu]

Created attachment 434923 [details]
Revise the patch based on Jer's comments

[Peng Liu]

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.

[EWS]

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].