Bug 207007 - Add GPU Process support for accessLog et al.
Summary: Add GPU Process support for accessLog et al.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-30 10:47 PST by Tim Monroe
Modified: 2020-02-27 11:15 PST (History)
12 users (show)

See Also:


Attachments
Patch (13.44 KB, patch)
2020-01-30 10:51 PST, Tim Monroe
no flags Details | Formatted Diff | Diff
Patch (13.74 KB, patch)
2020-02-05 10:14 PST, Tim Monroe
no flags Details | Formatted Diff | Diff
Patch (13.74 KB, patch)
2020-02-25 08:49 PST, Tim Monroe
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Monroe 2020-01-30 10:47:35 PST
Add GPU process support for accessLog, errorLog, updateVideoFullscreenInlineImage, etc.
Comment 1 Tim Monroe 2020-01-30 10:51:34 PST
Created attachment 389268 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-01-30 10:57:07 PST
Comment on attachment 389268 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.cpp:199
> +void RemoteMediaPlayerManagerProxy::updateVideoFullscreenInlineImage(MediaPlayerPrivateRemoteIdentifier id)

Please don't use 'id'. It will cause compile errors if this file is ever made a .mm file.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in:40
> +    AccessLog(WebKit::MediaPlayerPrivateRemoteIdentifier id) -> (String accessLog) Synchronous
> +    ErrorLog(WebKit::MediaPlayerPrivateRemoteIdentifier id) -> (String errorLog) Synchronous

no id

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:243
> +void RemoteMediaPlayerProxy::updateVideoFullscreenInlineImage(void)

No need for void

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

Does this have to be synchronous IPC? We try to avoid that as much as possible.
Comment 3 Peng Liu 2020-01-30 11:33:40 PST
Comment on attachment 389268 [details]
Patch

Can we enable some layout tests in this patch?
Comment 4 Jer Noble 2020-01-30 11:52:38 PST
iOS bots are failing due to:

/Volumes/Data/worker/iOS-13-Simulator-Build-EWS/build/Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:530:79: error: no member named 'AccessLog' in namespace 'Messages::RemoteMediaPlayerProxy'; did you mean 'Messages::RemoteMediaPlayerManagerProxy::AccessLog'?

Looks like it's a simple fix; the message is defined on RemoteMediaPlayerManagerProxy, not RemoteMediaPlayerProxy.
Comment 5 Tim Monroe 2020-02-05 10:14:47 PST
Created attachment 389825 [details]
Patch
Comment 6 Tim Monroe 2020-02-25 08:49:13 PST
Created attachment 391655 [details]
Patch
Comment 7 Peng Liu 2020-02-25 10:12:50 PST
Comment on attachment 391655 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerManagerProxy.messages.in:39
> +    AccessLog(WebKit::MediaPlayerPrivateRemoteIdentifier playerIdentifier) -> (String accessLog) Synchronous

Is it possible to use Async messages here?
Comment 8 Tim Monroe 2020-02-27 09:05:57 PST
I'm not sure how we would do this asynchronously. At any rate, my understanding of these two methods suggests that they would be called rarely, so perhaps the synchronous nature of the call will not be a real issue?
Comment 9 Simon Fraser (smfr) 2020-02-27 11:00:45 PST
(In reply to Tim Monroe from comment #8)
> I'm not sure how we would do this asynchronously. At any rate, my
> understanding of these two methods suggests that they would be called
> rarely, so perhaps the synchronous nature of the call will not be a real
> issue?

It doesn't matter how rare they are. Any synchronous message can cause hangs in the calling process if the receiving process is busy.
Comment 10 Said Abou-Hallawa 2020-02-27 11:04:13 PST
(In reply to Simon Fraser (smfr) from comment #9)
> (In reply to Tim Monroe from comment #8)
> > I'm not sure how we would do this asynchronously. At any rate, my
> > understanding of these two methods suggests that they would be called
> > rarely, so perhaps the synchronous nature of the call will not be a real
> > issue?
> 
> It doesn't matter how rare they are. Any synchronous message can cause hangs
> in the calling process if the receiving process is busy.

Or the receiving process crashed.
Comment 11 WebKit Commit Bot 2020-02-27 11:14:36 PST
Comment on attachment 391655 [details]
Patch

Clearing flags on attachment: 391655

Committed r257577: <https://trac.webkit.org/changeset/257577>
Comment 12 WebKit Commit Bot 2020-02-27 11:14:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2020-02-27 11:15:21 PST
<rdar://problem/59854422>