177869 – Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic

RESOLVED FIXED 177869

Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic

https://bugs.webkit.org/show_bug.cgi?id=177869

Summary Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic

Alejandro G. Castro

Reported 2017-10-04 05:43:19 PDT

We need this in the GTK port in order to use libwebrtc backend and endpoint classes.

Attachments
Patch (75.90 KB, patch) 2017-10-04 05:54 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (75.73 KB, patch) 2017-10-04 06:22 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (75.88 KB, patch) 2017-10-04 06:28 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (77.68 KB, patch) 2017-10-04 08:08 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (75.99 KB, patch) 2017-10-04 08:51 PDT, youenn fablet	no flags	Details Formatted Diff Diff
Patch (77.77 KB, patch) 2017-10-04 09:03 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (78.83 KB, patch) 2017-10-04 10:00 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (75.75 KB, patch) 2017-10-05 02:47 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (73.87 KB, patch) 2017-10-05 05:18 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (74.57 KB, patch) 2017-10-05 05:32 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (74.58 KB, patch) 2017-10-05 05:44 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (75.26 KB, patch) 2017-10-11 03:49 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (75.25 KB, patch) 2017-10-11 03:58 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Patch (74.67 KB, patch) 2017-10-12 22:18 PDT, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.

Alejandro G. Castro

Comment 1 2017-10-04 05:54:08 PDT

Created attachment 322658 [details] Patch

Build Bot

Comment 2 2017-10-04 05:55:50 PDT

Attachment 322658 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:73: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:74: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 3 2017-10-04 06:22:10 PDT

Created attachment 322663 [details] Patch

Build Bot

Comment 4 2017-10-04 06:23:50 PDT

Attachment 322663 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:83: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:84: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 5 2017-10-04 06:28:36 PDT

Created attachment 322665 [details] Patch

Build Bot

Comment 6 2017-10-04 06:30:32 PDT

Attachment 322665 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:83: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:84: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 7 2017-10-04 08:08:38 PDT

Created attachment 322672 [details] Patch

Build Bot

Comment 8 2017-10-04 08:10:24 PDT

Attachment 322672 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

youenn fablet

Comment 9 2017-10-04 08:51:54 PDT

Created attachment 322675 [details] Patch

Build Bot

Comment 10 2017-10-04 08:54:09 PDT

Attachment 322675 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 11 2017-10-04 09:03:54 PDT

Created attachment 322679 [details] Patch

Build Bot

Comment 12 2017-10-04 09:05:57 PDT

Attachment 322679 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 13 2017-10-04 10:00:03 PDT

Created attachment 322685 [details] Patch

Build Bot

Comment 14 2017-10-04 10:03:05 PDT

Attachment 322685 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 15 2017-10-05 02:47:21 PDT

Created attachment 322810 [details] Patch

Build Bot

Comment 16 2017-10-05 02:49:41 PDT

Attachment 322810 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

youenn fablet

Comment 17 2017-10-05 03:58:09 PDT

Comment on attachment 322810 [details] Patch r=me provided that bots are happy. Some comments below to improve the code since we are touching it. Thinking about it, I guess RealtimeXXMac.h/.cpp should probably named RealtimeXXCocoa.h/.cpp View in context: https://bugs.webkit.org/attachment.cgi?id=322810&action=review > Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.cpp:47 > + auto source = RealtimeIncomingVideoSourceMac::create(WTFMove(videoTrack), WTFMove(trackId)); We can probably move RealtimeIncomingVideoSource::create implementation to RealtimeIncomingVideoSourceMac.cpp. That would remove the FIXME and the need to include RealtimeIncomingVideoSourceMac.h here. We duplicate source->start() but this is probably ok. > Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.h:67 > + void OnFrame(const webrtc::VideoFrame&) override { }; Can we remove this one since GTK is not compiling with LIBWEBRTC on currently? > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:49 > + return RealtimeOutgoingVideoSourceMac::create(WTFMove(videoSource)); Ditto here for moving this to RealtimeOutgoingVideoSourceMac.cpp. > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:87 > + bool GetStats(Stats*) final; Could be inlined since it is a one liner function. > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:99 > + virtual void videoSampleAvailable(MediaSample&) { }; Since videoSampleAvailable would be made virtual, maybe it is best to kill it and implement the already virtual sampleBufferUpdated directly in the port specific classes. > Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceMac.cpp:149 > + callOnMainThread([protectedThis = WTFMove(protectedThis), sample = WTFMove(sample), width, height, rotation] { protectedThis = makeRef(*this) > Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceMac.h:16 > + * software without specific prior written permission. Remove this third clause here and in other Mac.h/.cpp files since they are new. > Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSourceMac.h:40 > + ~RealtimeOutgoingVideoSourceMac() { } Probably not needed.

Alejandro G. Castro

Comment 18 2017-10-05 04:17:53 PDT

(In reply to youenn fablet from comment #17) > Comment on attachment 322810 [details] > Patch > > r=me provided that bots are happy. There is still some compilation issue with the mac ones I try to find. > Some comments below to improve the code since we are touching it. > > Thinking about it, I guess RealtimeXXMac.h/.cpp should probably named > RealtimeXXCocoa.h/.cpp > Right. > View in context: > https://bugs.webkit.org/attachment.cgi?id=322810&action=review > > > Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.cpp:47 > > + auto source = RealtimeIncomingVideoSourceMac::create(WTFMove(videoTrack), WTFMove(trackId)); > > We can probably move RealtimeIncomingVideoSource::create implementation to > RealtimeIncomingVideoSourceMac.cpp. > That would remove the FIXME and the need to include > RealtimeIncomingVideoSourceMac.h here. > We duplicate source->start() but this is probably ok. > Right. > > Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.h:67 > > + void OnFrame(const webrtc::VideoFrame&) override { }; > > Can we remove this one since GTK is not compiling with LIBWEBRTC on > currently? > Right, until we upload the branch. > > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:49 > > + return RealtimeOutgoingVideoSourceMac::create(WTFMove(videoSource)); > > Ditto here for moving this to RealtimeOutgoingVideoSourceMac.cpp. > Right. > > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:87 > > + bool GetStats(Stats*) final; > > Could be inlined since it is a one liner function. > Right. > > Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:99 > > + virtual void videoSampleAvailable(MediaSample&) { }; > > Since videoSampleAvailable would be made virtual, maybe it is best to kill > it and implement the already virtual sampleBufferUpdated directly in the > port specific classes. > Right. > > Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceMac.cpp:149 > > + callOnMainThread([protectedThis = WTFMove(protectedThis), sample = WTFMove(sample), width, height, rotation] { > > protectedThis = makeRef(*this) > Right > > Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceMac.h:16 > > + * software without specific prior written permission. > > Remove this third clause here and in other Mac.h/.cpp files since they are > new. > Right. > > Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSourceMac.h:40 > > + ~RealtimeOutgoingVideoSourceMac() { } > > Probably not needed. I'll try it.

Alejandro G. Castro

Comment 19 2017-10-05 05:18:54 PDT

Created attachment 322825 [details] Patch

Build Bot

Comment 20 2017-10-05 05:21:35 PDT

Attachment 322825 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 21 2017-10-05 05:32:40 PDT

Created attachment 322829 [details] Patch

Build Bot

Comment 22 2017-10-05 05:35:53 PDT

Attachment 322829 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 23 2017-10-05 05:44:01 PDT

Created attachment 322830 [details] Patch

Build Bot

Comment 24 2017-10-05 05:47:00 PDT

Attachment 322830 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 25 2017-10-11 03:49:03 PDT

Created attachment 323403 [details] Patch

Build Bot

Comment 26 2017-10-11 03:52:01 PDT

Attachment 323403 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 27 2017-10-11 03:58:40 PDT

Created attachment 323405 [details] Patch

Build Bot

Comment 28 2017-10-11 04:00:09 PDT

Attachment 323405 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Alejandro G. Castro

Comment 29 2017-10-12 22:18:08 PDT

Created attachment 323636 [details] Patch

Build Bot

Comment 30 2017-10-12 22:20:15 PDT

Attachment 323636 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:85: is_screencast is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.h:86: needs_denoising is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Commit Bot

Comment 31 2017-10-16 07:44:56 PDT

Comment on attachment 323636 [details] Patch Clearing flags on attachment: 323636 Committed r223406: <https://trac.webkit.org/changeset/223406>

WebKit Commit Bot

Comment 32 2017-10-16 07:44:58 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 33 2017-10-16 07:45:42 PDT

<rdar://problem/35006338>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebRTC

Assignee

Alejandro G. Castro

Reported

2017-10-04 05:43 PDT

Modified

2017-10-16 07:45 PDT History

CC List

4 users Show

URL

Keywords InRadar

Depends on

Blocks