Summary: | [Catalina][GPUP] Some API tests fail after GPU Process features are enabled | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||||||||||
Component: | Tools / Tests | Assignee: | Peng Liu <peng.liu6> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aakash_jain, ap, calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, philipj, ryanhaddad, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | Other | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=228720 https://bugs.webkit.org/show_bug.cgi?id=228759 https://bugs.webkit.org/show_bug.cgi?id=228760 |
||||||||||||||||
Attachments: |
|
Description
Peng Liu
2021-07-29 09:38:52 PDT
Created attachment 434529 [details]
Patch
Created attachment 434535 [details]
[fast-cq] Patch
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]. The bug is not resolved yet. Created attachment 434869 [details]
WIP patch
Created attachment 434870 [details]
WIP patch
Bug 228759 and 228760 will track the fixes for the remaining test failures. Created attachment 434887 [details]
Patch
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. Created attachment 434923 [details]
Revise the patch based on Jer's comments
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. 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]. |