Bug 227894 - [GPUP] RemoteMediaPlayerProxy may not send the latest "naturalSize" to MediaPlayerPrivateRemote
Summary: [GPUP] RemoteMediaPlayerProxy may not send the latest "naturalSize" to MediaP...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-12 20:50 PDT by Peng Liu
Modified: 2021-07-13 14:08 PDT (History)
7 users (show)

See Also:


Attachments
Patch (10.82 KB, patch)
2021-07-12 21:12 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (2.10 KB, patch)
2021-07-13 10:03 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-12 20:50:43 PDT
[GPUP] RemoteMediaPlayerProxy may not send the latest “naturalSize” to MediaPlayerPrivateRemote
Comment 1 Radar WebKit Bug Importer 2021-07-12 20:51:46 PDT
<rdar://problem/80498060>
Comment 2 Peng Liu 2021-07-12 21:12:42 PDT
Created attachment 433385 [details]
Patch
Comment 3 Eric Carlson 2021-07-13 08:09:04 PDT
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?
Comment 4 Peng Liu 2021-07-13 10:03:37 PDT
Created attachment 433414 [details]
Patch
Comment 5 Jer Noble 2021-07-13 10:07:53 PDT
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 6 Peng Liu 2021-07-13 10:09:37 PDT
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!
Comment 7 EWS 2021-07-13 14:08:34 PDT
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].