Skip to content

WIP: firefox: enable pulseaudio by default in wrapper#24012

Closed
corngood wants to merge 1 commit intoNixOS:masterfrom
corngood:firefox-pulse
Closed

WIP: firefox: enable pulseaudio by default in wrapper#24012
corngood wants to merge 1 commit intoNixOS:masterfrom
corngood:firefox-pulse

Conversation

@corngood
Copy link
Contributor

@corngood corngood commented Mar 18, 2017

Motivation for this change

Firefox 52.0 removed the alsa backend, so without pulseaudio in LD_LIBRARY_PATH you get no audio.

WIP because:

  • How does this affect other uses of the wrapper (I only tested firefox 52)?

  • Is the predicate correct?
    config.pulseaudio or true is what's used for pulse support in chromium. Wine uses config.pulseaudio or stdenv.isLinux.

  • Should we just enable the ALSA backend instead?
    See firefox: reenable ALSA backend #23989

  • Should we require the user to set config.pulseaudio?
    Most other packages don't seem to require this. Especially ones without alsa support.

  • Should we add a pulseSupport parameter like in chromium and opt in/out where appropriate?

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@corngood, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @edolstra and @vcunat to be potential reviewers.

@joachifm joachifm added the 9.needs: community feedback This needs feedback from more community members. label Mar 19, 2017
@edolstra
Copy link
Member

See also #23989, which re-enables the ALSA backend.

@grahamc grahamc closed this Jul 16, 2017
@dtzWill
Copy link
Member

dtzWill commented Feb 7, 2018

ALSA backend is enabled, but it still seems useful to include pulseaudio in the wrapper by default?

  • ALSA will still work if user does not use pulseaudio
  • Worries about closure size can be addressed by users with config.pulseaudio = false, which they need to set anyway since firefox has dependencies that pull it along anyway/by default[1].
  • Users on other distributions that default to pulseaudio don't have to discover magical config options to get things working (Firefox doesn't see host pulseaudio, audio doesn't work #34712)

I haven't tested the first point-- but unless that's false (?) then ... can we default to this being enabled for usability's sake?

FWIW a NixOS system with pulseaudio enabled does not necessarily have "config.pulseaudio" set one way or the other.

[1]

$ nix why-depends -f . firefox libpulseaudio --all
/nix/store/6sblfm52hkvzr2b4fp8y5bn8fvj2j6ag-firefox-58.0.1
╠═══bin/firefox: …D_LIBRARY_PATH:+':'}'/nix/store/07gd5ls8bbqz4kkmb5rycvn9cayzqmdh-ffmpeg-3.4.1/lib:/nix/store/22h…
║   => /nix/store/07gd5ls8bbqz4kkmb5rycvn9cayzqmdh-ffmpeg-3.4.1
║   ╠═══lib/libavdevice.so.57.10.100: …mdh-ffmpeg-3.4.1/lib:/nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0/lib:/nix/sto…
║   ║   => /nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0
║   ╚═══lib/libavdevice.so.57.10.100: ….6.libavdevice.so.57./nix/store/mgwv5454g23gh11hcmzc9v7jfh1prwgk-SDL2-2.0.7/lib:/nix/store/07gd5…
║       => /nix/store/mgwv5454g23gh11hcmzc9v7jfh1prwgk-SDL2-2.0.7
║       ╚═══lib/libSDL2-2.0.so.0.7.0: …x-alsa-lib-1.1.5/lib:/nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0/lib:/nix/sto…
║           lib/libSDL2.la: …5/lib/libasound.la -L/nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0/lib /nix/sto…
║           => /nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0
╚═══bin/firefox: …K_PATH${GTK_PATH:+:}'/nix/store/xflgnag4wwc2s0w4chsaggxrf62dc0d1-libcanberra-0.30/lib/gtk-2.0/'.…
    => /nix/store/xflgnag4wwc2s0w4chsaggxrf62dc0d1-libcanberra-0.30
    ╚═══lib/libcanberra-0.30/libcanberra-pulse.la: ….dependency_libs=' -L/nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0/lib /nix/sto…
        lib/libcanberra-0.30/libcanberra-pulse.so: …libcanberra-pulse.so./nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0/lib:/nix/sto…
        => /nix/store/mkcv47viwcy654wgdckja0mkxxvpjs9g-libpulseaudio-11.0

@oxij
Copy link
Member

oxij commented Feb 9, 2018 via email

@dtzWill
Copy link
Member

dtzWill commented Feb 9, 2018

Okay, well I'm just saying it's already included in the closure of firefox so I'm not sure it helps anyone to avoid it in the wrapper. Doing so seems to only break some setups (non-NixOS pulseaudio users) and nothing else... which is why it seems we might as well help firefox find its libraries.

(I would not suggest this if it wasn't already being included)

Does it help anyone to continue with the current behavior?

@oxij
Copy link
Member

oxij commented Feb 11, 2018 via email

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

Labels

9.needs: community feedback This needs feedback from more community members.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants