Bug 231455 – [iOS] Support getDisplayMedia

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

Support getDisplayMedia on iOS devices.

[Radar WebKit Bug Importer]

<rdar://problem/84044495>

[Eric Carlson]

Created attachment 441098 [details]
Patch

[youenn fablet]

Comment on attachment 441098 [details]
Patch

LGTM, worth rebasing.
Some nits below.

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

> Source/WebCore/SourcesCocoa.txt:539
> +platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp

Should be moved to platform/mediastream/cocoa?

> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:144
> +            return;

Should we retain the sampleBuffer even in that code path?

> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:150
> +        RunLoop::main().dispatch([weakThis, sampleBuffer = retainPtr(sampleBuffer)]() mutable {

Sounds good like this right now.
In the future, given we are now ok with calling videoSampleAvailable in non main thread, we might be able to not hop to main thread.
Maybe a FIXME to keep track of this?

> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:231
> +    m_interrupted = true;

So instead of failing, we mute the capture, compared to AVVideoCaptureSource.
Is there a way capture can fail?

> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:237
> +    static constexpr Seconds verifyCaptureInterval = 2_s;

Should we increase to something bigger? For camera,2 seconds is not a lot.

[Eric Carlson]

Created attachment 441142 [details]
Patch

[Eric Carlson]

Comment on attachment 441098 [details]
Patch

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

>> Source/WebCore/SourcesCocoa.txt:539
>> +platform/mediastream/mac/DisplayCaptureSourceCocoa.cpp
> 
> Should be moved to platform/mediastream/cocoa?

Good point, moved.

>> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:144
>> +            return;
> 
> Should we retain the sampleBuffer even in that code path?

We will once we support audio capture, but I don't t think we need to now.

>> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:150
>> +        RunLoop::main().dispatch([weakThis, sampleBuffer = retainPtr(sampleBuffer)]() mutable {
> 
> Sounds good like this right now.
> In the future, given we are now ok with calling videoSampleAvailable in non main thread, we might be able to not hop to main thread.
> Maybe a FIXME to keep track of this?

Added.

>> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:231
>> +    m_interrupted = true;
> 
> So instead of failing, we mute the capture, compared to AVVideoCaptureSource.
> Is there a way capture can fail?

We mute rather than failing because ReplayKit will silently stop delivering frames when the app is backgrounded, but it resumes when the app comes back to the foreground. I'm sure it is possible for capture to fail, we'll have to test to figure out how to differentiate the two.

>> Source/WebCore/platform/mediastream/ios/ReplayKitCaptureSource.mm:237
>> +    static constexpr Seconds verifyCaptureInterval = 2_s;
> 
> Should we increase to something bigger? For camera,2 seconds is not a lot.

I think it is OK given that we only mute, but we will definitely want to experiment.

[Eric Carlson]

Created attachment 441146 [details]
Patch

[youenn fablet]

Comment on attachment 441146 [details]
Patch

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

> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.cpp:161
> +    m_timer.startRepeating(1_s / frameRate());

It seems commitConfiguration(); is missing here, which might explain debug bot test issues.

> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.cpp:224
> +        [](RetainPtr<IOSurfaceRef> surface) -> IntSize {

I wonder whether this triggers refing?
Maybe RetainPtr<IOSurfaceRef>& would be slightly better?
Ditto below.

[Eric Carlson]

Created attachment 441313 [details]
Patch

[Eric Carlson]

Comment on attachment 441146 [details]
Patch

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

>> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.cpp:161
>> +    m_timer.startRepeating(1_s / frameRate());
> 
> It seems commitConfiguration(); is missing here, which might explain debug bot test issues.

Indeed it was!

>> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.cpp:224
>> +        [](RetainPtr<IOSurfaceRef> surface) -> IntSize {
> 
> I wonder whether this triggers refing?
> Maybe RetainPtr<IOSurfaceRef>& would be slightly better?
> Ditto below.

Good catch, changed.

[Eric Carlson]

Created attachment 441379 [details]
Patch

[Jer Noble]

Comment on attachment 441379 [details]
Patch

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

> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.h:61
> +        class Observer : public CanMakeWeakPtr<Observer> {

Nit: Can we use WTF::Observer here? An inner client class is not forward-declarable.

[Eric Carlson]

Created attachment 441444 [details]
Patch

[Eric Carlson]

Comment on attachment 441379 [details]
Patch

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

>> Source/WebCore/platform/mediastream/cocoa/DisplayCaptureSourceCocoa.h:61
>> +        class Observer : public CanMakeWeakPtr<Observer> {
> 
> Nit: Can we use WTF::Observer here? An inner client class is not forward-declarable.

This Observer class originally had several methods, but since it only has one now I replaced it with WTF::Observer.

[EWS]

Committed r284295 (243093@main): <https://commits.webkit.org/243093@main>

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

[Ben]

On which iOS release is it supposed to work?
getDisplayMedia is not available on iOS 15.4.

[Ben]

Is there an update on this? Status is Resolved since 2021 but it's still not possible to call getDisplayMedia on iOS and iPadOS in Safari 16.5