Bug 167293 – [WebRTC][Mac] Activate libwebrtc

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

LibWebRTC endpoint should be implemented behind a compilation flag until the implementation can actually be deployed.
This will allow not put burden on others to compile/link libwebrtc in this early phase

[youenn fablet]

Created attachment 299472 [details]
Patch

[youenn fablet]

Created attachment 299481 [details]
Patch

[Alex Christensen]

Comment on attachment 299481 [details]
Patch

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

I think this should be USE(LIBWEBRTC) instead.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:168
>  ENABLE_WEB_RTC = ENABLE_WEB_RTC;
> +ENABLE_LIBWEBRTC_ENDPOINT = ;

It is nice to have these two together, but I think the intent is that this list be alphabetically sorted.

[youenn fablet]

Created attachment 300087 [details]
Misc patch

[youenn fablet]

Created attachment 300865 [details]
Patch

[Geoffrey Garen]

<rdar://problem/30401864>

[youenn fablet]

Created attachment 300930 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 300930 [details]
Patch

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

> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:434
>  (with-filter (extension "com.apple.webkit.microphone")
>      (allow device-microphone))
>  
> +(allow device-microphone)

This doesn't look right. Why do we allow microphone both conditionally and unconditionally?

Also, what tracks removing the unconditional rule?

[Brent Fulgham]

Comment on attachment 300930 [details]
Patch

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

> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:201
> +(allow network*)

Yikes!

>> Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:434
>> +(allow device-microphone)
> 
> This doesn't look right. Why do we allow microphone both conditionally and unconditionally?
> 
> Also, what tracks removing the unconditional rule?

This shouldn't be needed after Bug 167669.

[youenn fablet]

(In reply to comment #8)
> Comment on attachment 300930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300930&action=review
> 
> > Source/WebKit2/WebProcess/com.apple.WebProcess.sb.in:434
> >  (with-filter (extension "com.apple.webkit.microphone")
> >      (allow device-microphone))
> >  
> > +(allow device-microphone)
> 
> This doesn't look right. Why do we allow microphone both conditionally and
> unconditionally?
> 
> Also, what tracks removing the unconditional rule?

This patch is not intended to be landed as is. But it allows testing/demoing WebKit with webrtc.

[Brent Fulgham]

Comment on attachment 300930 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/mac/com.apple.WebKit.NetworkProcess.sb.in:201
>> +(allow network*)
> 
> Yikes!

This shouldn't be needed after Bug 168010.

[Alex Christensen]

Created attachment 300980 [details]
Patch

[Brent Fulgham]

Created attachment 301070 [details]
Patch

[Alex Christensen]

Created attachment 301119 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r212040

Comment on attachment 301119 [details]
Patch

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

> Source/WebKit2/Configurations/WebKit.xcconfig:47
> +OTHER_LDFLAGS = $(inherited) $(UNEXPORTED_SYMBOL_LDFLAGS) $(ASAN_OTHER_LDFLAGS) $(FRAMEWORK_AND_LIBRARY_LDFLAGS) $(OTHER_LDFLAGS_PLATFORM) -framework WebKitLegacy -sub_umbrella WebKitLegacy -lboringssl -lwebrtc;

Why do these static libraries need to be linked into both WebCore and WebKit? WebKit already links against WebCore, and it seems like this either already does or can easily lead to redundancy.

[youenn fablet]

(In reply to comment #16)
> Comment on attachment 301119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301119&action=review
> 
> > Source/WebKit2/Configurations/WebKit.xcconfig:47
> > +OTHER_LDFLAGS = $(inherited) $(UNEXPORTED_SYMBOL_LDFLAGS) $(ASAN_OTHER_LDFLAGS) $(FRAMEWORK_AND_LIBRARY_LDFLAGS) $(OTHER_LDFLAGS_PLATFORM) -framework WebKitLegacy -sub_umbrella WebKitLegacy -lboringssl -lwebrtc;
> 
> Why do these static libraries need to be linked into both WebCore and
> WebKit? WebKit already links against WebCore, and it seems like this either
> already does or can easily lead to redundancy.

We'll work on removing these for WebKit and WebKit2.

[Alex Christensen]

Rolled out in https://trac.webkit.org/r212042

[Chris Dumez]

Seems to have caused failures?
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r212040%20(3463)/results.html

[Alex Christensen]

And it broke a bunch of internal build stuff.

[Alex Christensen]

Comment on attachment 301119 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1225
> +#if PLATFORM(COCOA)

32-bit El Capitan will not work because of ARC.  We should probably disable it for El Capitan, for 32-bit, or for both.

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 301119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301119&action=review
> 
> > Source/WTF/wtf/Platform.h:1225
> > +#if PLATFORM(COCOA)
> 
> 32-bit El Capitan will not work because of ARC.  We should probably disable
> it for El Capitan, for 32-bit, or for both.

We should try to remove all files requiring -fobj-arc for mac builds.
For iOS builds, we might need to keep a few of them.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 301199 [details]
Patch

[youenn fablet]

Created attachment 301202 [details]
Patch

[Build Bot]

Comment on attachment 301202 [details]
Patch

Attachment 301202 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3065236

New failing tests:
fast/mediastream/RTCPeerConnection-icecandidate-event.html

[Build Bot]

Created attachment 301219 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301202 [details]
Patch

Attachment 301202 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3065256

New failing tests:
imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html
imported/w3c/web-platform-tests/webrtc/no-media-call.html
imported/w3c/web-platform-tests/webrtc/promises-call.html

[Build Bot]

Created attachment 301221 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301202 [details]
Patch

Attachment 301202 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3065288

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl.html
imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html
imported/w3c/web-platform-tests/webrtc/no-media-call.html
imported/w3c/web-platform-tests/webrtc/promises-call.html
imported/w3c/web-platform-tests/webrtc/RTCDataChannelEvent-constructor.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

[Build Bot]

Created attachment 301226 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 301702 [details]
Trying the dylib way

[youenn fablet]

Created attachment 301708 [details]
Adding some more exports

[Alex Christensen]

Comment on attachment 301708 [details]
Adding some more exports

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

> Source/WebCore/Configurations/WebCore.xcconfig:69
> +LIBWEBRTC_LDFLAGS = -lwebrtc -framework CoreMedia -framework VideoToolbox;

Instead of -lwebrtc let's do -weak_library <full path to libwebrtc from macros>

[Build Bot]

Comment on attachment 301708 [details]
Adding some more exports

Attachment 301708 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3126647

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl.html
imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html
imported/w3c/web-platform-tests/webrtc/no-media-call.html
imported/w3c/web-platform-tests/webrtc/promises-call.html
imported/w3c/web-platform-tests/webrtc/RTCDataChannelEvent-constructor.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

[Build Bot]

Created attachment 301718 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301708 [details]
Adding some more exports

Attachment 301708 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3132449

New failing tests:
fast/mediastream/RTCPeerConnection-icecandidate-event.html

[Build Bot]

Created attachment 301738 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301708 [details]
Adding some more exports

Attachment 301708 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3132824

New failing tests:
imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html
imported/w3c/web-platform-tests/webrtc/no-media-call.html
imported/w3c/web-platform-tests/webrtc/promises-call.html

[Build Bot]

Created attachment 301751 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 301708 [details]
Adding some more exports

Attachment 301708 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3132662

New failing tests:
imported/w3c/web-platform-tests/webrtc/datachannel-emptystring.html
imported/w3c/web-platform-tests/webrtc/no-media-call.html
fast/scrolling/ios/scroll-events-back-forward.html
imported/w3c/web-platform-tests/webrtc/promises-call.html

[Build Bot]

Created attachment 301766 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

(In reply to comment #34)
> Comment on attachment 301708 [details]
> Adding some more exports
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301708&action=review
> 
> > Source/WebCore/Configurations/WebCore.xcconfig:69
> > +LIBWEBRTC_LDFLAGS = -lwebrtc -framework CoreMedia -framework VideoToolbox;
> 
> Instead of -lwebrtc let's do -weak_library <full path to libwebrtc from
> macros>

Right.
The question is where should we put the libwebrtc library, at the root folder as currently done or within a Framework subfolder like done for other dylib?
I am not sure I have the right settings currently. Mitz, any idea?

[youenn fablet]

Or maybe use -weak-lwebrtc for the linking

(In reply to comment #44)
> Or maybe use -weak-lwebrtc for the linking

Yeah, that’d be better.

But independently of that you still have to decide the install path and install name for the dylib in all build configurations.

I think in iOS builds the install path and install name can be
/System/Library/PrivateFrameworks/WebCore.framework/Frameworks/libwebrtc.dylib

In normal macOS builds (when WK_USE_OVERRIDE_FRAMEWORKS_DIR is not set to YES) the install path can be
/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks/libwebrtc.dylib
which can also be the install name.

In macOS builds when WK_USE_OVERRIDE_FRAMEWORKS_DIR is set to YES, the install path can be $(WK_OVERRIDE_FRAMEWORKS_DIR) and the install name can be @loader_path/../../../libwebrtc.dylib (which is the path from the WebCore binary to the dylib), assuming the use of @loader_path is compatible with Apple’s combination of use of restricted entitlements and library validation on the various executables in Safari Technology Preview and in its Safari updates.

The libwebrtc headers can probably go into a webrtc subdirectory of WebCore’s PrivateHeaders directory (WebCore’s configuration computes where that ends up being).

[youenn fablet]

Created attachment 301967 [details]
Patch

[Alex Christensen]

Comment on attachment 301967 [details]
Patch

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

!

> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:23
> -EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = audio_device_ios.mm voice_processing_audio_unit.mm audio_device_not_implemented_ios.mm RTCAudioSessionConfiguration.m RTCAudioSessionDelegateAdapter.mm RTCAudioSession.mm RTCAudioSession+Configuration.mm RTCUIApplication.mm;
> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = voice_processing_audio_unit.mm;

I remember excluding these because 32-bit El Capitan can't build with -fobjc-arc.  Are these files needed now?

> Source/WebKit2/UIProcess/WebPreferences.cpp:208
> +    return true;

return false if libwebrtc isn't there?

[youenn fablet]

Comment on attachment 301967 [details]
Patch

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

>> Source/ThirdParty/libwebrtc/Configurations/libwebrtc.xcconfig:23
>> +EXCLUDED_SOURCE_FILE_NAMES[sdk=macosx*] = voice_processing_audio_unit.mm;
> 
> I remember excluding these because 32-bit El Capitan can't build with -fobjc-arc.  Are these files needed now?

These files are no longer part of the build system.

>> Source/WebKit2/UIProcess/WebPreferences.cpp:208
>> +    return true;
> 
> return false if libwebrtc isn't there?

If we are not using libwebrtc, we always activate WebRTC.
This should allow taking care of GTK port.

[Build Bot]

Comment on attachment 301967 [details]
Patch

Attachment 301967 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3145011

New failing tests:
editing/spelling/spellcheck-async.html

[Build Bot]

Created attachment 301987 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Comment on attachment 301967 [details]
Patch

Test failure is probably unrelated

[Alex Christensen]

http://trac.webkit.org/r212699

[Alex Christensen]

And out again in http://trac.webkit.org/r212704

[Alex Christensen]

Created attachment 302353 [details]
Patch

Comment on attachment 302353 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1218
> +asdf

qwer

[Joseph Pecoraro]

Comment on attachment 302353 [details]
Patch

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

> Source/WebKit2/Configurations/WebKit.xcconfig:50
> +LIBWEBRTC_LDFLAGS_ =;

Style: Normally we have a space between `=` and `;` for empty values. Sometimes we just leave it off entirely, but I like seeing it explicitly.

> Tools/Scripts/webkitdirs.pm:2154
> +    prependToEnvironmentVariableList("DYLD_LIBRARY_PATH", $dyldFrameworkPath);

This looks identical to the line above it. Perhaps one of these was meant to be __XPC_DYLD_FRAMEWORK_PATH to cover the same 4 as above?

[youenn fablet]

Comment on attachment 302353 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1216
> +#if PLATFORM(COCOA) && ENABLE(WEB_RTC)

If we are going COCOA, then we should probably also link iOS/iOSsimulator webcore/webkit2 to libwebrtc.
That would be useful.

>> Tools/Scripts/webkitdirs.pm:2154
>> +    prependToEnvironmentVariableList("DYLD_LIBRARY_PATH", $dyldFrameworkPath);
> 
> This looks identical to the line above it. Perhaps one of these was meant to be __XPC_DYLD_FRAMEWORK_PATH to cover the same 4 as above?

Right!

[Alex Christensen]

Created attachment 302362 [details]
Patch

Comment on attachment 302362 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2153
> +    prependToEnvironmentVariableList("__XPC_DYLD_FRAMEWORK_PATH", $dyldFrameworkPath);

What is this for?

[youenn fablet]

Patch has my name on it although Alex updated it so I am not sure whether I can r+.
But anyway, r=me, let's land it and move forward!

[Alex Christensen]

Created attachment 302364 [details]
Patch

Comment on attachment 302364 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2153
> +    prependToEnvironmentVariableList("__XPC_DYLD_FRAMEWORK_PATH", $dyldFrameworkPath);

What is this for?

[Alex Christensen]

Created attachment 302368 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 302368 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:171
> +ENABLE_WEB_RTC_Production = ;
> +ENABLE_WEB_RTC_Debug = ENABLE_WEB_RTC;
> +ENABLE_WEB_RTC_Release = ENABLE_WEB_RTC;

This looks unusual. We don't differentiate between release and production builds when running tests, so how can we enable any tests when the feature is disabled in production builds?

[Alex Christensen]

Created attachment 302370 [details]
Patch

[Alex Christensen]

(In reply to comment #64)
> Comment on attachment 302368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302368&action=review
> 
> > Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:171
> > +ENABLE_WEB_RTC_Production = ;
> > +ENABLE_WEB_RTC_Debug = ENABLE_WEB_RTC;
> > +ENABLE_WEB_RTC_Release = ENABLE_WEB_RTC;
> 
> This looks unusual. We don't differentiate between release and production
> builds when running tests, so how can we enable any tests when the feature
> is disabled in production builds?
The point of this patch is to get some of the bots building webrtc.  For the purposes of this patch, we could skip all the webrtc tests in all release/production builds and I think this patch would still be a huge step in the right direction: enabling building on some bots to catch build regressions and running some tests (even if debug only) on some bots.  We can improve from there, but we're having enough trouble getting something building somewhere I think this is the least of our worries.

[Alex Christensen]

Created attachment 302372 [details]
Patch

[Alex Christensen]

Created attachment 302373 [details]
Patch

[Alex Christensen]

Comment on attachment 302373 [details]
Patch

http://trac.webkit.org/r212812

[Alex Christensen]

(In reply to comment #64)
> We don't differentiate between release and production
> builds when running tests, so how can we enable any tests when the feature
> is disabled in production builds?
Maybe we should skip all webrtc tests on all production/release builds until we get the production builds up and running.  I think it would be quite useful to run them on debug bots until then.

[Build Bot]

Comment on attachment 302373 [details]
Patch

Attachment 302373 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3171237

New failing tests:
editing/spelling/spellcheck-async.html

[Build Bot]

Created attachment 302378 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Alex Christensen]

http://trac.webkit.org/r212849