Bug 177869

Summary: Make RealtimeIncomingVideoSources and RealtimeOutgoingVideoSources port agnostic
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebRTCAssignee: Alejandro G. Castro <alex>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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>