Support widevine in linux by bundling by simonhong · Pull Request #1475 · brave/brave-core

simonhong · 2019-01-28T06:12:51Z

In linux, WidevineCdm library is bundled instead of installing it
by component updating during the runtime. In linux, we can't load
cdm library during the runtime because processes except browser
process are forked from zygote process not created from brave.exe.
So, cdm should be loaded(not initialized) into zygote process space
before zygote enters the sandbox.

Chrome browser also uses bundling for linux.

This PR should be merged with brave/brave-browser#3037

Fix brave/brave-browser#413

Submitter Checklist:

Test Plan:

Start browser with clean profile
Load https://bitmovin.com/demos/drm
Check No drm
Install widevine via content settings bubble
Check widevine is enabled
Restart browser and check widevine is enabled

yarn test brave_unittest --filter=CdmRegistryImplTest.WidevineCdmExcludeTest

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

simonhong · 2019-01-28T10:49:23Z

When widevine cdm is bundled, widevine cdm library is loaded into zygote process and
it is registered to CdmRegistry at startup.
Because of this, widevine is initialized regardless of the value of kWidevineOptedIn.
So, need to find how prevent initializing widevine when user doesn't accept explicitly.

To handle this, I tried to get kWidevineOptedIn from CdmRegistry via ContentBrowserClient() when renderer asks whether widevine is enabled or not.
However, there are some difficulties - it is ambiguous which profile should be used and profile manager is only allowed to access by main thread.

If we also want to show content settings bubble in linux, I need to find how to prevent cdm initialization. Or if we can disable content bubble in linux, only first & second commits in this PR are needed.
@bbondy @iefremov Any advice would be welcome!

iefremov · 2019-01-28T11:46:50Z

@simonhong
Maybe we can ignore the fact that different profiles may have different values of kWidevineOptedIn during Widevine initialization? It's hard to skip loading the binary if the user has one profile that opted int and another that didn't.

This way, if any of user profiles has this preference enabled, we can somehow persist the fact that the binary should be loaded (e.g. create a marker file) and, for example, check it in ChromeContentClient::AddContentDecryptionModules.

simonhong · 2019-01-28T13:32:11Z

With bundling, cdm loading and initialization is in separated phase.
Loading is done during the startup by PreloadLibraryCdms().
And initialization is done by CdmModule::Initialize() when cdm service is launched.
This launching is triggered by renderer that tries to play contents.

@iefremov As you said, we can't avoid loading. But, I thought we can control cdm service launching.
For having same UX with mac/win, my initial idea was modifying CdmRegistryImpl::GetAllRegisteredCdms().
In this method, I tried to exclude widevine CdmInfo when kWidevineOptedIn is not set.
With this, I think we could make renderer think widevine is not available for now.

However, I met multi profile issue and accessing profile_manager from io thread.
If we must have same UX with other platforms, we should find the way and I think it's not impossible but would introduce many patches.

So, I also want to know we should have same UX. If not, it can be achieved with simple patch. Only first & second commits are needed as I said earlier.

IMO, current content setting bubble is not suitable because cdm library is already installed and loaded into process. It is just not initialized.

simonhong · 2019-01-29T06:06:56Z

With the 4th commit, widevine is only initialized after users allowed to use widevine via content settings bubble.
One remained issue is widevine isn't initialized by reloading. To use widevine after user accept, contents page should be loaded in the new tab or restart. I'm trying to find the reason now...

simonhong · 2019-01-29T09:22:17Z

I think this PR is ready to review.
With this PR, linux has same widevine UX with mac/win.

simonhong · 2019-01-30T03:09:29Z

@bbondy Due to the restriction of zygote in linux, I enabled widevine by bundling.
That means, widevine is loaded into zygote process by default but not initialized.
It is only initialized and cmd service is launched when user accepts via content settings bubble.
If we don't load it by default, we would need restart to use widevine after user accepts.
If loading by default is not allowed by our privacy policy, please let me know.

iefremov · 2019-01-30T10:56:21Z

browser/brave_browser_main_extra_parts.cc

 void BraveBrowserMainExtraParts::PreMainMessageLoopRun() {
  brave::AutoImportMuon();
+#if BUILDFLAG(BUNDLE_WIDEVINE_CDM)
+  RegisterWidevineCdmToCdmRegistry();


rename -> MaybeRegister... ?

And also skip ToCdmRegistry, sounds obvious :)

Thanks. it's more clear 👍

iefremov · 2019-01-30T11:14:16Z

browser/ui/content_settings/brave_content_setting_image_models.cc

 #include "third_party/widevine/cdm/buildflags.h"

-#if BUILDFLAG(ENABLE_WIDEVINE_CDM_COMPONENT)
+// On Windows and MacOS, widevine is supported by component updater.


Not sure if the comment is relevant here, maybe move it to the widevine.gni patch?

Yep, widevine.gni is better place for this comment.
Done.

iefremov · 2019-01-30T12:03:22Z

patches/content-browser-media-cdm_registry_impl.cc.patch

+ void CdmRegistryImpl::Init() {
+  // In linux, we want to register widevine cdm to CdmRegistry when users opted
+  // in. If not, widevine is initialized by default.
+  // Instead, we tries to register it in


I'm not a native speaker, but probably it should be "on linux", "when users opt in", "if not" -> otherwise, "we tries" -> "we try"

iefremov · 2019-01-30T12:06:07Z

patches/content-browser-media-cdm_registry_impl.cc.patch

+  // Instead, we tries to register it in
+  // BraveBrowserMainExtraParts::PreMainMessageLoopRun() by checking opted in
+  // prefs.
+#if defined(BRAVE_CHROMIUM_BUILD) && !defined(OS_LINUX)


~~ENABLE_WIDEVINE_CDM_COMPONENT instead of !OS_LINUX ?~~
nevermind

I'm wondering whether we potentially block CDMs other than Widevine on Linux?

good catch.
Actually, widevine alone is used for now but don't know in the future whether chrome registers more cdms.
So, fixed to exclude widevine only after getting cdms list.

iefremov · 2019-01-30T12:24:06Z

Generally LGTM with small nits

simonhong · 2019-01-31T04:18:16Z

Created f/u issue - brave/brave-browser#3172

iefremov · 2019-01-31T05:46:13Z

browser/ui/content_settings/brave_content_setting_image_models.cc

Was this change announced somewhere? A think, if it was then we should fix all sources at once, otherwise its better to fix the linter. @bbondy could you please clarify?

@iefremov it is enforced by the linter. There was a discussion in #brave-core

@bridiver maybe we can fix the whole codebase by a single PR then? I can do one

iefremov · 2019-01-31T05:50:37Z

patches/third_party-widevine-cdm-widevine.gni.patch

Mind placing a follow-up issue number here?

has this been vetted by @bbondy? We're specifically prohibited from bundling on other platforms

Yes, I discussed in dm and he agreed to do it as a next step. (long term)

iefremov · 2019-01-31T05:53:21Z

chromium_src/content/browser/media/cdm_registry_impl_unittest.cc

in gtest macroses expected argument is the first, i.e. EXPECT_EQ(0u, cdms.size());

iefremov · 2019-01-31T05:55:03Z

chromium_src/content/browser/media/cdm_registry_impl_unittest.cc

This erasure happens only on Linux

simonhong

All done. PTAL.
Also this one - brave/brave-browser#3037

simonhong · 2019-01-31T06:25:50Z

chromium_src/content/browser/media/cdm_registry_impl_unittest.cc

simonhong · 2019-01-31T06:25:54Z

chromium_src/content/browser/media/cdm_registry_impl_unittest.cc

simonhong · 2019-01-31T06:26:00Z

patches/third_party-widevine-cdm-widevine.gni.patch

bridiver · 2019-01-31T22:32:04Z

patches/content-browser-media-cdm_registry_impl.cc.patch

please do not add comments to patches. We want to minimize the size of patches

this patch file was deleted by overriding.

bridiver · 2019-01-31T22:33:31Z

patches/chrome-renderer-media-chrome_key_systems_provider.cc.patch

I don't understand why we're patching this?

W/o this patch in this file, reloading doesn't enable widevine after user accepts.

bridiver · 2019-01-31T22:35:58Z

patches/content-browser-media-cdm_registry_impl.cc.patch

I don't understand why we're doing this here, why wouldn't we do it in AddContentDecryptionModules where we don't have to patch anything?

It seems my reply was deleted after force push.
There are some places where original cmd list is needed like PreloadLibraryCdms().
So, we can't override ChromeContentClient::AddContentDecryptionModules().
Also, this patch is removed by file overriding.

bridiver · 2019-01-31T22:37:35Z

browser/ui/content_settings/brave_widevine_bundle_util.cc

this seems very fragile to me. Where did this list of codecs come from?

Yes, I think so. I copied this logic from IsWidevineAvailable() in chrome_content_client.cc.

Ah,, I think this can be improved by storing excluded cdminfo in CdmRegistry in at some place and can be reused again.

All this log is deleted and used cached info from CdmRegistryImpl.

All duplicated code is deleted by caching CdmInfo that created by upstream.
81ca1a1

bridiver · 2019-01-31T22:52:44Z

chromium_src/content/browser/media/cdm_registry_impl.cc

I think you're missing what I was trying to say here. It calls GetContentClient()->AddContentDecryptionModules and we can implement AddContentDecryptionModules in BraveContentClient

I thought that first.
But, other places needs original list like PreloadLibraryCdms() in content_main_runner_impl.cc.

I don't understand, the result would be identical because it passes a reference that can be modified

ChromeContentClient::AddContentDecryptionModules() fills cdms to passed pointer.
Caller of above method will have different instance.
Of course, client of CdmRegistry will see same list.

Oh, many comments were deleted..
Caller of AddContentDecryptionMoudles maintains it's own cmd list.
Clients of CdmRegistry see same list because CdmRegistry::GetAllRegisteredCdms() returns list reference.

bridiver · 2019-01-31T23:13:40Z

browser/ui/content_settings/brave_content_setting_image_models.cc

* is always preferable to & because it's more clear to the caller that you're passing something which might be modified when the method returns. If you're looking at a caller using ref, it's indistinguishable from passing by const ref or value unless you look at the method sig

Yes, totally agree about that.

basically, this rule is fixed in CC, so we don't even need to have comments like this in the codebase :)

Right. using like this is natural in chromium project :) Removed.

In linux, WidevineCdm library is bundled instead of installing it by component updating during the runtime. In linux, we can't load cdm library during the runtime because processes except browser process are forked from zygote process not created from brave.exe. So, cdm should be loaded(not initialized) into zygote process space before zygote enters the sandbox. Chrome browser also uses bundling for linux.

In linux, widevine should be enabled only when user accpeted.

With this, widevine cdm is initialized when users allowed to use widevine cdm via content settings bubble.

In linux, bundled widevine cdm is loaded by default but not initialized by default. It is only initialized when user allows it. Then, widevine cdm is added to CdmRegistry. That means renderer should also update supported KeySystems during the runtime.

We only want to exclude widevine. So, exclude it explicitly from list.

This test checks CdmRegistryImpl erases widevine from cdm list after fetching cdm list from content client. This erase only happened on linux.

Instead, use cached CdmInfo from CdmRegistryImpl.

simonhong · 2019-02-07T05:17:38Z

New PR (#1606) is opened for this.

simonhong added the pr/work-in-progress label Jan 28, 2019

simonhong self-assigned this Jan 28, 2019

simonhong mentioned this pull request Jan 28, 2019

Enable widevine on linux brave/brave-browser#3037

Closed

16 tasks

simonhong force-pushed the widevine_support_in_linux branch from c54c5f2 to 67b1a9f Compare January 29, 2019 08:13

simonhong removed the pr/work-in-progress label Jan 29, 2019

simonhong requested review from bbondy and iefremov January 29, 2019 09:20

simonhong changed the title ~~WIP: Support widevine in linux by bundling~~ Support widevine in linux by bundling Jan 29, 2019

simonhong added this to the 0.62.x - Nightly milestone Jan 29, 2019

iefremov reviewed Jan 30, 2019

View reviewed changes

simonhong force-pushed the widevine_support_in_linux branch from e7b50fe to 481dc08 Compare January 31, 2019 05:44

iefremov reviewed Jan 31, 2019

View reviewed changes

iefremov previously approved these changes Jan 31, 2019

View reviewed changes

simonhong dismissed iefremov’s stale review via c164607 January 31, 2019 06:25

simonhong force-pushed the widevine_support_in_linux branch from 481dc08 to c164607 Compare January 31, 2019 06:25

simonhong commented Jan 31, 2019

View reviewed changes

iefremov previously approved these changes Jan 31, 2019

View reviewed changes

bridiver reviewed Jan 31, 2019

View reviewed changes

simonhong dismissed iefremov’s stale review via 03e0940 January 31, 2019 22:49

bridiver reviewed Jan 31, 2019

View reviewed changes

simonhong force-pushed the widevine_support_in_linux branch 3 times, most recently from 0b8a725 to 15bcb7c Compare February 1, 2019 02:43

simonhong added 10 commits February 1, 2019 12:59

Add libwidevinecdm.lib into dist_resources when bundling is enabled

06c6b61

Enable widevine content settings bubble in linux

cd87646

In linux, widevine should be enabled only when user accpeted.

Only registers widevine cdm to CdmRegistry when users opted in

932f4aa

With this, widevine cdm is initialized when users allowed to use widevine cdm via content settings bubble.

Preserve other cdms except widevine when CdmRegistryImpl::Init()

87d1b6f

We only want to exclude widevine. So, exclude it explicitly from list.

Fix lint error and typo

72afe60

Add CdmRegistryImplTest.WidevineCdmExcludeTest

f93c3aa

This test checks CdmRegistryImpl erases widevine from cdm list after fetching cdm list from content client. This erase only happened on linux.

Use override technique for cdm_registry_impl.cc

9a814a0

Delete copied logic for creating widevine CdmInfo

81ca1a1

Instead, use cached CdmInfo from CdmRegistryImpl.

simonhong force-pushed the widevine_support_in_linux branch from 15bcb7c to 81ca1a1 Compare February 1, 2019 05:30

Remove useless comment

064a84b

simonhong closed this Feb 7, 2019

bsclifton deleted the widevine_support_in_linux branch January 15, 2020 17:22

Conversation

simonhong commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist:

Test Plan:

Reviewer Checklist:

Uh oh!

simonhong commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iefremov commented Jan 28, 2019

Uh oh!

simonhong commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonhong commented Jan 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonhong commented Jan 29, 2019

Uh oh!

simonhong commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iefremov Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iefremov commented Jan 30, 2019

Uh oh!

simonhong commented Jan 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonhong Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonhong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonhong commented Jan 28, 2019 •

edited

Loading

simonhong commented Jan 28, 2019 •

edited

Loading

simonhong commented Jan 28, 2019 •

edited

Loading

simonhong commented Jan 29, 2019 •

edited

Loading

simonhong commented Jan 30, 2019 •

edited

Loading

iefremov Jan 30, 2019 •

edited

Loading

simonhong Jan 31, 2019 •

edited

Loading

simonhong Jan 31, 2019 •

edited

Loading