WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-10-08 14:43:54 PDT
<
rdar://problem/84044495
>
Eric Carlson
Comment 2
2021-10-13 10:36:47 PDT
Created
attachment 441098
[details]
Patch
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
Created
attachment 441142
[details]
Patch
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
Created
attachment 441146
[details]
Patch
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
Created
attachment 441313
[details]
Patch
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
Created
attachment 441379
[details]
Patch
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
Created
attachment 441444
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug