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.

Sign up for GitHub

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

Jump to bottom

chromium: Build with VA-API but disable it by default #85253

Merged
merged 1 commit into from
Apr 18, 2020
Merged

chromium: Build with VA-API but disable it by default #85253

merged 1 commit into from
Apr 18, 2020
+73 −11

Conversation

primeos
Copy link
Member

@primeos primeos commented Apr 14, 2020 •
edited
Loading

This makes it possible to enable VA-API without having to rebuild
Chromium.

Co-Authored-By: ElXreno elxreno@gmail.com
Co-Authored-By: misuzu bakalolka@gmail.com

Motivation for this change

Alternative implementation of #85076. Solves the VA-API part of #82443.

Advantages:

  • Doesn't require --ignore-gpu-blacklist, i.e. VA-API can be configured independently of other GPU features
  • Will explicitly break all existing VA-API setups (vs. silently). Bonus: We could add instructions via a throw statement.
  • Doesn't interfere with commandLineArgs. I.e. we don't accidentally enable VA-API if that argument is set.

Disadvantages:

  • More complex!

TODOs:

Tests:

  • On a working VA-API setup
  • Without any VA-API drivers
  • With XWayland (expected: VA-API doesn't work, depending on the driver)
    • enableVaapi = false: Works as expected
      • Only drawback: Additionally prints vaInitialize failed: unknown libva error
      • But: chrome://gpu now even shows the correct Video Decode information: Unavailable -> Software only. Hardware acceleration disabled
    • enableVaapi = true: Wrong VA information, but ok
      • The same as before, but chrome://gpu now incorrectly shows Video Decode: Hardware accelerated (despite the libva error and VA not actually working)
  • Optional: In a VM (virtio)
  • Invalid LIBVA_DRIVER_NAME value: Fallback to SW rendering (only wrong VA information shown, like in the second XWayland case)
  • VA-API on radeon almost works now (at least on my old HD 6950) \o/
    • Was broken before, probably fixed in the meantime by a Mesa update
    • Update: There are some minor visual glitches left at the bottom of the screen but it's watchable (vs. completely visually broken as before)
  • Tested drivers: i965, iHD, and r600 (some visual glitches with VA-API, but ok)

Note: We could also add chromium-vaapi but I'd like to avoid that for now (as we're still experimenting with it).

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.

Sorry, something went wrong.

@primeos primeos added the 2.status: work-in-progress This PR isn't done label Apr 14, 2020
@primeos primeos mentioned this pull request Apr 14, 2020
Closed
10 tasks
@ofborg ofborg bot requested review from thefloweringash and bendlas April 14, 2020 22:08
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Apr 14, 2020
@primeos primeos mentioned this pull request Apr 14, 2020
Closed
10 tasks
@offlinehacker
Copy link
Contributor

For some reason I tought this was already enabled. I think we should build with va-api by default, it's pretty common feature.

@primeos
Copy link
Member Author

primeos commented Apr 15, 2020

I added some minor improvements but it's basically a refactoring except for the Ozone "fix" and added deprecation note via throw (but the derivations for chromium and chromium.override { enableVaapi = true; } remain unchanged).

@primeos
Copy link
Member Author

primeos commented Apr 15, 2020 •
edited
Loading

@thefloweringash nearly forgot to ask: Could you test this on AArch64 to see if the build still works or if we have to disable VA-API there?

Edit: Apart from that this PR is ready for review :)

@thefloweringash
Copy link
Member

I'm not sure what to make of this.

On Aarch64 it does build and run, but the GPU thread crashes during startup and prints a backtrace including VADisplayState::Initialize. Inspecting chrome://gpu shows the following warning:

GPU process was unable to boot: GPU process crashed too many times with SwiftShader.
Disabled Features: all

Software webgl is no longer available. This happens even with the default chromium running with --disable-accelerated-video-decode --disable-accelerated-video-encode.

This could be a symptom of my particular graphics stack. I only have one graphical system to test this on at the moment and I'm using the panfrost driver which IIUC is still under active development.

@primeos
Copy link
Member Author

primeos commented Apr 16, 2020

@thefloweringash huge thanks for your testing, that's very helpful! As for AArch64 I decided to disable VA-API for now as I'm not sure if we could get this to work properly across all platforms or if we even package VA-API drivers for GPUs on ARM systems.

If you have time for it: Could you test the new version of this PR again (which would also cover #85360) to verify that the regressions are gone. The only difference for AArch64 should now be the additional --disable-accelerated-video-decode --disable-accelerated-video-encode flags (could be avoided as well but then we might risk enabling VA-API at runtime if we should build it for AArch64 in the future).

@primeos
Copy link
Member Author

primeos commented Apr 16, 2020

Note: I'll try to run some more tests on x86_64, seems like this change is more complicated than anticipated. I also realized that I did only test this on systems where I tested VA-API before and it's properly supported. I'll try to test the following cases as well:

  • Without any VA-API drivers
  • With XWayland (doesn't work, depending on the driver)
  • Optional: In a VM

@primeos primeos mentioned this pull request Apr 16, 2020
Merged
10 tasks
@thefloweringash
Copy link
Member

Tested on aarch64: some browsing, youtube and software webgl are working. LGTM.

@primeos
chromium: Build with VA-API but disable it by default
267eefc 
This makes it possible to enable VA-API without having to rebuild
Chromium: `chromium.override { enableVaapi = true; }`
@primeos
Copy link
Member Author

primeos commented Apr 18, 2020

My testing on x86_64 didn't show any important regressions, let's hope this'll work well with any GPU+driver combination (at least for the default case with enableVaapi = false). Also this PR should at least avoid the regressions from the last PR since we disable VA-API at runtime by default for now.

I'll merge it now 🎉

Note: The last force-push was just to clean up the history (rebase+squashing - no other changes).

@primeos primeos merged commit 34643fc into NixOS:master Apr 18, 2020
@offlinehacker
Copy link
Contributor

offlinehacker commented Apr 18, 2020 •
edited
Loading

enableVaapidoes not seem to be passed to mkChromiumDerivation

https://github.com/primeos/nixpkgs/blob/267eefcdb7e1ee2b0c8e1714b29125517474f54f/pkgs/applications/networking/browsers/chromium/default.nix#L37

@primeos
Copy link
Member Author

primeos commented Apr 18, 2020

@offlinehacker that's intentional, it only affects the wrapper ;)

@primeos primeos mentioned this pull request Jun 16, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@offlinehacker offlinehacker

@thefloweringash thefloweringash

@bendlas bendlas

Assignees
No one assigned
Labels
2.status: work-in-progress This PR isn't done 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@primeos @offlinehacker @thefloweringash