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

teams: fixing pulseaudio access #85451

Merged
merged 1 commit into from Apr 27, 2020
Merged

teams: fixing pulseaudio access #85451

merged 1 commit into from Apr 27, 2020

Conversation

ghost
Copy link

@ghost ghost commented Apr 17, 2020

Motivation for this change

I've discovered an issue with Microsoft Teams, related to audio quality. Wrapping it with a LD_LIBRARY_PATH fixes #85449 for me.

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.

@ghost ghost changed the title teams: fixing a pulseaudio access teams: fixing pulseaudio access Apr 17, 2020
@ofborg ofborg bot requested a review from liff April 17, 2020 16:35
Copy link
Contributor

@liff liff left a comment

Choose a reason for hiding this comment

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

LGTM, though I would be interested to know why prepending pulseaudio (and systemd) to runpath fixes the problem? Those are the first entries in RUNPATH of bin/teams already (per readelf -d).

@ghost
Copy link
Author

ghost commented Apr 17, 2020

I do not know why it works, but for me it fixed it. I'm not sure it's the cleanest solution. I used strace and saw that for some reason some pulseaudio libraries where not loaded (but since the log is a mess, I'm not even sure of this).

@adisbladis
Copy link
Member

Using LD_LIBRARY_PATH is a hack and should generally be discouraged. Couldn't you use patchelf and patch the rpath of the teams binary instead?

@liff
Copy link
Contributor

liff commented Apr 19, 2020

I think I may have found a solution that doesn’t involve changing LD_LIBRARY_PATH.

The *.node files in the package seem to be some kind of dynamic libraries loaded by Electron/Node and it seems that one of them (slimcore.node I suspect) is attempting to load pulseaudio dynamically. It seems that the RPATH from the main application binary is not taken into account when the module is attempting to dynamically load(?) pulseaudio.

Adding runtimeDependencies to the RPATH of the Node modules seems to fix the issue.

@elyhaka can you test removing the makeWrapper bits and adding these? At least for me it changes the client to “Skype” in pavucontrol, like you described.

  dontAutoPatchelf = true;

  # Include runtimeDependencies in the RPATH of the included Node modules
  # so that dynamic loading works. See: https://github.com/NixOS/nixpkgs/issues/85449
  postFixup = ''
    autoPatchelf "$out"

    runtime_rpath="${lib.makeLibraryPath runtimeDependencies}"

    for mod in $(find "$out/opt/teams" -name '*.node'); do
      mod_rpath="$(patchelf --print-rpath "$mod")"

      echo "Adding runtime dependencies to RPATH of Node module $mod"
      patchelf --set-rpath "$runtime_rpath:$mod_rpath" "$mod"
    done;
  '';

@ghost
Copy link
Author

ghost commented Apr 19, 2020

If fixes the issue indeed ! @liff : Do you want me to update my PR with this fix or you want to open another one ?

(EDIT: sorry for the miss click on "Close" while responding... 🙄)

@ghost ghost closed this Apr 19, 2020
@ghost ghost reopened this Apr 19, 2020
@liff
Copy link
Contributor

liff commented Apr 19, 2020

Updating your PR should be fine, thanks.

@ghost
Copy link
Author

ghost commented Apr 19, 2020

Done !

Maybe this fix should be backported to 20.03 ?

@ofborg ofborg bot requested a review from liff April 19, 2020 12:08
@ghost
Copy link
Author

ghost commented Apr 27, 2020

Is there anything still blocking this PR ? :)

@ofborg ofborg bot requested a review from liff April 27, 2020 13:22
@Mic92
Copy link
Member

Mic92 commented Apr 27, 2020

Result of nixpkgs-review pr 85451 1

1 package built:
- teams

@Mic92 Mic92 merged commit 31bc167 into NixOS:master Apr 27, 2020
@Mic92
Copy link
Member

Mic92 commented Apr 27, 2020

Is a backport required?

@ghost
Copy link
Author

ghost commented Apr 27, 2020

I believe this issue also exists on 20.03, I'm not sure to be honest ? Is there an easy way to try without switching my whole system to 20.03 ?

@Mic92
Copy link
Member

Mic92 commented Apr 27, 2020

git checkout release-20.03 && git cherry-pick -x 73e46470852753b3f3e22bf79bd4230ff0e1cdc2 && nix-shell -I nixpkgs=$PWD -p teams

@ghost
Copy link
Author

ghost commented Apr 27, 2020

Thank you for the command ! Without this commit I do reproduce the issue on the 20.03.

@Mic92
Copy link
Member

Mic92 commented Apr 27, 2020

Backport:
[detached HEAD b4f2f29] teams: fixing pulseaudio access
Author: Elyhaka 57923898+Elyhaka@users.noreply.github.com
Date: Fri Apr 17 18:23:31 2020 +0200
1 file changed, 22 insertions(+)
Press Enter to continue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Microsoft teams: while calling, teams fallbacks on ALSA plugin back-end which butchers the output
3 participants