Bug 231455 - [iOS] Support getDisplayMedia
Summary: [iOS] Support getDisplayMedia
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-08 14:43 PDT by Eric Carlson
Modified: 2023-05-30 22:52 PDT (History)
10 users (show)

See Also:


Attachments
Patch (97.29 KB, patch)
2021-10-13 10:36 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (100.84 KB, patch)
2021-10-13 15:10 PDT, Eric Carlson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (100.79 KB, patch)
2021-10-13 15:21 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (100.83 KB, patch)
2021-10-14 17:34 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (101.28 KB, patch)
2021-10-15 08:21 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (101.30 KB, patch)
2021-10-15 15:49 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2021-10-08 14:43:42 PDT
Support getDisplayMedia on iOS devices.
Comment 1 Radar WebKit Bug Importer 2021-10-08 14:43:54 PDT
<rdar://problem/84044495>
Comment 2 Eric Carlson 2021-10-13 10:36:47 PDT
Created attachment 441098 [details]
Patch
Comment 3 youenn fablet 2021-10-13 11:27:54 PDT
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.
Comment 4 Eric Carlson 2021-10-13 15:10:49 PDT
Created attachment 441142 [details]
Patch
Comment 5 Eric Carlson 2021-10-13 15:12:11 PDT
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.
Comment 6 Eric Carlson 2021-10-13 15:21:59 PDT
Created attachment 441146 [details]
Patch
Comment 7 youenn fablet 2021-10-14 04:13:24 PDT
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.
Comment 8 Eric Carlson 2021-10-14 17:34:34 PDT
Created attachment 441313 [details]
Patch
Comment 9 Eric Carlson 2021-10-14 17:36:12 PDT
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.
Comment 10 Eric Carlson 2021-10-15 08:21:06 PDT
Created attachment 441379 [details]
Patch
Comment 11 Jer Noble 2021-10-15 09:39:41 PDT
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.
Comment 12 Eric Carlson 2021-10-15 15:49:52 PDT
Created attachment 441444 [details]
Patch
Comment 13 Eric Carlson 2021-10-15 15:56:29 PDT
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.
Comment 14 EWS 2021-10-15 17:34:33 PDT
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].
Comment 15 Ben 2022-04-10 07:40:38 PDT
On which iOS release is it supposed to work?
getDisplayMedia is not available on iOS 15.4.
Comment 16 Ben 2023-05-30 22:52:25 PDT
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