Bug 234029 – [macOS] Add new screen and window capture backend

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

Add new screen and window capture backend

[Eric Carlson]

Created attachment 446426 [details]
Patch

[Eric Carlson]

Created attachment 446441 [details]
Patch

[Eric Carlson]

Created attachment 446454 [details]
Patch

[Eric Carlson]

Created attachment 446467 [details]
Patch

[youenn fablet]

Comment on attachment 446467 [details]
Patch

iOS and GTK compilation issues seem legit.
Some feedback below.

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:119
> +    void setUseMockCaptureDevices(bool);

It is not clear to me why we need the getters/setters.
Until now, we use MockRealtimeMediaSourceCenter to set the mock factories and that is sufficient.
If we have a factory that is almost the same with and without mocks, that is fine.
We can still ask MockRealtimeMediaSourceCenter to use the factory and set it in mock mode without surfacing mocks here.
Ditto for useScreenCatpureKit.

> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:72
> +        ScreenCaptureKitCaptureSource::screenCaptureDevices(m_devices);

if () return

> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:84
> +#if PLATFORM(MAC) && HAVE(SCREEN_CAPTURE_KIT)

Why PLATFORM(MAC), isn't HAVE(SCREEN_CAPTURE_KIT) sufficient?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:56
> +    ScreenCaptureKitCaptureSource(const CaptureDevice&);

explicit, should it be private?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:69
> +    void streamFailedWithError(RetainPtr<NSError>, const String&);

RetainPtr<NSError>&&?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:83
> +        strongSelf->_callback->streamFailedWithError(WTFMove(error), "-[SCStreamDelegate stream:didStopWithError:] called");

Is it possible for _callback to be nullptr after the callOnMainRunLoop?
Should we do the if(!m_callback) in the lambda.

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:101
> +        oldAPI = !!PAL::getSCStreamFilterClass();

Do we need to support both APIs or only the latest one?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:126
> +    m_deviceID = *parseInteger<uint32_t>(device.persistentId());

Let's pass deviceID to the constructor to not reparse it twice.

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:162
> +            callOnMainRunLoop([weakThis, error = RetainPtr { error }]() mutable {

weakThis = WTFMove(weakThis)

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:164
> +                    weakThis->streamFailedWithError(WTFMove(error), "-[SCStream stopWithHandler:] failed");

""_s here and in other places

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:185
> +void ScreenCaptureKitCaptureSource::processSharableContent(RetainPtr<SCShareableContent> shareableContent, RetainPtr<NSError> error)

RetainPtr&& ?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:194
> +    if (m_captureDevice.type() == CaptureDevice::DeviceType::Screen) {

Maybe 
ASSERT(m_captureDevice.type() == CaptureDevice::DeviceType::Screen || m_captureDevice.type() == CaptureDevice::DeviceType::Window).

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:238
> +    GetShareableContentCompletionHandler completionHandler = usingOldAPI() ? (GetShareableContentCompletionHandler)oldAPICompletionHandler.get() : newAPICompletionHandler.get();

Can we rewrite it as:
GetShareableContentCompletionHandler completionHandler;
if (!usingOldAPI()) {
    completionHandler = makeBlockPtr([weakThis = WeakPtr { *this }] (SCShareableContent *shareableContent) mutable {. ....
} else {
    completionHandler = (GetShareableContentCompletionHandler)(makeBlockPtr(...))
}

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:254
> +    [m_streamConfiguration setShowsCursor:YES];

Interesting!

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:260
> +        [m_streamConfiguration setMinimumFrameTime:1 / m_frameRate];

Nice as well!

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:296
> +        streamFailedWithError(nil, "Failed to allocate SCContentFilter");

_s

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:305
> +        streamFailedWithError(nil, "Failed to allocate SLContentStream");

_s

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:315
> +                weakThis->streamFailedWithError(WTFMove(error), "-[SCStream startCaptureWithFrameHandler:completionHandler:] failed");

_s

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:359
> +

So, if we want to update frame rate on the fly, we would call this, this looks good to me.

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:362
> +            return;

Is the error making capture fail?
Or is it just continuing to capture with the old configuration.
If the latter, updateStreamConfiguration() could take a CompletionHandler so that other parts of the code can react upon the error (reject an applyConstraint promise say).

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:401
> +        if (!weakThis)

If m_frameAvailableHandler is called in the captureQueue, this weakThis check is probably not worth it.

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:522
> +        windows.append({ String::number(windowID), CaptureDevice::DeviceType::Window, windowTitle, emptyString(), true });

How are working windowID?
Are they constantly increasing (and not reused) or can they be recycled.
Say user selected a given window, we get its windowID, then a few milliseconds later, capture did not start but window is closed and a new window is created.
Do we have a risk of starting to capture the wrong window?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:543
> +            return;

What happens if user selects an on screen window, which before capture goes out of screen?
Would we fail the capture or would we succeed?

> Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.h:51
> +    virtual bool canPromptForGetDisplayMedia();

Do we need virtual functions, or can we just add these as regular methods and use #if to specialise them to different platforms?

> Source/WebKit/UIProcess/WebPageProxy.cpp:8561
> +    WebCore::RealtimeMediaSourceCenter::singleton().setUseMockCaptureDevices(preferences().mockCaptureDevicesEnabled());

We already have handling for mock capture devices, see UserMediaPermissionRequestManagerProxy::syncWithWebCorePrefs.
I would be tempted to do the same if needed for useScreenCaptureKit.
Not also that we might need to keep it in sync in UIProcess and GPUProcess if we do capture in GPUProcess.

> Source/WebKit/UIProcess/mac/DisplayCaptureSessionManager.h:43
> +

Line not needed.

> Source/WebKit/UIProcess/mac/DisplayCaptureSessionManager.h:44
> +    static DisplayCaptureSessionManager& singleton();

DisplayCaptureSessionManager seems to be a singleton, we might not need CanMakeWeakPtr?

> Source/WebKit/UIProcess/mac/DisplayCaptureSessionManager.mm:55
> +static Vector<WindowInfo> getMockWindowList()

It would be nice if we could avoid DisplayCaptureSessionManager to handle mocks/not mocks.
Maybe the display factory should provide a list of window info.

> Source/WebKit/UIProcess/mac/DisplayCaptureSessionManager.mm:103
> +    return getCGWindowList();

Can we make it so that we do something like:
return RealtimeMediaSourceCenter::singleton().displayFactory().mockWindowList().

Are we sure we can get this list synchronously? Should we use a lambda?

[Eric Carlson]

Created attachment 446614 [details]
Patch

[Eric Carlson]

Comment on attachment 446467 [details]
Patch

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

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:119
>> +    void setUseMockCaptureDevices(bool);
> 
> It is not clear to me why we need the getters/setters.
> Until now, we use MockRealtimeMediaSourceCenter to set the mock factories and that is sufficient.
> If we have a factory that is almost the same with and without mocks, that is fine.
> We can still ask MockRealtimeMediaSourceCenter to use the factory and set it in mock mode without surfacing mocks here.
> Ditto for useScreenCatpureKit.

Fixed.

>> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:72
>> +        ScreenCaptureKitCaptureSource::screenCaptureDevices(m_devices);
> 
> if () return

Fixed.

>> Source/WebCore/platform/mediastream/mac/DisplayCaptureManagerCocoa.cpp:84
>> +#if PLATFORM(MAC) && HAVE(SCREEN_CAPTURE_KIT)
> 
> Why PLATFORM(MAC), isn't HAVE(SCREEN_CAPTURE_KIT) sufficient?

Good point, fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:56
>> +    ScreenCaptureKitCaptureSource(const CaptureDevice&);
> 
> explicit, should it be private?

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:69
>> +    void streamFailedWithError(RetainPtr<NSError>, const String&);
> 
> RetainPtr<NSError>&&?

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:83
>> +        strongSelf->_callback->streamFailedWithError(WTFMove(error), "-[SCStreamDelegate stream:didStopWithError:] called");
> 
> Is it possible for _callback to be nullptr after the callOnMainRunLoop?
> Should we do the if(!m_callback) in the lambda.

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:126
>> +    m_deviceID = *parseInteger<uint32_t>(device.persistentId());
> 
> Let's pass deviceID to the constructor to not reparse it twice.

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:162
>> +            callOnMainRunLoop([weakThis, error = RetainPtr { error }]() mutable {
> 
> weakThis = WTFMove(weakThis)

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:164
>> +                    weakThis->streamFailedWithError(WTFMove(error), "-[SCStream stopWithHandler:] failed");
> 
> ""_s here and in other places

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:185
>> +void ScreenCaptureKitCaptureSource::processSharableContent(RetainPtr<SCShareableContent> shareableContent, RetainPtr<NSError> error)
> 
> RetainPtr&& ?

Fixed.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:194
>> +    if (m_captureDevice.type() == CaptureDevice::DeviceType::Screen) {
> 
> Maybe 
> ASSERT(m_captureDevice.type() == CaptureDevice::DeviceType::Screen || m_captureDevice.type() == CaptureDevice::DeviceType::Window).

Good idea, I added the assert to the create method.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:8561
>> +    WebCore::RealtimeMediaSourceCenter::singleton().setUseMockCaptureDevices(preferences().mockCaptureDevicesEnabled());
> 
> We already have handling for mock capture devices, see UserMediaPermissionRequestManagerProxy::syncWithWebCorePrefs.
> I would be tempted to do the same if needed for useScreenCaptureKit.
> Not also that we might need to keep it in sync in UIProcess and GPUProcess if we do capture in GPUProcess.

Good idea, I've moved it to syncWithWebCorePrefs.

>> Source/WebKit/UIProcess/mac/DisplayCaptureSessionManager.h:44
>> +    static DisplayCaptureSessionManager& singleton();
> 
> DisplayCaptureSessionManager seems to be a singleton, we might not need CanMakeWeakPtr?

Both fixed.

>> Source/WebKit/UIProcess/mac/DisplayCaptureSessionManager.mm:55
>> +static Vector<WindowInfo> getMockWindowList()
> 
> It would be nice if we could avoid DisplayCaptureSessionManager to handle mocks/not mocks.
> Maybe the display factory should provide a list of window info.

Great idea, I updated the display factory to return a window list.

[Eric Carlson]

Created attachment 446617 [details]
Patch

[Eric Carlson]

Created attachment 446623 [details]
Patch

[Jer Noble]

Comment on attachment 446623 [details]
Patch

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

r=me with a nit and a couple of questions.

> Source/WebCore/platform/mediastream/MediaConstraints.h:576
> -        if (!m_exact.isEmpty())
> +        if (m_exact.isEmpty())

!!

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:170
> +#if HAVE(SCREEN_CAPTURE_KIT)
> +inline bool RealtimeMediaSourceCenter::useScreenCaptureKit() const
> +{
> +    return m_useScreenCaptureKit;
> +}
> +
> +inline void RealtimeMediaSourceCenter::setUseScreenCaptureKit(bool useScreenCaptureKit)
> +{
> +    m_useScreenCaptureKit = useScreenCaptureKit;
> +}
> +#endif
> +

Minor nit: it would be slightly less screen real estate to just put these definitions inline in the class definition.

> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.h:64
> +class CapturerObserver : public CanMakeWeakPtr<CapturerObserver> {
> +public:
> +    virtual ~CapturerObserver() = default;
> +
> +    virtual void capturerIsRunningChanged(bool) { }
> +    virtual void capturerFailed() { };
> +};
> +
> +class DisplayCaptureSourceCocoa final
> +    : public RealtimeMediaSource
> +    , public CapturerObserver {

I'm having a hard time understanding why the CapturerObserver class is necessary. It seems to only be accessed within DisplayCaptureSourceCocoa. Can't the Capturer just reference DisplayCaptureSourceCocoa directly?

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:91
> +    static void forEachNSWindow(const Function<bool(NSDictionary *, unsigned, const String&)>&);

Nit: static privates can just be free functions in the implementation file.

> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.mm:543
> +    RetainPtr<NSArray> windowList = adoptNS((__bridge NSArray *)CGWindowListCopyWindowInfo(kCGWindowListOptionOnScreenOnly | kCGWindowListExcludeDesktopElements, kCGNullWindowID));

How expensive is this call? Should we consider caching the answer? I guess invalidation would be a big problem.

[Eric Carlson]

Created attachment 446635 [details]
Patch

[Eric Carlson]

Comment on attachment 446623 [details]
Patch

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

>> Source/WebCore/platform/mediastream/MediaConstraints.h:576
>> +        if (m_exact.isEmpty())
> 
> !!

:-/

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:170
>> +
> 
> Minor nit: it would be slightly less screen real estate to just put these definitions inline in the class definition.

This way avoids the style-bot error about adding an inline function to an exported class.

>> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.h:64
>> +    , public CapturerObserver {
> 
> I'm having a hard time understanding why the CapturerObserver class is necessary. It seems to only be accessed within DisplayCaptureSourceCocoa. Can't the Capturer just reference DisplayCaptureSourceCocoa directly?

As we discussed, I'll clean this up in a followup patch.

>> Source/WebCore/platform/mediastream/mac/ScreenCaptureKitCaptureSource.h:91
>> +    static void forEachNSWindow(const Function<bool(NSDictionary *, unsigned, const String&)>&);
> 
> Nit: static privates can just be free functions in the implementation file.

Good point!

[Eric Carlson]

Created attachment 446738 [details]
Patch for landing

[Eric Carlson]

Created attachment 446751 [details]
Patch for landing

[Jer Noble]

Created attachment 446798 [details]
Patch for landing

[EWS]

Committed r286893 (245121@main): <https://commits.webkit.org/245121@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446798 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86347726>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 234197

[Eric Carlson]

rdar://84760077

[Eric Carlson]

Created attachment 449949 [details]
Patch

[Eric Carlson]

Created attachment 449954 [details]
Patch

[Eric Carlson]

Created attachment 449968 [details]
Patch

[Eric Carlson]

Created attachment 449976 [details]
Patch

[Eric Carlson]

Created attachment 449980 [details]
Patch

[Eric Carlson]

Created attachment 449982 [details]
Patch

[Eric Carlson]

LINK : fatal error LNK1104: cannot open file 'C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\bin64\WebKit.dll'

Windows failure is unrelated.

[EWS]

Committed r288604 (246423@main): <https://commits.webkit.org/246423@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449982 [details].