Bug 178860 – [GTK] Make libwebrtc backend buildable for GTK port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Alejandro G. Castro]

Add the cmake compilation file, required dependencies and options to make it build for the GTK port.

[Alejandro G. Castro]

Created attachment 325000 [details]
Patch

[Alejandro G. Castro]

Created attachment 334994 [details]
Patch

[Alejandro G. Castro]

Created attachment 335194 [details]
Patch

[Michael Catanzaro]

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

[Alejandro G. Castro]

(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!

[Alejandro G. Castro]

Comment on attachment 335194 [details]
Patch

After r229378 we need to update the patch.

[Alejandro G. Castro]

Created attachment 336194 [details]
Patch

[Alex Christensen]

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?

[Michael Catanzaro]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[youenn fablet]

(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.

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 336265 [details]
Patch

[Alejandro G. Castro]

Created attachment 336267 [details]
Patch

[Alejandro G. Castro]

Created attachment 336268 [details]
Patch

[Alejandro G. Castro]

Created attachment 336273 [details]
Patch

[Alejandro G. Castro]

Created attachment 336282 [details]
Patch

[youenn fablet]

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.

[Alejandro G. Castro]

(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!

[Alejandro G. Castro]

Created attachment 336378 [details]
Patch

[youenn fablet]

Comment on attachment 336378 [details]
Patch

r=me as long as the bots are happy

[Michael Catanzaro]

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

[youenn fablet]

(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.

[Alejandro G. Castro]

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!

[Alejandro G. Castro]

(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!

[Alejandro G. Castro]

Created attachment 336976 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 336976 [details]
Patch for landing

Clearing flags on attachment: 336976

Committed r230152: <https://trac.webkit.org/changeset/230152>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39102256>