Bug 208651 – Implement setWirelessPlaybackTarget, performTaskAtMediaTime, and wouldTaintOrigin in GPUProcess

• Status: ASSIGNED
• Component: Media

[Eric Carlson]

Implement setWirelessPlaybackTarget, performTaskAtMediaTime, and wouldTaintOrigin

[Radar WebKit Bug Importer]

<rdar://problem/60088298>

[Eric Carlson]

Created attachment 392592 [details]
Patch

[Eric Carlson]

Created attachment 392593 [details]
Patch

[Eric Carlson]

Created attachment 392609 [details]
Patch

[youenn fablet]

Comment on attachment 392609 [details]
Patch

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

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:81
> +    m_performTaskAtMediaTimeCompletionHandler = nullptr;

We should call this completion handler otherwise we will get some debug assert.
Maybe the completion handler should return an Optional<Time>, WTF::null opt meaning that it was not done.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:797
> +void RemoteMediaPlayerProxy::performTaskAtMediaTime(MediaTime&& taskTime, WallTime&& messageTime, CompletionHandler<void(const MediaTime&)>&& completionHandler)

I don't think we need a MediaTime&& since it only contains int values. Ditto for WallTime that we can simply pass by value.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:808
> +    m_player->performTaskAtMediaTime([this, weakThis = makeWeakPtr(this)] {

It might be just simpler to have the lambda capture completionHandler so that we do not have to store it in m_performTaskAtMediaTimeCompletionHandler, which could be overridden by two successive RemoteMediaPlayerProxy::performTaskAtMediaTime calls.

> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:817
> +void RemoteMediaPlayerProxy::wouldTaintOrigin(struct WebCore::SecurityOriginData originData, CompletionHandler<void(Optional<bool>)>&& completionHandler)

s/struct WebCore::SecurityOriginData/const SecurityOriginData&/

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:848
> +    if (!connection().sendSync(Messages::RemoteMediaPlayerProxy::WouldTaintOrigin(origin.data()), Messages::RemoteMediaPlayerProxy::WouldTaintOrigin::Reply(wouldTaint), m_id))

Could there be a way to use m_cachedState to remove the sync IPC as a follow-up?

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1034
> +    m_performTaskAtMediaTimeCompletionHandler = WTFMove(completionHandler);

There is the risk that performTaskAtMediaTime is called twice so that we would override a non null m_performTaskAtMediaTimeCompletionHandler.
It might be simpler to just capture completionHandler in the lambda passed to sendWithAsyncReply.

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:353
> +    bool performTaskAtMediaTime(WTF::Function<void()>&&, const MediaTime&) final;

s/WTF:://

> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:373
> +    WTF::Function<void()> m_performTaskAtMediaTimeCompletionHandler;

s/WTF:://

[Eric Carlson]

Comment on attachment 392609 [details]
Patch

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

Thanks for the review!

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:81
>> +    m_performTaskAtMediaTimeCompletionHandler = nullptr;
> 
> We should call this completion handler otherwise we will get some debug assert.
> Maybe the completion handler should return an Optional<Time>, WTF::null opt meaning that it was not done.

Great suggestion, I have made this change.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:797
>> +void RemoteMediaPlayerProxy::performTaskAtMediaTime(MediaTime&& taskTime, WallTime&& messageTime, CompletionHandler<void(const MediaTime&)>&& completionHandler)
> 
> I don't think we need a MediaTime&& since it only contains int values. Ditto for WallTime that we can simply pass by value.

Fixed.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:808
>> +    m_player->performTaskAtMediaTime([this, weakThis = makeWeakPtr(this)] {
> 
> It might be just simpler to have the lambda capture completionHandler so that we do not have to store it in m_performTaskAtMediaTimeCompletionHandler, which could be overridden by two successive RemoteMediaPlayerProxy::performTaskAtMediaTime calls.

MediaPlayerPrivateAVFoundationObjC is the only media player to implement this callback so far, and it only keeps one pending callback at a time. I have added a comment explaining this.

>> Source/WebKit/GPUProcess/media/RemoteMediaPlayerProxy.cpp:817
>> +void RemoteMediaPlayerProxy::wouldTaintOrigin(struct WebCore::SecurityOriginData originData, CompletionHandler<void(Optional<bool>)>&& completionHandler)
> 
> s/struct WebCore::SecurityOriginData/const SecurityOriginData&/

Oops, copy pasta FTW!

Fixed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:848
>> +    if (!connection().sendSync(Messages::RemoteMediaPlayerProxy::WouldTaintOrigin(origin.data()), Messages::RemoteMediaPlayerProxy::WouldTaintOrigin::Reply(wouldTaint), m_id))
> 
> Could there be a way to use m_cachedState to remove the sync IPC as a follow-up?

As we discussed offline, this is possible for some canvas elements so I'll do that in the patch for landing. I have also added a { SecurityOriginData, Optional<bool> } cache to MediaPlayerPrivateRemote, so we will only need to make the sync call once for a given origin in any case.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.cpp:1034
>> +    m_performTaskAtMediaTimeCompletionHandler = WTFMove(completionHandler);
> 
> There is the risk that performTaskAtMediaTime is called twice so that we would override a non null m_performTaskAtMediaTimeCompletionHandler.
> It might be simpler to just capture completionHandler in the lambda passed to sendWithAsyncReply.

Fixed

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:353
>> +    bool performTaskAtMediaTime(WTF::Function<void()>&&, const MediaTime&) final;
> 
> s/WTF:://

Fixed.

>> Source/WebKit/WebProcess/GPU/media/MediaPlayerPrivateRemote.h:373
>> +    WTF::Function<void()> m_performTaskAtMediaTimeCompletionHandler;
> 
> s/WTF:://

Removed.

[Eric Carlson]

Created attachment 392894 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 392894 [details]
Patch for landing

Clearing flags on attachment: 392894

Committed r258082: <https://trac.webkit.org/changeset/258082>