chromium (72): Add hardware video decoding using VAAPI #56973

amazari · 2019-03-06T18:28:03Z

Motivation for this change

Using HW video decoding should improve performances and "smoothness" while reducing battery consumption.
Seems to be an often requested feature per issue #21481

Things done

- Introduces a VAAPISupport parameter to the expression (default to false) - Import patches from the Arch package - Enable use_vaapi gn parameter - Put libva.so in dlopen seach path by tweaking LD_LIBRARY_PATH env var in the launcher script.

amazari · 2019-03-06T18:36:12Z

This supersedes #56794 where I messed up the git history.
Here the different commits of the aforementioned PR are merged into one and a fix was added to expose libva in dlopen search path, using the method provided by @colemickens.

I can confirm that videos are indeed decoded through libva, both with the i965 and iHD. Checked in chromium logs, chrome://media-internals and gpu-top.

Instead of explicitly setting LD_LIBRARY_PATH to expose the lib, I'd much prefer using the ''runtimeDependencies'' mechanism that modifies the binary rpath. I guess some post-build hooks should be set up. I'll look into it.

@colemickens @hyperfekt @yorickvP

colemickens · 2019-03-06T19:55:39Z

pkgs/applications/networking/browsers/chromium/common.nix

@@ -143,6 +145,11 @@ let
      # ++ optional (versionRange "68" "72") ( githubPatch "<patch>" "0000000000000000000000000000000000000000000000000000000000000000" )
    ] ++ optionals (!stdenv.cc.isClang && (versionRange "71" "72")) [
      ( githubPatch "65be571f6ac2f7942b4df9e50b24da517f829eec" "1sqv0aba0mpdi4x4f21zdkxz2cf8ji55ffgbfcr88c5gcg0qn2jh" )
+    ] ++ optionals (VAAPISupport && (versionRange "72" "73")) [
+      ./patches/enable-vaapi-72.patch
+      ./patches/vaapi-relax-the-version-check-for-VA-API-72.patch


I'm not sure if there's any reason to include the libva version relaxations, they're already in the tree anyway (just a matter of time until the patches don't apply), and it shouldn't be a problem, right? (this comment applies to both of the VA_CHECK patches)

Are they in the tree ? What release will they be included in ? I haven't really checked chromium history log tbh.
My thinking was to only apply those to 72.xx as 73 should barely need any patching. I think versionRange is non-inclusive in its upper range.

I just meant, since we're on NixOS and we know compile time and runtime libva will match, we can save ourselves all of the headache and ignore these 2 VA_CHECK patches. In the short-term, they don't matter to us, in the longer-term, they will be upstream anyway.

Otherwise, I think everything is in chromium upstream (maybe not in any release yet) except the "enable-mojo" patch (and it's my understanding that Chromium isn't interested in taking that patch upstream). I can dig up the links to upstream commits if you're curious for the use_vaapi patch and the 2 VA_CHECK patches.

To put it another way, if all three channels build with this and these patches still apply, then I think it's fine you have it here.

colemickens · 2019-03-06T19:56:08Z

pkgs/applications/networking/browsers/chromium/common.nix

+    ] ++ optionals (VAAPISupport && (versionRange "72" "73")) [
+      ./patches/enable-vaapi-72.patch
+      ./patches/vaapi-relax-the-version-check-for-VA-API-72.patch
+      ./patches/enable-mojo-video-decoders-by-default-72.patch


The other patches should be applied to versions > 72, not clamped to 73.

My understanding is that 73 wont need those patches. Am I in the wrong ?

I had thought so, and may have said so, but I think @hyperfekt pointed out that the patch to enable-by-default and add the disable-hw-accel flag is still needed. After looking more, I agree with that conclusion and have been having success including it for all builds (dev/beta/stable) on my branch.

AFAIK, and from trying to look for these commits upstream, for 73+, only the "enable-mojo-video-..." patch will be needed.

colemickens · 2019-03-06T19:56:38Z

pkgs/applications/networking/browsers/chromium/common.nix

@@ -261,6 +268,8 @@ let
    } // optionalAttrs pulseSupport {
      use_pulseaudio = true;
      link_pulseaudio = true;
+    } // optionalAttrs (VAAPISupport && (versionRange "72" "73")) {


again, shouldn't version clamp

colemickens · 2019-03-06T19:57:35Z

pkgs/applications/networking/browsers/chromium/default.nix

@@ -92,6 +93,9 @@ in stdenv.mkDerivation {
  buildCommand = let
    browserBinary = "${chromium.browser}/libexec/chromium/chromium";
    getWrapperFlags = plugin: "$(< \"${plugin}/nix-support/wrapper-flags\")";
+    libPath = stdenv.lib.makeLibraryPath (
+      versionAtLeast version "72" && stdenv.lib.optional VAAPISupport libva


Can we have an assert that VAAPISupport is only allowed true when version is > 72 instead? It seems weird to let the user specify VAAPISupport and then ignore it everywhere by && with a version check.

Might be cleaner indeed.

Fix wrapper creation when putting libva in dlopen search path.

amazari · 2019-03-06T22:26:48Z

BTW, I think we might replace the LD_LIBRARY_PATH dance by adding 'autoPatchelfHook' to the build inputs and 'libva' to 'runtimeDependencies'
I will try it out once I have access to a faster building rig. Waiting for 2 hours ain't fun.
Is there any way to keep the build artifacts and enjoy incremental compilation ?

bendlas · 2019-03-07T04:33:27Z

Is there any way to keep the build artifacts and enjoy incremental compilation ?

There has been some thought on that problem .. IIRC @aszlig wants to create ninja2nix, which I think is doable, because ninja is pretty regular and has querying capability.

I'm thinking towards including ccache into the compilation (maybe similar to the existing distcc stdenv).

One way or the other (or even another), it's not quite trivial ...

bendlas · 2019-03-07T20:05:12Z

I tried running this build, and it works, but I couldn't determine vaapi status, because my NVIDIA GPU doesn't support it. Is vdpau support on your radar as well?
Apparently this can be done with something called libva-vdpau-driver-chromium
https://wiki.archlinux.org/index.php/Hardware_video_acceleration

…unds only once Don't patch libva version check relaxation in, as it is a controlled and known quantity in nix. Also, simplify chromium version check by using a unique assertion instead of checking it everytime.

amazari · 2019-03-09T14:07:54Z

@bendlas Thanks for the infos regarding incremental compilation. I'll look into it.
Also, from what I am reading, the nvidia support requires patching libva-vdpau so is not related to the PR. Having no Nvidia GPU at hand, I wont be able to do the job.

@colemickens is the current state of the PR what you have in mind ? Could you check if it works ok with the non-stable channels ? I dont have the cpu power to build them in a reasonable time.

hyperfekt · 2019-03-11T03:45:18Z

pkgs/applications/networking/browsers/chromium/default.nix

@@ -13,8 +13,11 @@
 , cupsSupport ? true
 , pulseSupport ? config.pulseaudio or stdenv.isLinux
 , commandLineArgs ? ""
+, VAAPISupport ? false, libva ? null


I maintain we would want to enable this by default in versions >72, otherwise almost noone would be able to use it given the extremely long compile times.

I have no strong opinion on that. Would create an alias (something like "chromium-vaapi") force the builders to cache a binary ?
Another solution would be to condition it on libva presence in hardware.opengl.extraPackages.

Creating a separate alias would cause Hydra to build Chromium twice - probably not something we want, as the evaluations already take long enough.
AFAIK it's not possible (and shouldn't be) to incorporate NixOS config values in Nix packages.

Creating a separate alias would cause Hydra to build Chromium twice - probably not something we want, as the evaluations already take long enough.

Maybe. Anyway, I guess this decision is the package maintainers to make. @bendlas ?

AFAIK it's not possible (and shouldn't be) to incorporate NixOS config values in Nix packages.
Indeed, I looked at it from a nixos centric pov. You're right.

I'm fine with enabling by default on master. I'd leave it disabled by default on 19.03 (for now).

hyperfekt · 2019-03-11T03:51:28Z

@bendlas AFAIK, the VDPAU-VAAPI-translation is somewhat buggy, which is why it isn't generally included.

hyperfekt · 2019-03-13T09:53:33Z

pkgs/applications/networking/browsers/chromium/common.nix

@@ -143,6 +145,9 @@ let
      # ++ optional (versionRange "68" "72") ( githubPatch "<patch>" "0000000000000000000000000000000000000000000000000000000000000000" )
    ] ++ optionals (!stdenv.cc.isClang && (versionRange "71" "72")) [
      ( githubPatch "65be571f6ac2f7942b4df9e50b24da517f829eec" "1sqv0aba0mpdi4x4f21zdkxz2cf8ji55ffgbfcr88c5gcg0qn2jh" )
+    ] ++ optionals VAAPISupport [
+      ./patches/enable-vaapi-72.patch
+      ./patches/enable-mojo-video-decoders-by-default-72.patch


Chromium 73 was released to stable yesterday, so we should be able to drop the ./patches/enable-mojo-video-decoders-by-default-72.patch needed only for 72 and require version 73 for enabled VAAPI support (default.nix:L81), as I imagine the number of people who need specifically 72 with VAAPI support should be negligible.
Also enable-vaapi-72.patch is not 72-specific, so you might want to remove that version number from the name?
There's also still a bunch of patch files in this PR that aren't actually used.

Merge commit 'refs/pull/57837/head' of github.com:NixOS/nixpkgs fixes NixOS#57837 fixes NixOS#56973

colemickens requested changes Mar 6, 2019

View reviewed changes

chromium: add hw video decoding, fix wrapper

1da0f0b

Fix wrapper creation when putting libva in dlopen search path.

GrahamcOfBorg requested a review from bendlas March 6, 2019 21:20

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Mar 6, 2019

amazari closed this Mar 6, 2019

amazari reopened this Mar 6, 2019

chromium: vaapi support, remove useless patches and assert version bo…

fb74cae

…unds only once Don't patch libva version check relaxation in, as it is a controlled and known quantity in nix. Also, simplify chromium version check by using a unique assertion instead of checking it everytime.

hyperfekt reviewed Mar 11, 2019

View reviewed changes

hyperfekt reviewed Mar 13, 2019

View reviewed changes

bendlas mentioned this pull request Mar 18, 2019

chromium: enable vaapi/hw-accel #57837

Closed

10 tasks

bendlas added a commit to bendlas/nixpkgs that referenced this pull request Mar 21, 2019

chromium: enable vaapi

027b370

Merge commit 'refs/pull/57837/head' of github.com:NixOS/nixpkgs fixes NixOS#57837 fixes NixOS#56973

bendlas closed this in 60e2d2c Mar 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chromium (72): Add hardware video decoding using VAAPI #56973

chromium (72): Add hardware video decoding using VAAPI #56973

amazari commented Mar 6, 2019

amazari commented Mar 6, 2019

colemickens Mar 6, 2019

amazari Mar 6, 2019

colemickens Mar 6, 2019

colemickens Mar 6, 2019 •

edited

amazari Mar 12, 2019

colemickens Mar 6, 2019

amazari Mar 6, 2019

colemickens Mar 6, 2019

amazari Mar 12, 2019

colemickens Mar 6, 2019

amazari Mar 12, 2019

colemickens Mar 6, 2019

amazari Mar 6, 2019

amazari Mar 12, 2019

amazari commented Mar 6, 2019

bendlas commented Mar 7, 2019 •

edited

bendlas commented Mar 7, 2019

amazari commented Mar 9, 2019

hyperfekt Mar 11, 2019 •

edited

amazari Mar 11, 2019

hyperfekt Mar 11, 2019 •

edited

amazari Mar 12, 2019

bendlas Mar 12, 2019

hyperfekt commented Mar 11, 2019

hyperfekt Mar 13, 2019 •

edited

chromium (72): Add hardware video decoding using VAAPI #56973

chromium (72): Add hardware video decoding using VAAPI #56973

Conversation

amazari commented Mar 6, 2019

Motivation for this change

Things done

amazari commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colemickens Mar 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amazari commented Mar 6, 2019

bendlas commented Mar 7, 2019 • edited

bendlas commented Mar 7, 2019

amazari commented Mar 9, 2019

hyperfekt Mar 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperfekt Mar 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperfekt commented Mar 11, 2019

hyperfekt Mar 13, 2019 • edited

Choose a reason for hiding this comment

colemickens Mar 6, 2019 •

edited

bendlas commented Mar 7, 2019 •

edited

hyperfekt Mar 11, 2019 •

edited

hyperfekt Mar 11, 2019 •

edited

hyperfekt Mar 13, 2019 •

edited