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

pipewire: 0.3.15 -> 0.3.16 #104553

Open
wants to merge 1 commit into
base: master
from
Open

pipewire: 0.3.15 -> 0.3.16 #104553

wants to merge 1 commit into from

Conversation

@jansol
Copy link
Contributor

@jansol jansol commented Nov 22, 2020

Changes: https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/09d373f094f0e6797aef3d97cde2c0167dccc986

Follow-up on #104504. Feel free to open a replacement PR if someone ends up finishing work on this while I'm asleep. (@primeos)

Not sure if we want to deal with coexistence actual pulseaudio and pipewire.pulse-server in the nixos module at all, feel free to remove that bit if the consensus is no.

Bluetooth would be nice to have a setting for since it is off by default due to conflicts with pulseaudio, but that can be done separately if it takes too much time to figure out now. I'm not familiar with modifying applications' config files from within nix.

Motivation for this change

Upstream update that should fix a lot of the current pulseaudio woes by throwing out the libpulseaudio shim and instead providing a module that acts as a fake pulseaudio server.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)

  • Built on platform(s)

    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)

  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"

  • Tested execution of all binary files (usually in ./result/bin/)

  • Determined the impact on package closure size (by running nix path-info -S before and after)

  • Ensured that relevant documentation is up to date

  • Fits CONTRIBUTING.md.

Copy link
Member

@primeos primeos left a comment

Awesome, thanks! I've added a few of my nitpicks/thoughts. Not sure how WIP this is so feel free to ignore them for now ;)

nixos/modules/services/desktops/pipewire.nix Outdated Show resolved Hide resolved
nixos/modules/services/desktops/pipewire.nix Outdated Show resolved Hide resolved
nixos/modules/services/desktops/pipewire.nix Outdated Show resolved Hide resolved
nixos/modules/services/desktops/pipewire.nix Outdated Show resolved Hide resolved
bluetooth = {
# TODO: add "-e bluez5" to 'exec pipewire-media-session' in pipewire.conf
enable = mkEnableOption "Load bluez5 module";
Comment on lines 52 to 54

This comment has been minimized.

@primeos

primeos Nov 22, 2020
Member

Seems difficult to achieve with the current module. We could also ignore this for now if you want.

This comment has been minimized.

@jansol

jansol Nov 22, 2020
Author Contributor

Since upstream supports bluetooth I'd very much like to have the option to use it, I just have no idea how to do it.

@jansol jansol force-pushed the jansol:pipewire branch 3 times, most recently from ad96082 to 96fd13e Nov 22, 2020
@jansol jansol marked this pull request as ready for review Nov 22, 2020
@jansol
Copy link
Contributor Author

@jansol jansol commented Nov 22, 2020

Removed bluetooth option for now to make this mergeable sooner.

@jansol jansol force-pushed the jansol:pipewire branch from 96fd13e to 944560e Nov 22, 2020
@jansol jansol force-pushed the jansol:pipewire branch from 944560e to 68bb5e3 Nov 22, 2020
@ofborg ofborg bot requested a review from jtojnar Nov 22, 2020
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 22, 2020

There is also another path in

https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/7ca8491be3a730cba61ea0578b654a83bcdd552c/src/daemon/meson.build#L21

but if I am not missing something, that is only used in a comment in the config file:

https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/7ca8491be3a730cba61ea0578b654a83bcdd552c/src/daemon/pipewire.conf.in

and would cause a dependency cycle if corrected. We might need to watch monitor it in case it gets used somewhere else in the future.

@jansol
Copy link
Contributor Author

@jansol jansol commented Nov 22, 2020

The path to the pulse server needs to be patched at least here

What's the preferred way to generate patches for nixpkgs?

but if I am not missing something, that is only used in a comment in the config file

Yeah that is only for when one wants to launch pipewire with the pulse bridge loaded automatically. The comment even mentions that loading the pulse bridge with a systemd service is preferred. And that's this PR is doing now, assuming I got it right.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Nov 22, 2020

What's the preferred way to generate patches for nixpkgs?

I just clone the repo, change something and then do git diff > foo.patch. Others prefer using quilt but I haven’t figured that out yet.

@primeos primeos mentioned this pull request Nov 22, 2020
3 of 10 tasks complete
@jansol jansol force-pushed the jansol:pipewire branch from 68bb5e3 to 22f5b97 Nov 22, 2020
This release replaces the libpulseaudio shim with a pipewire module that acts as a fake pulseaudio server along with a systemd service that loads that module on demand.
@jansol jansol force-pushed the jansol:pipewire branch from 22f5b97 to aca9784 Nov 23, 2020
@ofborg ofborg bot requested a review from jtojnar Nov 23, 2020
@jtojnar jtojnar requested a review from eadwu Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.