Bug 221747 - Make RemoteRealtimeVideoSource a RealtimeVideoCaptureSource
Summary: Make RemoteRealtimeVideoSource a RealtimeVideoCaptureSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 221201 221331
  Show dependency treegraph
 
Reported: 2021-02-11 03:08 PST by youenn fablet
Modified: 2021-02-12 02:55 PST (History)
10 users (show)

See Also:


Attachments
WIP (28.47 KB, patch)
2021-02-11 03:11 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (30.58 KB, patch)
2021-02-11 05:02 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (30.63 KB, patch)
2021-02-11 05:18 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (37.27 KB, patch)
2021-02-12 01:28 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.26 KB, patch)
2021-02-12 02:10 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-02-11 03:08:06 PST
Make RemoteRealtimeVideoSource a RealtimeVideoCaptureSource
Comment 1 youenn fablet 2021-02-11 03:11:49 PST
Created attachment 419969 [details]
WIP
Comment 2 youenn fablet 2021-02-11 05:02:57 PST
Created attachment 419976 [details]
Patch
Comment 3 youenn fablet 2021-02-11 05:18:27 PST
Created attachment 419978 [details]
Patch
Comment 4 Eric Carlson 2021-02-11 09:04:42 PST
Comment on attachment 419978 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:312
> +void UserMediaCaptureManagerProxy::generatePresets(RealtimeMediaSourceIdentifier id, CompletionHandler<void(Vector<VideoPresetData>&&, IntSize)>&& completionHandler)

This name isn't entirely accurate because it generates presets and fetches the current size. I don't love either of these names, but they would be more correct: `getSizeAndGeneratePresets`, or `generatePresetsAndGetSize`?

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeVideoSource.cpp:83
> +        connection()->sendWithAsyncReply(Messages::UserMediaCaptureManagerProxy::GeneratePresets(identifier()), [this, protectedThis = WTFMove(protectedThis)](auto&& presets, auto size) {

As we discussed, can this be moved into CreateMediaSourceForCaptureDeviceWithConstraints so we don't need to make two calls to the UI process to become ready?

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeVideoSource.cpp:248
> +void RemoteRealtimeVideoSource::setFrameRateWithPreset(double, RefPtr<VideoPreset>)
>  {
> -    m_hasRequestedToEnd = true;
> -    connection()->send(Messages::UserMediaCaptureManagerProxy::RequestToEnd { m_identifier }, 0);
>  }

Can this be removed completely?
Comment 5 youenn fablet 2021-02-11 09:17:25 PST
(In reply to Eric Carlson from comment #4)
> Comment on attachment 419978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419978&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:312
> > +void UserMediaCaptureManagerProxy::generatePresets(RealtimeMediaSourceIdentifier id, CompletionHandler<void(Vector<VideoPresetData>&&, IntSize)>&& completionHandler)
> 
> This name isn't entirely accurate because it generates presets and fetches
> the current size. I don't love either of these names, but they would be more
> correct: `getSizeAndGeneratePresets`, or `generatePresetsAndGetSize`?
> 
> > Source/WebKit/WebProcess/cocoa/RemoteRealtimeVideoSource.cpp:83
> > +        connection()->sendWithAsyncReply(Messages::UserMediaCaptureManagerProxy::GeneratePresets(identifier()), [this, protectedThis = WTFMove(protectedThis)](auto&& presets, auto size) {
> 
> As we discussed, can this be moved into
> CreateMediaSourceForCaptureDeviceWithConstraints so we don't need to make
> two calls to the UI process to become ready?

Right, let's add it to CreateMediaSourceForCaptureDeviceWithConstraints.
Comment 6 youenn fablet 2021-02-12 01:28:06 PST
Created attachment 420103 [details]
Patch for landing
Comment 7 youenn fablet 2021-02-12 01:29:42 PST
> > Source/WebKit/WebProcess/cocoa/RemoteRealtimeVideoSource.cpp:248
> > +void RemoteRealtimeVideoSource::setFrameRateWithPreset(double, RefPtr<VideoPreset>)
> >  {
> > -    m_hasRequestedToEnd = true;
> > -    connection()->send(Messages::UserMediaCaptureManagerProxy::RequestToEnd { m_identifier }, 0);
> >  }
> 
> Can this be removed completely?

Actually, I need to implement it for clone-then-increase-resolution-or-frameRate cases.
I removed on the other hand the applyConstraintsFailed/Succeeded since they are no longer needed for video.
Comment 8 EWS 2021-02-12 02:06:48 PST
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Comment 9 youenn fablet 2021-02-12 02:10:12 PST
Created attachment 420107 [details]
Patch
Comment 10 EWS 2021-02-12 02:54:38 PST
Committed r272778: <https://commits.webkit.org/r272778>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420107 [details].
Comment 11 Radar WebKit Bug Importer 2021-02-12 02:55:14 PST
<rdar://problem/74270196>