Bug 178860

Created attachment 325000 [details] Patch

Created attachment 334994 [details] Patch

Created attachment 335194 [details] Patch

Comment on attachment 335194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335194&action=review > Source/cmake/OptionsGTK.cmake:90 > +WEBKIT_OPTION_DEFINE(USE_LIBWEBRTC "Whether to use libwebrtc for WebRTC support" PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES}) I don't think you need a new option, since you set USE_LIBWEBRTC unconditionally below if ENABLE_MEDIA_STREAM or ENABLE_WEB_RTC is enabled. A new build option should only be needed if you were going to allow using ENABLE_MEDIA_STREAM or ENABLE_WEB_RTC with USE_LIBWEBRTC disabled. > Source/cmake/OptionsGTK.cmake:226 > + set(USE_LIBWEBRTC TRUE) > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC TRUE) I don't think you need both

(In reply to Michael Catanzaro from comment #4) > Comment on attachment 335194 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335194&action=review > > > Source/cmake/OptionsGTK.cmake:90 > > +WEBKIT_OPTION_DEFINE(USE_LIBWEBRTC "Whether to use libwebrtc for WebRTC support" PRIVATE ${ENABLE_EXPERIMENTAL_FEATURES}) > > I don't think you need a new option, since you set USE_LIBWEBRTC > unconditionally below if ENABLE_MEDIA_STREAM or ENABLE_WEB_RTC is enabled. A > new build option should only be needed if you were going to allow using > ENABLE_MEDIA_STREAM or ENABLE_WEB_RTC with USE_LIBWEBRTC disabled. > We do this for the moment but we could try to use a gstreamer webrtc backend at some point in the future and I guess in that case it makes sense people forcing this backend can keep this option? > > Source/cmake/OptionsGTK.cmake:226 > > + set(USE_LIBWEBRTC TRUE) > > + SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC TRUE) > > I don't think you need both Right. Thanks for the review!

Comment on attachment 335194 [details] Patch After r229378 we need to update the patch.

Created attachment 336194 [details] Patch

Why are we checking in a bunch of generated assembly instead of making the assembly generation part of the build process? You also only include x86_64 assembly. What about other architectures?

There's a configure option to use system OpenSSL instead of BoringSSL... does that not work? Seems like that would be *slightly* less bad.

BTW, I'm talking to GStreamer developers about using GDtlsConnection in GStreamer's WebRTC code, which we'd gain automatically once we dump libwebrtc. Currently it relies on OpenSSL via a wrapper lib. GDtlsConnection is an abstraction that will allow us to switch between different TLS libraries to avoid licensing issues; the default would be GnuTLS, because OpenSSL is unacceptable on desktop Linux, and it can be switched to OpenSSL for embedded systems where GnuTLS is unacceptable. This way, we can delete libwebrtc rather than rewriting its network code to drop the BoringSSL/OpenSSL dependency.

(In reply to Michael Catanzaro from comment #10) > BTW, I'm talking to GStreamer developers about using GDtlsConnection in > GStreamer's WebRTC code, which we'd gain automatically once we dump > libwebrtc. To be clear: that's a planned near-term enhancement to GStreamer. Currently it relies on OpenSSL via a wrapper lib.

(In reply to Michael Catanzaro from comment #9) > There's a configure option to use system OpenSSL instead of BoringSSL... > does that not work? Seems like that would be *slightly* less bad. It is supposed to work but I heard that this option is not widely used so probably not very well tested.

(In reply to Alex Christensen from comment #8) > Why are we checking in a bunch of generated assembly instead of making the > assembly generation part of the build process? You also only include x86_64 > assembly. What about other architectures? Thanks for the review! Basically I was assuming you did it this way when you pushed libwebrtc the first time because we are not running a complete compilation process of libwebrtc and all its third parties, it is smaller and simpler compilation, that is one of my goals, it really makes live easier for us when updating the code. In this patch I updated the ones I needed in my current setup and decided to start supporting that architecture in our case to add new ones in the future, this is just the first commit. But now that you say it I think it could be simpler if we can avoid this setup, so I'll try the option OPENSSL_NO_ASM. And we will add support for the generation if required. Maybe we should remove all the .S files if you are not using it.

Created attachment 336265 [details] Patch

Created attachment 336267 [details] Patch

Created attachment 336268 [details] Patch

Created attachment 336273 [details] Patch

Created attachment 336282 [details] Patch

Comment on attachment 336282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336282&action=review > Source/WebCore/platform/mediastream/RealtimeIncomingAudioSource.cpp:41 > +Ref<RealtimeIncomingAudioSource> RealtimeIncomingAudioSource::create(rtc::scoped_refptr<webrtc::AudioTrackInterface>&& audioTrack, String&& audioTrackId) Can we create new files such as RealtimeIncomingAudioSourceGlib.cpp that would contain that implementation and later on other needed stuff, similarly to RealtimeIncomingVideoSourceCocoa.cpp? You will need to specialize OnFrame anyway at some point. Ditto for other classes.

(In reply to youenn fablet from comment #19) > Comment on attachment 336282 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336282&action=review > > > Source/WebCore/platform/mediastream/RealtimeIncomingAudioSource.cpp:41 > > +Ref<RealtimeIncomingAudioSource> RealtimeIncomingAudioSource::create(rtc::scoped_refptr<webrtc::AudioTrackInterface>&& audioTrack, String&& audioTrackId) > > Can we create new files such as RealtimeIncomingAudioSourceGlib.cpp that > would contain that implementation and later on other needed stuff, similarly > to RealtimeIncomingVideoSourceCocoa.cpp? > You will need to specialize OnFrame anyway at some point. > > Ditto for other classes. Right, we have those in follow up commits, I can add a smaller version in this commit already. Thanks for the review!

Created attachment 336378 [details] Patch

Comment on attachment 336378 [details] Patch r=me as long as the bots are happy

FWIW, I imagine getting rid of the asm will significantly affect performance on small embedded devices. I doubt it would be noticeable on desktops.

(In reply to Michael Catanzaro from comment #23) > FWIW, I imagine getting rid of the asm will significantly affect performance > on small embedded devices. I doubt it would be noticeable on desktops. Sure, at the same time, when you are doing WebRTC, you are doing so many other things that I am not sure how big the impact will actually be.

Bots complain when compiling the libvpx dependency, apparently google changes the generated archive files every time you download, so I'll replace that to use the tag in the git repository and try to land. Just pinged the gtk+ and wpe bot admins anyway, because it adds a couple of dependencies and they can cause issues. Thanks everyone for the reviews!

(In reply to Michael Catanzaro from comment #23) > FWIW, I imagine getting rid of the asm will significantly affect performance > on small embedded devices. I doubt it would be noticeable on desktops. Yep, you are right. Anyway we are currently not using the asm in the raspberrry pi and we are doing quite well, we will keep this in mind when checking performance on that device for the next steps. Thanks for the comment!

Created attachment 336976 [details] Patch for landing

Comment on attachment 336976 [details] Patch for landing Clearing flags on attachment: 336976 Committed r230152: <https://trac.webkit.org/changeset/230152>

All reviewed patches have been landed. Closing bug.

<rdar://problem/39102256>