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.16 -> 0.3.17 #105362

Merged
merged 8 commits into from Dec 3, 2020
Merged

pipewire: 0.3.16 -> 0.3.17 #105362

merged 8 commits into from Dec 3, 2020

Conversation

@gebner
Copy link
Member

@gebner gebner commented Nov 29, 2020

Motivation for this change

New upstream release.

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.
@jansol
Copy link
Contributor

@jansol jansol commented Nov 29, 2020

Changelog: https://gitlab.freedesktop.org/pipewire/pipewire/-/commit/f5f5beb0ece64c7d08edb5413dde537ec5b6c3e2

We should probably start using /etc/pipewire/media-session.d/ for enabling the compat modules.

Otherwise sound.extraConfig has no effect.
@gebner
Copy link
Member Author

@gebner gebner commented Nov 29, 2020

We should probably start using /etc/pipewire/media-session.d/ for enabling the compat modules.

The nixos module now creates these files. I think this is only effects the pipewire-media-session daemon, so we don't need to change anything else.

@gebner
Copy link
Member Author

@gebner gebner commented Nov 29, 2020

Ok, I've added some more stuff:

  • pipewire looks in /etc for configuration instead of ${pipewire}/etc (the with-* files were ignored because of this)
  • The nixos module generates a pipewire.conf file. It's ugly that we copy parts of the example configuration file, but at least we can add arguments to pipewire-media-session now.
  • alsamixer works with pipewire now, as suggested by @ilya-fedin.

On the positive side, mSBC seems to work now. (It's really fragile and you have to restart hsphfpd a few times, but hopefully that'll get better over time.) I'm using the following configuration:

{ hardware.bluetooth.hsphfpd.enable = true;

  services.pipewire = {
    enable = true;
    mediaSessionExtraArguments = [ "-p" "bluez5.msbc-support=true" ];
    alsa.enable = true;
    jack.enable = true;
    pulse.enable = true;
  }; }

cc @collares (you were commenting on the pipewire bluetooth issue)

create-object spa-node-factory factory.name=support.node.driver node.name=Dummy priority.driver=8000
exec ${cfg.package}/bin/pipewire-media-session ${lib.concatStringsSep " " cfg.mediaSessionExtraArguments}
Copy link
Member Author

@gebner gebner Nov 29, 2020

Maybe it would be better to make this whole line configurable? Wireplumber can replace pipewire-media-session: https://pipewire.pages.freedesktop.org/wireplumber/daemon/running.html?gi-language=c

Suggested change
exec ${cfg.package}/bin/pipewire-media-session ${lib.concatStringsSep " " cfg.mediaSessionExtraArguments}
exec wireplumber

Copy link
Contributor

@jansol jansol Nov 29, 2020

Sounds good, but then the whole compat setup thing should be changed a bit.
For pipewire.conf I guess exec ${cfg.sessionManager} ${lib.concatStringsSep " " cfg.sessionManagerArguments} would do, but I don't know what the best way to handle /etc/pipewire/media-session.d would be in that case?

Copy link
Contributor

@jansol jansol Nov 30, 2020

Maybe pipewire-media-session should be split into its own package? Then these options and the /etc/pipewire/media-session.d/ stuff could perhaps be done there? And switching the sessionManager package to wireplumber with whatever config setup that wants would maybe Just Work...

Copy link
Member Author

@gebner gebner Dec 2, 2020

Ok, I've added the sessionManager and sessionManagerArguments options. But yes, wireplumber support won't be as easy as setting sessionManager; it needs its own configuration file.

Copy link
Contributor

@jansol jansol Dec 2, 2020

Hmm should this be handled by something like extraEtcFiles? Or, on second thought, maybe this should be handled entirely on the module level, like services.xserver.displayManager.*? Something like services.pipewire.sessionManager.{pipewireMediaSession, wireplumber}. Considering that displayManager is a pretty visible precedent maybe that's the best solution already for consistency reasons.

Copy link
Contributor

@ilya-fedin ilya-fedin Dec 3, 2020

Just want to mention that the exec line could be dropped and session manager launched via systemd-user instead.

Copy link
Member Author

@gebner gebner Dec 3, 2020

Something like services.pipewire.sessionManager.{pipewireMediaSession, wireplumber}

I think we should wait with designing such an elaborate system until wireplumber/pipewire stabilizes a bit. For now, the current options are enough to 1) enable optional features in pipewire-media-session, and 2) change/disable the session manager if you want to run wireplumber manually.

Anything beyond this should probably wait at least until we have a wireplumber package.

Just want to mention that the exec line could be dropped and session manager launched via systemd-user instead.

That seems like a neat idea. I'm surprised that upstream doesn't do that by default.

@gebner
Copy link
Member Author

@gebner gebner commented Dec 3, 2020

I'll merge this PR now. I don't have enough time right now to refactor the session manager part. I also think it'll be more productive to discuss this when/if somebody packages wireplumber.

@gebner gebner merged commit 6e80073 into NixOS:master Dec 3, 2020
19 checks passed
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

3 participants