Bug 208423

Summary: [GPUP] Plumb through more MediaPlayer methods
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cdumez, commit-queue, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, philipj, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Eric Carlson 2020-02-29 18:13:51 PST 
Plumb through more MediaPlayer methods when media is loaded in the GPU process.
Comment 1 Radar WebKit Bug Importer 2020-02-29 18:14:00 PST 
<rdar://problem/59924386>
Comment 2 Eric Carlson 2020-02-29 18:22:35 PST 
Created attachment 392084 [details]
Patch
Comment 3 Eric Carlson 2020-02-29 19:53:46 PST 
Created attachment 392089 [details]
Patch
Comment 4 Eric Carlson 2020-02-29 22:12:19 PST 
Created attachment 392093 [details]
Patch
Comment 5 youenn fablet 2020-03-02 05:11:33 PST 
Comment on attachment 392093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392093&action=review

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:279
> +    m_cachedState.didPassCORSAccessCheck = m_player->didPassCORSAccessCheck();

Should some of these state updates being added to updateCachedState(), for instance wirelessVideoPlaybackDisabled()?
Should we always resync all values for extra safety?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:675
> +    if (!connection().sendSync(Messages::RemoteMediaPlayerProxy::AccessLog(m_id), Messages::RemoteMediaPlayerProxy::AccessLog::Reply(log), m_id))

Seems to make this fail on iOS since AccessLog is not defined for RemoteMediaPlayerProxy.
Comment 6 Eric Carlson 2020-03-02 05:33:39 PST 
Comment on attachment 392093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=392093&action=review

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:279
>> +    m_cachedState.didPassCORSAccessCheck = m_player->didPassCORSAccessCheck();
> 
> Should some of these state updates being added to updateCachedState(), for instance wirelessVideoPlaybackDisabled()?
> Should we always resync all values for extra safety?

I don't think we want to update all of these in updateCachedState because it is called so frequently

I tried to make sure every cached state is updated in the appropriate media player callback. For example, m_cachedState.wirelessVideoPlaybackDisabled and m_cachedState.wirelessPlaybackTargetName are updated in RemoteMediaPlayerProxy::mediaPlayerCurrentPlaybackTargetIsWirelessChanged if the framework changes @externalPlaybackActive or @allowsExternalPlayback.
Comment 7 Eric Carlson 2020-03-02 05:42:37 PST 
Created attachment 392135 [details]
Patch
Comment 8 Eric Carlson 2020-03-02 06:36:12 PST 
Created attachment 392138 [details]
Patch
Comment 9 WebKit Commit Bot 2020-03-02 07:54:35 PST 
Comment on attachment 392138 [details]
Patch

Clearing flags on attachment: 392138

Committed r257711: <https://trac.webkit.org/changeset/257711>
Comment 10 WebKit Commit Bot 2020-03-02 07:54:37 PST 
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Carlson 2020-03-02 11:17:03 PST 
Reopening to attach new patch.
Comment 12 Eric Carlson 2020-03-02 11:17:04 PST 
Created attachment 392159 [details]
Patch
Comment 13 Eric Carlson 2020-03-02 11:18:37 PST 
Created attachment 392160 [details]
Patch
Comment 14 WebKit Commit Bot 2020-03-02 12:07:48 PST 
Comment on attachment 392160 [details]
Patch

Clearing flags on attachment: 392160

Committed r257724: <https://trac.webkit.org/changeset/257724>
Comment 15 WebKit Commit Bot 2020-03-02 12:07:50 PST 
All reviewed patches have been landed.  Closing bug.