Bug 227894 – [GPUP] RemoteMediaPlayerProxy may not send the latest "naturalSize" to MediaPlayerPrivateRemote

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

[GPUP] RemoteMediaPlayerProxy may not send the latest “naturalSize” to MediaPlayerPrivateRemote

[Radar WebKit Bug Importer]

<rdar://problem/80498060>

[Peng Liu]

Created attachment 433385 [details]
Patch

[Eric Carlson]

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?

[Peng Liu]

Created attachment 433414 [details]
Patch

[Jer Noble]

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.

[Peng Liu]

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!

[EWS]

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