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

Enable vaapi support #4039

Merged
merged 3 commits into from
Aug 21, 2019
Merged

Enable vaapi support #4039

merged 3 commits into from
Aug 21, 2019

Conversation

qnixsynapse
Copy link
Contributor

@qnixsynapse qnixsynapse commented Apr 7, 2019

Partially fixes #1024

By default browser will blacklist all drivers on linux for vaapi decoding.

To enable it , need to pass --ignore-gpu-blacklist in order to get it working since libva has problems atm with intel GPUs. I am hoping that it will be fixed in the future versions. There are a lot of activities in upstream in this.
Only adding this support on 64 bit build at the moment.

Based on this upstream commit: chromium/chromium@31225b9

Need to add libva 2.4 as a build and runtime dependency.

Test Plan:

  • Check if build succeeds.
  • Run brave with --ignore-gpu-blacklist flag and see if the video is playing with hardware decoders or not. ( video needs to be supported by hardware)

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions.

Partially fixes #1024 

By default browser will blacklist all drivers on linux for vaapi decoding. 

To enable it , need to pass `--ignore-gpu-blacklist` in order to get it working since libva has problems atm with intel GPUs. I am hoping that it will be fixed in the future versions. There are a lot of activities in upstream in this.
Only adding this support on 64 bit build at the moment.

 Test case:
Run brave with ignore-gpu-blacklist flag and see if the video is playing or not
Based on this upstream [commit](chromium/chromium@31225b9)

Need to add libva 2.4 as a build and runtime dependency.
@maximbaz
Copy link

maximbaz commented Jun 21, 2019

Hey, I'm maintainer of chromium-vaapi for Arch Linux and I'm eager to try your PR 🙂 Unfortunately I can't build Brave right now due to #305, but I'm curious, don't you also need to actually have this patch applied to make it work on non-Chrome-OS hosts?

UPDATE: I think I understand it now, instead of having to patch sources all that needs to be done is to pass --ignore-gpu-blacklist flag - neat!

@qnixsynapse
Copy link
Contributor Author

qnixsynapse commented Jun 21, 2019

@maximbaz Hello! This is my patch from Fedora but no because few intel devices are having problem with vaapi. I opened this PR but not getting enough time to work on it. Also there is #305. It would be helpful if you try it for me. (maybe opening a github repo that I can see and contribute?) :)

An arch linux PKGBUILD will be enough.

@maximbaz
Copy link

maximbaz commented Jun 21, 2019

I have eventually managed to compile Brave binary from your PR in a very weird way, but I can confirm that Brave that is based on Chromium 74 + your patch + --ignore-gpu-blacklist does indeed use HW acceleration on my Intel machine! Video is accelerated, CPU fans are silent, all is perfect 🙂

Chromium 75 has introduced a regression that broke VA-API for both VP9 and H264 on my Intel machine.

I would really really love to see this PR merged and #305 fixed, so I can add Brave to the official repositories of Arch Linux. Hopefully Chromium/Intel fixes their regression as well.

@qnixsynapse
Copy link
Contributor Author

@maximbaz Thank you so much for testing!!!!

@maximbaz
Copy link

maximbaz commented Jul 3, 2019

@akarshanbiswas so the patch you shared on Arch forums helped fixing VA-API on Nvidia and Intel in the latest Chromium (once again thanks for that!), how do we go and include that patch in Brave, do you know? I'm not very familiar with the codebase yet, but it seems we would need to patch brave-core?

@qnixsynapse
Copy link
Contributor Author

@maximbaz I will try to contact upstream instead. Meanwhile we should move this conversation on the intel vaapi driver issue that you've mentioned to let other know of this.

@maximbaz
Copy link

maximbaz commented Jul 3, 2019

While contacting upstream is definitely a correct course of action, as a packager I'm considering what it would take to maintain Brave with VA-API support, comparing to Chromium with VA-API. Suppose #305 is fixed, my build steps are just the 3 commands below, and I have a chromium patch to apply - how difficult would that be to do, do you know?

npm install
npm run init
npm run create_dist Release --debug_build=false --official_build=true

@qnixsynapse
Copy link
Contributor Author

@maximbaz Please create a repo and add me as a maintainer. Let me see what I can do. For now please keep this thread clean.

Brave uses npm as a build system. I'm not very good at it. The patch which I shared need to be added to brave core if I am not wrong. I need to study it first.

@cg505
Copy link

cg505 commented Aug 3, 2019

@akarshanbiswas @maximbaz Will this PR work without the chromium vaapi patch at https://aur.archlinux.org/cgit/aur.git/tree/chromium-vaapi-fix.patch?h=brave?

@maximbaz
Copy link

maximbaz commented Aug 3, 2019

With this PR the browser will compile successfully, and if started with --ignore-gpu-blacklist hardware video acceleration will work on a certain subset of graphics cards (namely AMD and some Intel), but that patch is required to support the rest of Intel cards and all of Nvidia.

there is a benefit of merging this PR even without that chromium patch, if that's what you are asking 😉

remember that if users don't provide --ignore-gpu-blacklist, the browser on all devices will behave exactly as it currently does in release version, it will not suddenly break video playback for Nvidia users.

@qnixsynapse
Copy link
Contributor Author

@cg505 Yes. The normal behavior will be the same as the current release build of brave.
The patch you mentioned fixes certain regressions on specific intel and nvidia GPUs.

Nvidia GPUs doesn't support VPP, and some Intel drivers has issues with specially few skylake and others. This is the same reason I am not enabling(removing the blacklist) it by default.

The upstream is pretty active in fixing build errors with vaapi on Linux.

@cg505
Copy link

cg505 commented Aug 20, 2019

Awesome, I was able to verify that this works for me.

@bsclifton
Copy link
Member

@akarshanbiswas can you give this PR a rebase? This should kick off our continuous integration for Linux, which compiles + runs tests

@qnixsynapse
Copy link
Contributor Author

@bsclifton Sure 👍

@qnixsynapse
Copy link
Contributor Author

@bsclifton Rebased to latest changes from master branch. It needs to get reviewed.

@bsclifton bsclifton self-requested a review August 21, 2019 21:22
@bsclifton
Copy link
Member

ugh forgot CI doesn't run on forks 😓

Going to approve because @cg505 did test this out and it worked for him

@bsclifton bsclifton merged commit 5fe2cb7 into brave:master Aug 21, 2019
@qnixsynapse qnixsynapse deleted the patch-1 branch August 22, 2019 07:01
@fmarier fmarier mentioned this pull request Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable video decoding with vaapi by default
5 participants