Bug 177869 - Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic
Summary: Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-04 05:43 PDT by Alejandro G. Castro
Modified: 2017-10-16 07:45 PDT (History)
4 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2017-10-04 05:43:19 PDT 
We need this in the GTK port in order to use libwebrtc backend and endpoint classes.
Comment 1 Alejandro G. Castro 2017-10-04 05:54:08 PDT 
Created attachment 322658 [details]
Patch
Comment 2 Build Bot 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.
Comment 3 Alejandro G. Castro 2017-10-04 06:22:10 PDT 
Created attachment 322663 [details]
Patch
Comment 4 Build Bot 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.
Comment 5 Alejandro G. Castro 2017-10-04 06:28:36 PDT 
Created attachment 322665 [details]
Patch
Comment 6 Build Bot 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.
Comment 7 Alejandro G. Castro 2017-10-04 08:08:38 PDT 
Created attachment 322672 [details]
Patch
Comment 8 Build Bot 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.
Comment 9 youenn fablet 2017-10-04 08:51:54 PDT 
Created attachment 322675 [details]
Patch
Comment 10 Build Bot 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.
Comment 11 Alejandro G. Castro 2017-10-04 09:03:54 PDT 
Created attachment 322679 [details]
Patch
Comment 12 Build Bot 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.
Comment 13 Alejandro G. Castro 2017-10-04 10:00:03 PDT 
Created attachment 322685 [details]
Patch
Comment 14 Build Bot 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.
Comment 15 Alejandro G. Castro 2017-10-05 02:47:21 PDT 
Created attachment 322810 [details]
Patch
Comment 16 Build Bot 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.
Comment 17 youenn fablet 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.
Comment 18 Alejandro G. Castro 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.
Comment 19 Alejandro G. Castro 2017-10-05 05:18:54 PDT 
Created attachment 322825 [details]
Patch
Comment 20 Build Bot 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.
Comment 21 Alejandro G. Castro 2017-10-05 05:32:40 PDT 
Created attachment 322829 [details]
Patch
Comment 22 Build Bot 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.
Comment 23 Alejandro G. Castro 2017-10-05 05:44:01 PDT 
Created attachment 322830 [details]
Patch
Comment 24 Build Bot 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.
Comment 25 Alejandro G. Castro 2017-10-11 03:49:03 PDT 
Created attachment 323403 [details]
Patch
Comment 26 Build Bot 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.
Comment 27 Alejandro G. Castro 2017-10-11 03:58:40 PDT 
Created attachment 323405 [details]
Patch
Comment 28 Build Bot 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.
Comment 29 Alejandro G. Castro 2017-10-12 22:18:08 PDT 
Created attachment 323636 [details]
Patch
Comment 30 Build Bot 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.
Comment 31 WebKit Commit Bot 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>
Comment 32 WebKit Commit Bot 2017-10-16 07:44:58 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 33 Radar WebKit Bug Importer 2017-10-16 07:45:42 PDT 
<rdar://problem/35006338>