[GPUP] RemoteMediaPlayerProxy may not send the latest “naturalSize” to MediaPlayerPrivateRemote
<rdar://problem/80498060>
Created attachment 433385 [details] Patch
Comment on attachment 433385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433385&action=review > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:843 > + m_cachedState.startDate = m_player->getStartDate(); Why should we query this every time when it can only change when readyState changes? > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:844 > + m_cachedState.startTime = m_player->startTime(); Ditto > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:853 > + m_cachedState.networkState = m_player->networkState(); > + m_cachedState.readyState = m_player->readyState(); Why should we query these every time when each has a specific callback? > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:854 > + m_cachedState.movieLoadType = m_player->movieLoadType(); Why should we query this every time when it can only change when readyState changes? > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:868 > + m_cachedState.wouldTaintDocumentSecurityOrigin = m_player->wouldTaintOrigin(m_configuration.documentSecurityOrigin.securityOrigin()); Ditto > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:870 > + m_cachedState.canSaveMediaData = m_player->canSaveMediaData(); Ditto > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:875 > + m_cachedState.hasAvailableVideoFrame = m_player->hasAvailableVideoFrame(); This will never change from `true` to `false`. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:877 > + m_cachedState.wirelessVideoPlaybackDisabled = m_player->wirelessVideoPlaybackDisabled(); This won't change unless the readyState changes, except if `RemoteMediaPlayerProxy::setWirelessVideoPlaybackDisabled` is called. > Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:880 > + m_cachedState.hasSingleSecurityOrigin = m_player->hasSingleSecurityOrigin(); > + m_cachedState.didPassCORSAccessCheck = m_player->didPassCORSAccessCheck(); These won't change unless the readyState changes > Source/WebKit/WebProcess/GPU/media/RemoteMediaPlayerState.h:64 > + String languageOfPrimaryAudioTrack; Won't moving this into the middle of a bunch of bools increase the size of the struct unnecessarily?
Created attachment 433414 [details] Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=433385&action=review >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:843 >> + m_cachedState.startDate = m_player->getStartDate(); > > Why should we query this every time when it can only change when readyState changes? I don't think that's necessarily true; or at least, there's no guarantee that's true. >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:875 >> + m_cachedState.hasAvailableVideoFrame = m_player->hasAvailableVideoFrame(); > > This will never change from `true` to `false`. I don't think this is true; MSE can definitely set this to false. Generally, for all of these properties, there should not be a problem querying them more often; IMO, if it's expensive, we should add caching at the MediaPlayerPrivate level, and not bake in assumptions about when they change into RemoteMediaPlayerProxy.
Comment on attachment 433385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433385&action=review >> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:843 >> + m_cachedState.startDate = m_player->getStartDate(); > > Why should we query this every time when it can only change when readyState changes? The intention of this patch was to refresh most fields of m_cachedState in updateCachedState(). But that seems a bad idea because many fields are related to readyState changes. I have uploaded a new patch. Ditto to other comments. Thanks for the review!
Committed r279891 (239642@main): <https://commits.webkit.org/239642@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433414 [details].