[MediaStream] Push capabilities across process boundary during UIProcess capture.

[Jer Noble]

[MediaSource] Push capabilities across process boundary during UIProcess capture.

[Jer Noble]

Created attachment 306998 [details]
Patch

[Jer Noble]

Created attachment 306999 [details]
Patch

[Jer Noble]

Created attachment 307002 [details]
Patch

[Eric Carlson]

Comment on attachment 307002 [details]
Patch

Looks fine to me, but a real WK2 reviewer will have to give it an official r+.

[Jer Noble]

Created attachment 307011 [details]
Patch for landing

[Jer Noble]

Created attachment 307012 [details]
Patch for landing

[Jon Lee]

rdar://problem/31435730

[Alex Christensen]

Comment on attachment 307012 [details]
Patch for landing

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:133
> +    RealtimeMediaSourceCapabilities()
> +    {
> +    }

= default;

> Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:152
> +void UserMediaCaptureManagerProxy::capabilities(uint64_t id, std::optional<WebCore::RealtimeMediaSourceCapabilities>& capabilities)

Why not just return a std::optional?

> Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:161
> +    else
> +        capabilities = std::nullopt;

Is it necessary to set capabilities to nullopt here?  If so, why isn't it done in the early return?

> Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:144
> +    mutable bool m_capabilitiesInitialized { false };
> +    mutable std::optional<RealtimeMediaSourceCapabilities> m_capabilities;

This is very strange.  std::optional keeps track of whether it is initialized.  If you need more states than that, use WTF::Expected.

> Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:250
> +    m_process.sendSync(Messages::UserMediaCaptureManagerProxy::Capabilities(id), Messages::UserMediaCaptureManagerProxy::Capabilities::Reply(capabilities), 0);

Why do we need to add another sync message?

[Jer Noble]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 307012 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307012&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:133
> > +    RealtimeMediaSourceCapabilities()
> > +    {
> > +    }
> 
> = default;

Ok.

> > Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:152
> > +void UserMediaCaptureManagerProxy::capabilities(uint64_t id, std::optional<WebCore::RealtimeMediaSourceCapabilities>& capabilities)
> 
> Why not just return a std::optional?

Because that makes the message binding code super unhappy.

> > Source/WebKit2/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:161
> > +    else
> > +        capabilities = std::nullopt;
> 
> Is it necessary to set capabilities to nullopt here?  If so, why isn't it
> done in the early return?

Yes.

> > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:144
> > +    mutable bool m_capabilitiesInitialized { false };
> > +    mutable std::optional<RealtimeMediaSourceCapabilities> m_capabilities;
> 
> This is very strange.  std::optional keeps track of whether it is
> initialized.  If you need more states than that, use WTF::Expected.


Alex and I discussed offline and I may have managed to convince him that std::optional<std::unique_ptr<Capabilities>> is OK.

> > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:250
> > +    m_process.sendSync(Messages::UserMediaCaptureManagerProxy::Capabilities(id), Messages::UserMediaCaptureManagerProxy::Capabilities::Reply(capabilities), 0);
> 
> Why do we need to add another sync message?

Because the caller expects to receive a reply synchronously.

[Jer Noble]

Created attachment 307028 [details]
Patch for landing

[Jer Noble]

Created attachment 307064 [details]
Patch for landing

After discussing this with other members of the WebKit team, it became apparent that the best option was to remove the possibility of returning nullptr from RealtimeMediaSource::capabilities().

[Jer Noble]

Created attachment 307113 [details]
Patch for landing

One last version to make the GTK bts happy.

[WebKit Commit Bot]

Comment on attachment 307113 [details]
Patch for landing

Clearing flags on attachment: 307113

Committed r215362: <http://trac.webkit.org/changeset/215362>

[Sam Weinig]

(In reply to Jer Noble from comment #9)
> (In reply to Alex Christensen from comment #8)
> > Comment on attachment 307012 [details]
> > Patch for landing
> > > Source/WebKit2/WebProcess/cocoa/UserMediaCaptureManager.cpp:250
> > > +    m_process.sendSync(Messages::UserMediaCaptureManagerProxy::Capabilities(id), Messages::UserMediaCaptureManagerProxy::Capabilities::Reply(capabilities), 0);
> > 
> > Why do we need to add another sync message?
> 
> Because the caller expects to receive a reply synchronously.

Can you go into more detail about this? Adding more synchronous calls is something that should be taken very seriously, and all alternatives should be discussed before going forward with it. Why can't capabilities be pushed to the WebProcess proactively when the source is created?

[Maciej Stachowiak]

Comment on attachment 307002 [details]
Patch

Unflagging and obsoleting, since there is a newer patch for landing.