Bug 167289 – [WebRTC] Implement WebRTC PeerConnection backend based on libwebrtc

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Let's implement WebRTC PeerConnection backend based on libwebrtc, behind the flag until we are ready to pass some tests.

[youenn fablet]

Created attachment 299459 [details]
Patch

[WebKit Commit Bot]

Attachment 299459 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:55:  is_screencast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:56:  needs_denoising is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:58:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:117:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:144:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:104:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:130:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 8 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

This patch is not functional as is as it is missing WK1/WK2 pieces.

[youenn fablet]

Created attachment 299464 [details]
Patch

[WebKit Commit Bot]

Attachment 299464 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:55:  is_screencast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/mediastream/mac/RealtimeOutgoingVideoSource.h:56:  needs_denoising is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCUtils.cpp:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:117:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:144:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:104:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:130:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 8 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 299469 [details]
Patch

[youenn fablet]

Created attachment 299859 [details]
Patch

[WebKit Commit Bot]

Attachment 299859 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:107:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:135:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 299859 [details]
Patch

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

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:284
> +    RefPtr<LibWebRTCMediaEndpoint> protectedThis(this);
> +    callOnMainThread([protectedThis = WTFMove(protectedThis), stream = WTFMove(stream)] {

Here's the cool C++14/WTF way of doing things like this.
protectedThis = makeRef(*this)

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:107
> +        explicit CreateSessionDescriptionObserver(LibWebRTCMediaEndpoint &endpoint) : m_endpoint(endpoint) { }

needs to be on more than one line according to stylebot.

[Alex Christensen]

Comment on attachment 299859 [details]
Patch

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

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:13
> + * 3.  Neither the name of Apple Inc. nor the names of

I think we use a 2-clause copyright header by default for new files now.  See Source/WebCore/LICENSE-APPLE

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:64
> +    m_backend = client.createPeerConnection(*this);

This can be in the initializers.

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:316
> +    // ASSERT_NOT_REACHED();

Commented out?  Maybe this and the previous one should be notImplemented();

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:336
> +    default:

Could we not do a default?

[youenn fablet]

Comment on attachment 299859 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:13
>> + * 3.  Neither the name of Apple Inc. nor the names of
> 
> I think we use a 2-clause copyright header by default for new files now.  See Source/WebCore/LICENSE-APPLE

I'll check that

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:64
>> +    m_backend = client.createPeerConnection(*this);
> 
> This can be in the initializers.

ok

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:316
>> +    // ASSERT_NOT_REACHED();
> 
> Commented out?  Maybe this and the previous one should be notImplemented();

Sure

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.cpp:336
>> +    default:
> 
> Could we not do a default?

There is a kMax value that we could use instead of default.

>> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:107
>> +        explicit CreateSessionDescriptionObserver(LibWebRTCMediaEndpoint &endpoint) : m_endpoint(endpoint) { }
> 
> needs to be on more than one line according to stylebot.

I prefer this style when there is only one initialiser.

[youenn fablet]

Created attachment 299890 [details]
Patch

[WebKit Commit Bot]

Attachment 299890 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:103:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:131:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #13)
> Attachment 299890 [details] did not pass style-queue:
> 
> 
> ERROR:
> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:103: 
> Should be indented on a separate line, with the colon or comma first on that
> line.  [whitespace/indent] [4]
> ERROR:
> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:117: 
> Should be indented on a separate line, with the colon or comma first on that
> line.  [whitespace/indent] [4]
> ERROR:
> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:131: 
> Should be indented on a separate line, with the colon or comma first on that
> line.  [whitespace/indent] [4]
> Total errors found: 3 in 6 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Fixing style and license comments

[Alex Christensen]

Comment on attachment 299890 [details]
Patch

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

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:56
> +class LibWebRTCMediaEndpoint final : public RefCounted<LibWebRTCMediaEndpoint>, private webrtc::PeerConnectionObserver {

This should probably be ThreadSafeRefCounted<LibWebRTCMediaEndpoint> since we are ref'ing and deref'ing on different threads.  It hurts performance a bit if we ref and deref a lot because it has to clear the instruction pipeline, but it is necessary if we are ref'ing and deref'ing from different threads.

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:47
> +static std::unique_ptr<PeerConnectionBackend> createLibWebRTCPeerConnectionBackend(RTCPeerConnection& peerConnection)
> +{
> +    return std::unique_ptr<PeerConnectionBackend>(new LibWebRTCPeerConnectionBackend(peerConnection));
> +}

This whole function shouldn't be necessary.  Just make the constructor public and call std::make_unique

> Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:53
> +    ASSERT(peerConnection.scriptExecutionContext()->isDocument());

It would be nice if we could make every scriptExecutionContext have a LibWebRTCProvider.  We would need to make LibWebRTCProvider ThreadSafeRefCounted and pass Ref's around.  I might do it later.  This is fine for now, though.

[youenn fablet]

(In reply to comment #15)
> Comment on attachment 299890 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299890&action=review
> 
> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCMediaEndpoint.h:56
> > +class LibWebRTCMediaEndpoint final : public RefCounted<LibWebRTCMediaEndpoint>, private webrtc::PeerConnectionObserver {
> 
> This should probably be ThreadSafeRefCounted<LibWebRTCMediaEndpoint> since
> we are ref'ing and deref'ing on different threads.  It hurts performance a
> bit if we ref and deref a lot because it has to clear the instruction
> pipeline, but it is necessary if we are ref'ing and deref'ing from different
> threads.

Good point, it is probably the simplest solution.


> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:47
> > +static std::unique_ptr<PeerConnectionBackend> createLibWebRTCPeerConnectionBackend(RTCPeerConnection& peerConnection)
> > +{
> > +    return std::unique_ptr<PeerConnectionBackend>(new LibWebRTCPeerConnectionBackend(peerConnection));
> > +}
> 
> This whole function shouldn't be necessary.  Just make the constructor
> public and call std::make_unique

Will use make_unique.

> > Source/WebCore/Modules/mediastream/libwebrtc/LibWebRTCPeerConnectionBackend.cpp:53
> > +    ASSERT(peerConnection.scriptExecutionContext()->isDocument());
> 
> It would be nice if we could make every scriptExecutionContext have a
> LibWebRTCProvider.  We would need to make LibWebRTCProvider
> ThreadSafeRefCounted and pass Ref's around.  I might do it later.  This is
> fine for now, though.

There is no planned support for WebRTC in workers so is it really worth it.
Having a RefCounted LibWebRTCProvider would be useful for the mock implementation too though.

[youenn fablet]

Created attachment 299910 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 299910 [details]
Patch for landing

Clearing flags on attachment: 299910

Committed r211255: <http://trac.webkit.org/changeset/211255>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.