Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gstreamer: enable webrtc plugin by default #183884

Closed
wants to merge 2 commits into from

Conversation

razvanphp
Copy link

@razvanphp razvanphp commented Sep 7, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This PR enables gstreamer webrtc plugin by default.

Motivation

Since 2021 webrtc is recommended by W3C for web real-time communication, assigning multiple IETF standards for the same purpose. This plugin is a part of the GStreamer project since early 2018, when webrtcbin was merged and become part of the GStreamer 1.14 release. Since then, the team at Centricular has been working on improving the plugin and adding new features. This plugin is now considered stable and is used by many projects, including popular open-source WebRTC servers.

Issues

I've read this comment from another attempt, but I think I solved it easier, since libnice compiles without gstreamer dependancy.

Tests

I've compiled both gstreamer and libnice locally and tested with brew test & a local app I have developed in python, all is working well and gstreamer finds the necessary plugins (webrtcbin, libnice) on it's own from the brew plugins folder, using the latest version of libnice (which is good).

Screenshot 2024-09-07 at 23 22 35

References

CC @aconchillo @carlocab @ystreet

Since 2021 webrtc is recommended by W3C for web real-time communication, assigning multiple IETF standards for the same purpose. This plugin is a part of the GStreamer project since early 2018, when webrtcbin was merged and become part of the GStreamer 1.14 release. Since then, the team at Centricular has been working on improving the plugin and adding new features. This plugin is now considered stable and is used by many projects, including popular open-source WebRTC servers.
@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request long build Set a long timeout for formula testing labels Sep 7, 2024
@razvanphp razvanphp changed the title gstreamer: add webrtc plugin gstreamer: enable webrtc plugin by default Sep 7, 2024
Copy link
Contributor

github-actions bot commented Sep 7, 2024

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but this approach doesn't work either.

I think you'll need to make the approach at #171902 work instead.

@razvanphp
Copy link
Author

razvanphp commented Sep 12, 2024

Thanks, but this approach doesn't work either.

I think you'll need to make the approach at #171902 work instead.

Thank you for the quick review!

Unfortunately my limited knowledge in meson builds probably makes me the wrong guy for the task, but looking forward for the gstreamer to hop-in hopefully. @nirbheek maybe?

Cheers,
R

@nirbheek
Copy link

Unfortunately my limited knowledge in meson builds probably makes me the wrong guy for the task, but looking forward for the gstreamer to hop-in hopefully. @nirbheek maybe?

Unfortunately I know next-to-nothing about Brew, but I am happy to answer any questions you might have, and help with patches to meson build files in libnice or gstreamer.

@razvanphp
Copy link
Author

razvanphp commented Sep 13, 2024

Ok, great, then let's do this.

We need a way to compile gstreamer either:

  1. with using the local libnice if found, otherwise pull the latest libnice source, like it is currently doing
  2. have a flag like --use-local-libnice in the compiler

Here it is the build command ussed by brew (with args defined above:

system "meson", "setup", "build", *args, *std_meson_args

@nirbheek
Copy link

nirbheek commented Sep 13, 2024

That's what https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/6871 is for. Here's what you need to do:

  1. Add -Dgstreamer=disabled to libnice.rb
  2. Apply that patch to gstreamer.rb
  3. Add -Dlibnice-gstreamer-only=true to the options in gstreamer.rb. In this mode, gstreamer will download and build libnice only for the gstreamer plugin, and will use the library libnice from the system.

That's it, this breaks the circular dependency.

@razvanphp razvanphp force-pushed the include-gstreamer-webrtc branch from fdbfdd3 to a5cb47f Compare September 14, 2024 22:37
@razvanphp
Copy link
Author

There is a conflict on meson_options.txt in that MR.

I did the required changes.

To be honest, I hope for a cleaner solution on the gstreamer build side. I think this should be the default, check first if there is a libnice installed already via pkg-config instead of duplicating the build job.

@razvanphp razvanphp force-pushed the include-gstreamer-webrtc branch from a5cb47f to 549839a Compare September 14, 2024 22:53
@razvanphp razvanphp force-pushed the include-gstreamer-webrtc branch from 549839a to 18f09f2 Compare September 14, 2024 23:25
@nirbheek
Copy link

I think this should be the default, check first if there is a libnice installed already via pkg-config instead of duplicating the build job.

The gstnice plugin is not a part of the gstreamer source code. It's part of the libnice source code. GStreamer already does what you describe when pulling in libnice for the webrtc library.

@carlocab
Copy link
Member

You need to fix the brew style failures. Try running brew style gstreamer libnice with this branch checked out at "$(brew --repo homebrew/core)". Some of them might be fixed automatically with brew style --fix gstreamer libnice.

Also, please split your commits so that each commit only modifies one formula at a time.

  1. Add -Dlibnice-gstreamer-only=true to the options in gstreamer.rb. In this mode, gstreamer will download and build libnice only for the gstreamer plugin, and will use the library libnice from the system.

This will break when we start blocking formulae from downloading things at build-time, so it would be good to avoid downloading libnice during a meson invocation. This is a big part of why I took the approach I did in #171902.

@nirbheek
Copy link

nirbheek commented Sep 17, 2024

so it would be good to avoid downloading libnice during a meson invocation

If formulae can specify a fetch step manually, you can just run meson subprojects download libnice (or download the tarball and manually place it into subprojects/packagecache/). During setup, you should pass meson setup -Dwrap-mode=nodownload so that meson doesn't download anything automatically.

@carlocab
Copy link
Member

This is done in #171902.

You'll need to

brew update
brew upgrade gstreamer
brew install libnice-gstreamer

to pick up the changes. (You might need to wait 20 minutes or so for the formulae.brew.sh to pick up the update.)

@carlocab carlocab closed this Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge-skip `brew pr-automerge` will skip this pull request long build Set a long timeout for formula testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants