-
Notifications
You must be signed in to change notification settings - Fork 889
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
Support Widevine in Arm64 Brave on Windows #18695
Conversation
944d814
to
109c1c3
Compare
edf6e1d
to
e316186
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not do a thorough line-by-line review, but the only security-relevant change I asked for after looking through the code has been fixed.
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unlocking browser/net/*, patches and chromium_src need some more work as discussed
960a129
to
497793c
Compare
For my own reference: There's now version |
64e41bc
to
3189bb7
Compare
Thank you for your patch @mherrmann. We @ Linaro are actively watching the progress on this support and this patch. |
The implementation is described in the new README.md file.
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.h
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.cc
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.h
Outdated
Show resolved
Hide resolved
chromium_src/chrome/browser/component_updater/widevine_cdm_component_installer.h
Show resolved
Hide resolved
Thank you all for the progress. Is there a binary I can try? I tried building, but ran into compilation issues after many hours of build on my Windows on Arm box.. Haven't gotten around to fixing that, but an installer would be great. |
@ilina-linaro once this PR is merged, you should be able to get binaries from Brave's next Nightly release on the GitHub releases page. I normally use BraveBrowserStandaloneNightlySetupArm64.exe. |
return true; | ||
} | ||
|
||
void RegisterWidevineCdmComponent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we add a second copy of this method instead of using buildflags around the params? It's very confusing, especially because the two methods are not even together in the file. This one also has a VLOG that the other one doesn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The motivation was to split this source file into two clean halves: One with BUILDFLAG(WIDEVINE_ARM64_DLL_FIX)
and the other one without. It felt easier / clearer when implementing this PR. But I can see now that for example when searching for the definition of the function, it is less clear to see which definition will get used. Do you want me to file a follow-up PR that refactors this?
Resolves brave/brave-browser#28318.
Notes to reviewers
The design of the implementation is described in the newly-added file
brave/browser/widevine/README.md
, under section Widevine in Arm64 Brave on Windows: WIDEVINE_ARM64_DLL_FIX. The easiest way to review the code is to read the explanations there, reviewing the mentioned files as you go along.Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Please test that Arm64 Brave on (Arm64) Windows can successfully install the Widevine component and play DRM-protected content at the highest quality.
To check for regressions, please test that Widevine can still be installed and DRM-protected content can still be streamed at the highest quality on Windows and macOS. See for example brave/brave-browser#29469 (comment) for some good tests for checking the video quality.
All tests should be performed with a clean profile. On Windows for example, this means deleting (or temporarily renaming) the
%LOCALAPPDATA%\BraveSoftware\Brave-Browser-<channel>\User Data
directory. The reason why the profile should be clean is that an existing profile might have already installed the Widevine module.