Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792743002:
Use Stub CDM for building libwidevinecdmadapter.so (Closed)
DescriptionUse Stub CDM for building libwidevinecdmadapter.so
- This is a temporary fix for unblocking LLD.
- We use the stub CDM to build the CDM adapter.
- We ship the copied (real) CDM and the CDM adapter.
- Even though the CDM adapter is built against the stub cdm at
"stub_cdm/libwidevinecdm.so", only the filename is baked
into the adapter because of the gn dependency,
e.g., readelf -d libwidevinecdmadapter.so :
0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so]
- At run time, when loading the CDM adapter, the system will search
for the CDM in the same dir where the adapter is located. Hence
the real CDM will be loaded.
BUG=707488
Review-Url: https://codereview.chromium.org/2792743002
Cr-Commit-Position: refs/heads/master@{#462104}
Committed: https://chromium.googlesource.com/chromium/src/+/4bd7678fefa459f3f29a4ea511ff26971cfb5648
Patch Set 1 #
Total comments: 1
Patch Set 2 : Use Stub CDM for building widevinecdmadapter. #
Total comments: 4
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Fix chromeos. #Messages
Total messages: 28 (14 generated)
hmchen@chromium.org changed reviewers: + hmchen@chromium.org, xhwang@chromium.org
Prototype CL to use stub CDM in chrome build. PTAL.
For the context, there are chromium-based browsers (is_chrome_branded=false) using enable_widevine=true to build the stub CDM. We should not break that use case. One thing you can try is to limit your change for the following condition: is_chrome_branded=true && enable_widevine=true && (is_linux || is_chromeos) https://codereview.chromium.org/2792743002/diff/1/third_party/widevine/cdm/BU... File third_party/widevine/cdm/BUILD.gn (right): https://codereview.chromium.org/2792743002/diff/1/third_party/widevine/cdm/BU... third_party/widevine/cdm/BUILD.gn:27: # libwidevinecdm.so issue is fixed. Please file a bug and add bug number here for tracking.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Use Stub CDM for building libwidevinecdmadapter.so This avoids a libwidevinecdm issue in chrome build. BUG= ========== to ========== Use Stub CDM for building libwidevinecdmadapter.so This avoids a libwidevinecdm issue in chrome build. BUG=707488 ==========
xhwang: PTAL tinskip, pcc, krasin, rkuroiwa: FYI
pcc1 / krasin1: Does this fix the linker issue? hmchen: thanks for working out a workaround so quickly. I just have one comment. https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... File third_party/widevine/cdm/BUILD.gn (right): https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... third_party/widevine/cdm/BUILD.gn:28: enable_widevine = true This will enable VerifyCdmHost on linux and chromeos, which is not desired for now: https://cs.chromium.org/chromium/src/ppapi/features/features.gni?rcl=976a6d56... It seems to me we don't really need "enable_widevine" now. All the changes are confined with this file, and we check use_stub_cdm_for_chrome for everything. Can you simply drop it?
Please also provide more details about what we are doing in the CL description. For example: 1. we use the stub CDM to build the CDM adapter 2.We ship the copied (real) CDM and the CDM adapter. 3. Even though the CDM adapter is built against the stub cdm at stub_cdm/libwidevinecdm.so, only the filename is baked into the adapter, e.g.: 0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so] 4. At run time, when loading the CDM adapter, the system will search for the CDM in the same dir where the adapter is located. Hence the real CDM will be loaded. https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... File third_party/widevine/cdm/BUILD.gn (right): https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... third_party/widevine/cdm/BUILD.gn:216: root_build_dir) ] Actually, this is not necessary since we are depending on :widevinecdm which is a shared library. We only need this ldflag when we are copying the cdm.
Description was changed from ========== Use Stub CDM for building libwidevinecdmadapter.so This avoids a libwidevinecdm issue in chrome build. BUG=707488 ========== to ========== Use Stub CDM for building libwidevinecdmadapter.so - This is a temporary fix for unblocking LLD. - We use the stub CDM to build the CDM adapter. - We ship the copied (real) CDM and the CDM adapter. - Even though the CDM adapter is built against the stub cdm at "stub_cdm/libwidevinecdm.so", only the filename is baked into the adapter, e.g., readelf -d libwidevinecdmadapter.so : 0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so] - At run time, when loading the CDM adapter, the system will search for the CDM in the same dir where the adapter is located. Hence the real CDM will be loaded. BUG=707488 ==========
Description was changed from ========== Use Stub CDM for building libwidevinecdmadapter.so - This is a temporary fix for unblocking LLD. - We use the stub CDM to build the CDM adapter. - We ship the copied (real) CDM and the CDM adapter. - Even though the CDM adapter is built against the stub cdm at "stub_cdm/libwidevinecdm.so", only the filename is baked into the adapter, e.g., readelf -d libwidevinecdmadapter.so : 0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so] - At run time, when loading the CDM adapter, the system will search for the CDM in the same dir where the adapter is located. Hence the real CDM will be loaded. BUG=707488 ========== to ========== Use Stub CDM for building libwidevinecdmadapter.so - This is a temporary fix for unblocking LLD. - We use the stub CDM to build the CDM adapter. - We ship the copied (real) CDM and the CDM adapter. - Even though the CDM adapter is built against the stub cdm at "stub_cdm/libwidevinecdm.so", only the filename is baked into the adapter because of the gn dependency, e.g., readelf -d libwidevinecdmadapter.so : 0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so] - At run time, when loading the CDM adapter, the system will search for the CDM in the same dir where the adapter is located. Hence the real CDM will be loaded. BUG=707488 ==========
xhwang: thanks for you review. PTAL. https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... File third_party/widevine/cdm/BUILD.gn (right): https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... third_party/widevine/cdm/BUILD.gn:28: enable_widevine = true On 2017/04/04 06:48:52, xhwang_slow wrote: > This will enable VerifyCdmHost on linux and chromeos, which is not desired for > now: > > https://cs.chromium.org/chromium/src/ppapi/features/features.gni?rcl=976a6d56... > > It seems to me we don't really need "enable_widevine" now. All the changes are > confined with this file, and we check use_stub_cdm_for_chrome for everything. > Can you simply drop it? Done. https://codereview.chromium.org/2792743002/diff/60001/third_party/widevine/cd... third_party/widevine/cdm/BUILD.gn:216: root_build_dir) ] On 2017/04/04 07:00:10, xhwang_slow wrote: > Actually, this is not necessary since we are depending on :widevinecdm which is > a shared library. We only need this ldflag when we are copying the cdm. Done.
lgtm! Please double check with pcc1 and krasin1 to make sure this solves the problem before landing! Also +jrummell FYI. We are using the stub CDM as a workaround of some linker issue.
https://codereview.chromium.org/2792743002/diff/80001/third_party/widevine/cd... File third_party/widevine/cdm/BUILD.gn (right): https://codereview.chromium.org/2792743002/diff/80001/third_party/widevine/cd... third_party/widevine/cdm/BUILD.gn:27: if (is_chrome_branded && (is_linux || is_chromeos)) { is_linux includes is_chromeos, so "is_chromeos" is not necessary https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?rcl=85cae582...
PTAL. pcc1 / krasin1: can you check if this change solves the LLD issue? https://codereview.chromium.org/2792743002/diff/80001/third_party/widevine/cd... File third_party/widevine/cdm/BUILD.gn (right): https://codereview.chromium.org/2792743002/diff/80001/third_party/widevine/cd... third_party/widevine/cdm/BUILD.gn:27: if (is_chrome_branded && (is_linux || is_chromeos)) { On 2017/04/04 17:45:10, xhwang_slow wrote: > is_linux includes is_chromeos, so "is_chromeos" is not necessary > > https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?rcl=85cae582... Done. Also fixed line 100.
The CQ bit was checked by hmchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/04 18:11:45, Haoming Chen wrote: > pcc1 / krasin1: can you check if this change solves the LLD issue? Before this change: $ ninja libwidevinecdmadapter.so [127/127] SOLINK_MODULE ./libwidevinecdmadapter.so FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ [...] [...]/third_party/llvm-build/Release+Asserts/bin/ld.lld: error: invalid alignment of section headers clang: error: linker command failed with exit code 1 (use -v to see invocation) ninja: build stopped: subcommand failed. After patching in this change: $ ninja libwidevinecdmadapter.so [1/1] Regenerating ninja files [386/386] SOLINK_MODULE ./libwidevinecdmadapter.so So this change does appear to have fixed the LLD issue.
On 2017/04/04 18:41:29, pcc1 wrote: > On 2017/04/04 18:11:45, Haoming Chen wrote: > > pcc1 / krasin1: can you check if this change solves the LLD issue? > > Before this change: > > $ ninja libwidevinecdmadapter.so > [127/127] SOLINK_MODULE ./libwidevinecdmadapter.so > FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang++ [...] > [...]/third_party/llvm-build/Release+Asserts/bin/ld.lld: error: invalid > alignment of section headers > clang: error: linker command failed with exit code 1 (use -v to see invocation) > ninja: build stopped: subcommand failed. > > After patching in this change: > > $ ninja libwidevinecdmadapter.so > [1/1] Regenerating ninja files > [386/386] SOLINK_MODULE ./libwidevinecdmadapter.so > > So this change does appear to have fixed the LLD issue. Great to hear! Hopefully we can fix the root issue soon!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hmchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org Link to the patchset: https://codereview.chromium.org/2792743002/#ps100001 (title: "Fix chromeos.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491409272748650,
"parent_rev": "c18a893645532202d554f5c9ea20b68487372397", "commit_rev":
"4bd7678fefa459f3f29a4ea511ff26971cfb5648"}
Message was sent while issue was closed.
Description was changed from ========== Use Stub CDM for building libwidevinecdmadapter.so - This is a temporary fix for unblocking LLD. - We use the stub CDM to build the CDM adapter. - We ship the copied (real) CDM and the CDM adapter. - Even though the CDM adapter is built against the stub cdm at "stub_cdm/libwidevinecdm.so", only the filename is baked into the adapter because of the gn dependency, e.g., readelf -d libwidevinecdmadapter.so : 0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so] - At run time, when loading the CDM adapter, the system will search for the CDM in the same dir where the adapter is located. Hence the real CDM will be loaded. BUG=707488 ========== to ========== Use Stub CDM for building libwidevinecdmadapter.so - This is a temporary fix for unblocking LLD. - We use the stub CDM to build the CDM adapter. - We ship the copied (real) CDM and the CDM adapter. - Even though the CDM adapter is built against the stub cdm at "stub_cdm/libwidevinecdm.so", only the filename is baked into the adapter because of the gn dependency, e.g., readelf -d libwidevinecdmadapter.so : 0x0000000000000001 (NEEDED) Shared library: [libwidevinecdm.so] - At run time, when loading the CDM adapter, the system will search for the CDM in the same dir where the adapter is located. Hence the real CDM will be loaded. BUG=707488 Review-Url: https://codereview.chromium.org/2792743002 Cr-Commit-Position: refs/heads/master@{#462104} Committed: https://chromium.googlesource.com/chromium/src/+/4bd7678fefa459f3f29a4ea511ff... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/4bd7678fefa459f3f29a4ea511ff...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/2893853003/ by hmchen@chromium.org. The reason for reverting is: Using stub cdm is a workaround and it breaks the load_library_perf_test. This change reverts the stub cdm because the unaligned ELF header issue has been fixed for libwidevinecdm.so, so that we can re-enable the load_library_perf_test.. |
