RESOLVED FIXED 231455
[iOS] Support getDisplayMedia
https://bugs.webkit.org/show_bug.cgi?id=231455
Summary [iOS] Support getDisplayMedia
Eric Carlson
Reported 2021-10-08 14:43:42 PDT
Support getDisplayMedia on iOS devices.
Attachments
Patch (97.29 KB, patch)
2021-10-13 10:36 PDT, Eric Carlson
no flags
Patch (100.84 KB, patch)
2021-10-13 15:10 PDT, Eric Carlson
ews-feeder: commit-queue-
Patch (100.79 KB, patch)
2021-10-13 15:21 PDT, Eric Carlson
no flags
Patch (100.83 KB, patch)
2021-10-14 17:34 PDT, Eric Carlson
no flags
Patch (101.28 KB, patch)
2021-10-15 08:21 PDT, Eric Carlson
no flags
Patch (101.30 KB, patch)
2021-10-15 15:49 PDT, Eric Carlson
no flags
Radar WebKit Bug Importer
Comment 1 2021-10-08 14:43:54 PDT
Eric Carlson
Comment 2 2021-10-13 10:36:47 PDT
youenn fablet
Comment 3 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.
Eric Carlson
Comment 4 2021-10-13 15:10:49 PDT
Eric Carlson
Comment 5 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.
Eric Carlson
Comment 6 2021-10-13 15:21:59 PDT
youenn fablet
Comment 7 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.
Eric Carlson
Comment 8 2021-10-14 17:34:34 PDT
Eric Carlson
Comment 9 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.
Eric Carlson
Comment 10 2021-10-15 08:21:06 PDT
Jer Noble
Comment 11 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.
Eric Carlson
Comment 12 2021-10-15 15:49:52 PDT
Eric Carlson
Comment 13 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.
EWS
Comment 14 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].
Ben
Comment 15 2022-04-10 07:40:38 PDT
On which iOS release is it supposed to work? getDisplayMedia is not available on iOS 15.4.
Ben
Comment 16 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
Note You need to log in before you can comment on or make changes to this bug.