Bug 237316 - Do capture video frame downsampling in GPUProcess
Summary: Do capture video frame downsampling in GPUProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 237359
Blocks: 237704
  Show dependency treegraph
 
Reported: 2022-03-01 03:39 PST by youenn fablet
Modified: 2022-03-11 07:34 PST (History)
10 users (show)

See Also:


Attachments
Patch (55.42 KB, patch)
2022-03-10 02:40 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (55.38 KB, patch)
2022-03-10 03:16 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (57.46 KB, patch)
2022-03-10 04:31 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (55.07 KB, patch)
2022-03-10 05:20 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (66.42 KB, patch)
2022-03-11 02:57 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 2022-03-01 03:39:08 PST
Do capture video frame downsampling in GPUProcess
Comment 1 Radar WebKit Bug Importer 2022-03-08 03:40:16 PST
<rdar://problem/89959777>
Comment 2 youenn fablet 2022-03-10 02:40:23 PST
Created attachment 454328 [details]
Patch
Comment 3 youenn fablet 2022-03-10 03:16:43 PST
Created attachment 454330 [details]
Patch
Comment 4 youenn fablet 2022-03-10 04:31:30 PST
Created attachment 454334 [details]
Patch
Comment 5 youenn fablet 2022-03-10 05:20:56 PST
Created attachment 454338 [details]
Patch
Comment 6 Eric Carlson 2022-03-10 09:13:33 PST
Comment on attachment 454338 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        Since display tracks are already cloned remotely, we can merge RemoteRealtimeDisplaySource and RemoteRealtimeVideoSource.

This is great!

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:265
>      virtual void startProducingData() { }
>      virtual void stopProducingData() { }
> +    virtual void endProducingData() { stop(); }

I think it will be easy for someone that doesn't already know how these classes work to be confused about the difference between stopProducingData and endProducingData.

Conceptually these methods start, pause, and stop  (or cancel?) capture, so maybe they could be `startProducingData`, `pauseProducingData`, and `stopProducingData`. Or maybe we should change all three: `startDataProduction`, `pauseDataProduction`, and `endDataProduction`?

Renaming will touch a lot of files so it could be done in another patch

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:-4186
> -		418FCBE3271049DB00F96ECA /* RemoteRealtimeDisplaySource.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RemoteRealtimeDisplaySource.cpp; sourceTree = "<group>"; };
> -		418FCBE4271049DC00F96ECA /* RemoteRealtimeDisplaySource.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RemoteRealtimeDisplaySource.h; sourceTree = "<group>"; };

Can these files be deleted now?
Comment 7 youenn fablet 2022-03-11 02:54:33 PST
(In reply to Eric Carlson from comment #6)
> Comment on attachment 454338 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454338&action=review
> 
> > Source/WebKit/ChangeLog:11
> > +        Since display tracks are already cloned remotely, we can merge RemoteRealtimeDisplaySource and RemoteRealtimeVideoSource.
> 
> This is great!

Glad you like it, I think it will make things simpler indeed.
Next step is to make RemoteRealtimeAudioSource and RemoteRealtimeVideoSource share most of the code in RemoteRealtimeMediaSource.

> > Source/WebCore/platform/mediastream/RealtimeMediaSource.h:265
> >      virtual void startProducingData() { }
> >      virtual void stopProducingData() { }
> > +    virtual void endProducingData() { stop(); }
> 
> I think it will be easy for someone that doesn't already know how these
> classes work to be confused about the difference between stopProducingData
> and endProducingData.
> 
> Conceptually these methods start, pause, and stop  (or cancel?) capture, so
> maybe they could be `startProducingData`, `pauseProducingData`, and
> `stopProducingData`. Or maybe we should change all three:
> `startDataProduction`, `pauseDataProduction`, and `endDataProduction`?

Agreed.
start and stop are not terminal while end is stop+cannot start again.
I guess we could use pause/resume/end?

> Renaming will touch a lot of files so it could be done in another patch
> 
> > Source/WebKit/WebKit.xcodeproj/project.pbxproj:-4186
> > -		418FCBE3271049DB00F96ECA /* RemoteRealtimeDisplaySource.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RemoteRealtimeDisplaySource.cpp; sourceTree = "<group>"; };
> > -		418FCBE4271049DC00F96ECA /* RemoteRealtimeDisplaySource.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = RemoteRealtimeDisplaySource.h; sourceTree = "<group>"; };
> 
> Can these files be deleted now?

Will do.
Comment 8 youenn fablet 2022-03-11 02:57:57 PST
Created attachment 454466 [details]
Patch for landing
Comment 9 EWS 2022-03-11 07:34:19 PST
Committed r291167 (248327@main): <https://commits.webkit.org/248327@main>

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