Bug 207007 – Add GPU Process support for accessLog et al.

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

[Tim Monroe]

Add GPU process support for accessLog, errorLog, updateVideoFullscreenInlineImage, etc.

[Tim Monroe]

Created attachment 389268 [details]
Patch

[Simon Fraser (smfr)]

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.

[Peng Liu]

Comment on attachment 389268 [details]
Patch

Can we enable some layout tests in this patch?

[Jer Noble]

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.

[Tim Monroe]

Created attachment 389825 [details]
Patch

[Tim Monroe]

Created attachment 391655 [details]
Patch

[Peng Liu]

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?

[Tim Monroe]

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?

[Simon Fraser (smfr)]

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

[Said Abou-Hallawa]

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

[WebKit Commit Bot]

Comment on attachment 391655 [details]
Patch

Clearing flags on attachment: 391655

Committed r257577: <https://trac.webkit.org/changeset/257577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59854422>