Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1043673002:
Allow widevinecdmadapter to be built in Chromium (Closed)
|
Created:
5 years, 9 months ago by jrummell Modified:
5 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, eme-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow widevinecdmadapter to be built in Chromium
Adds a stub CDM that is used to link widevinecdmadapter when enable_widevine=1.
BUG=468567
TEST=Widevine EME browser tests run in Chromium with enable_widevine=1
Committed: https://crrev.com/534d59853e0803ee61e6c8a6a4ceacac44df6843
Cr-Commit-Position: refs/heads/master@{#323862}
Patch Set 1 #
Total comments: 39
Patch Set 2 : changes #
Total comments: 31
Patch Set 3 : Changes #
Total comments: 8
Patch Set 4 : build stub as widevinecdm #
Total comments: 6
Patch Set 5 : nits #
Messages
Total messages: 22 (4 generated)
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
https://codereview.chromium.org/1043673002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1043673002/diff/1/build/common.gypi#newcode569 build/common.gypi:569: 'enable_widevine%': 0, This is what Dan tried to do. I don't think it will be allowed. https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/encryp... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/encryp... chrome/browser/media/encrypted_media_browsertest.cc:186: if (expected_title == kEnded) { You shouldn't need this. Just make your stub return a dummy message. https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/wv_tes... File chrome/browser/media/wv_test_license_server_config.cc (right): https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/wv_tes... chrome/browser/media/wv_test_license_server_config.cc:124: #if defined(OS_LINUX) && defined(ARCH_CPU_X86_64) && defined(OFFICIAL_BUILD) This should not be committed. You can add a TODO to base this on branding=Chrome somehow. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... File media/cdm/ppapi/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:58: FailRequest(promise_id); Succeed if the parameters are valid. Also, call host_->SendMessage(). https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/widevi... File media/cdm/ppapi/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/widevi... media/cdm/ppapi/stub/widevine_cdm_version.h:14: #define WIDEVINE_CDM_VERSION_STRING "1.4.7.796" We don't need a version. This file should look like the Android one. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... File third_party/widevine/cdm/copy_with_rename.py (right): https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... third_party/widevine/cdm/copy_with_rename.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. 2015 unless you copied this file. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:47: [ 'branding != "Chrome" and OS != "android" and enable_widevine==1', { It would be nice if we could keep the paths more similar. For example, ideally we could share the block above somehow then override certain values. One idea would be to set the stub name in the block above (even if it's not used) then have the copy step copy the files to the normal output directory (lib/ ?)with the new (widevine) names. Then we'd just run the copies step normally. You'd still need to specify that the widevine_cdm_binary_files are in lib/ instead of third_party, so you would still need this block. I guess the only benefit would be that you'd run lines 212-219 in both cases (while introducing an extra copy). https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:52: '../../../media/cdm/ppapi/stub/widevine_cdm_version.h', this should be in third_party like the Android file. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:161: '<(DEPTH)/media/media.gyp:stubcdm', It appears that all except this line are common. Can we share the block above and only add this? https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:212: [ 'branding == "Chrome" or OS == "android"', { Android doesn't copy anything. Android never runs this rule anyway. See line 124 and 129. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:220: [ 'branding != "Chrome" and OS != "android" and enable_widevine==1', { ditto
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
I didn't review the gyp changes. I'll take another look after ddorwin's comments are resolved. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... File media/cdm/ppapi/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:9: const char kStubCdmVersion[] = "0.1.0.1"; Let's have some versioning scheme here. How about 1.4.8.0? When the CDM interface version increments, we'll update the 3rd number. Other numbers will not likely to change. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:137: NOTIMPLEMENTED(); NOTREACHED(); https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:144: NOTIMPLEMENTED(); NOTREACHED(); https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:148: std::string message("Stub CDM."); nit: "Operation not supported by stub CDM"? https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... File media/cdm/ppapi/stub/stub_cdm.h (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:6: #define MEDIA_CDM_PPAPI_STUB_STUB_CDM_H_ ddorwin/jrummell: Actually "external_clear_key" and "stub_cdm" have nothing to do with ppapi. The "ppapi" part is handled by cdm adapter etc. How about we put "external_clear_key" and "stub_cdm" under media/cdm, instead of media/cdm/ppapi? Imagine in the future we could have a mojo service loading and using the CDM, which has nothing to do with ppapi. The same for media/cdm/ppapi/api/*. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:18: StubCdm(Host* host); explicit https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:21: // ContentDecryptionModule implementation. s/CDM/StubCdmInterface/ https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:68: void FailRequest(uint32 promise_id); Add comment. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... File third_party/widevine/cdm/copy_with_rename.py (right): https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... third_party/widevine/cdm/copy_with_rename.py:28: return shutil.copyfile(argv[2], argv[4]) return twice?
https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm.cc File media/cdm/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.cc:55: void StubCdm::CreateSessionAndGenerateRequest( Add a comment that we provide a dummy message to enable a bit more testing and be consistent with existing testing without a license server. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. Widevine file should be in t_p/wv. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:8: 'enable_widevine%': 0, In common.gypi, this is after the nested block and has the identical variable name instead of 0: 'chromeos%': '<(chromeos)', https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:57: 'stub/widevine_cdm_version.h', Widevine-specific file should not be in common code. Should be in this directory. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:196: [ 'branding != "Chrome" and OS != "android" and enable_widevine==1', { remove the android check here too?
Updated. Still need to work on having the flag only work on Windows/Mac. https://codereview.chromium.org/1043673002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1043673002/diff/1/build/common.gypi#newcode569 build/common.gypi:569: 'enable_widevine%': 0, On 2015/03/28 04:00:11, ddorwin wrote: > This is what Dan tried to do. I don't think it will be allowed. Agreed. Moved to widevine_cdm.gyp. https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/encryp... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/encryp... chrome/browser/media/encrypted_media_browsertest.cc:186: if (expected_title == kEnded) { On 2015/03/28 04:00:11, ddorwin wrote: > You shouldn't need this. Just make your stub return a dummy message. Done. With the stub changes a message event is generated. https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/wv_tes... File chrome/browser/media/wv_test_license_server_config.cc (right): https://codereview.chromium.org/1043673002/diff/1/chrome/browser/media/wv_tes... chrome/browser/media/wv_test_license_server_config.cc:124: #if defined(OS_LINUX) && defined(ARCH_CPU_X86_64) && defined(OFFICIAL_BUILD) On 2015/03/28 04:00:11, ddorwin wrote: > This should not be committed. You can add a TODO to base this on branding=Chrome > somehow. Removed. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... File media/cdm/ppapi/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:9: const char kStubCdmVersion[] = "0.1.0.1"; On 2015/03/30 17:23:50, xhwang wrote: > Let's have some versioning scheme here. How about 1.4.8.0? When the CDM > interface version increments, we'll update the 3rd number. Other numbers will > not likely to change. Is the version used for anything? Changed to 1.4.8.0 and added a comment, but not clear what difference this makes. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:58: FailRequest(promise_id); On 2015/03/28 04:00:11, ddorwin wrote: > Succeed if the parameters are valid. Also, call host_->SendMessage(). Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:137: NOTIMPLEMENTED(); On 2015/03/30 17:23:50, xhwang wrote: > NOTREACHED(); Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:144: NOTIMPLEMENTED(); On 2015/03/30 17:23:50, xhwang wrote: > NOTREACHED(); Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.cc:148: std::string message("Stub CDM."); On 2015/03/30 17:23:50, xhwang wrote: > nit: "Operation not supported by stub CDM"? Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... File media/cdm/ppapi/stub/stub_cdm.h (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:6: #define MEDIA_CDM_PPAPI_STUB_STUB_CDM_H_ On 2015/03/30 17:23:50, xhwang wrote: > ddorwin/jrummell: > > Actually "external_clear_key" and "stub_cdm" have nothing to do with ppapi. The > "ppapi" part is handled by cdm adapter etc. How about we put > "external_clear_key" and "stub_cdm" under media/cdm, instead of media/cdm/ppapi? > Imagine in the future we could have a mojo service loading and using the CDM, > which has nothing to do with ppapi. > > The same for media/cdm/ppapi/api/*. > Moved stub to cdm/. external_clear_key can move later. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:18: StubCdm(Host* host); On 2015/03/30 17:23:50, xhwang wrote: > explicit Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:21: // ContentDecryptionModule implementation. On 2015/03/30 17:23:50, xhwang wrote: > s/CDM/StubCdmInterface/ Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/stub_c... media/cdm/ppapi/stub/stub_cdm.h:68: void FailRequest(uint32 promise_id); On 2015/03/30 17:23:50, xhwang wrote: > Add comment. Done. https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/widevi... File media/cdm/ppapi/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/1/media/cdm/ppapi/stub/widevi... media/cdm/ppapi/stub/widevine_cdm_version.h:14: #define WIDEVINE_CDM_VERSION_STRING "1.4.7.796" On 2015/03/28 04:00:11, ddorwin wrote: > We don't need a version. This file should look like the Android one. Done. Just went by the comments in third_party/widevine/cdm/widevine_cdm_version.h https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... File third_party/widevine/cdm/copy_with_rename.py (right): https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... third_party/widevine/cdm/copy_with_rename.py:2: # Copyright 2014 The Chromium Authors. All rights reserved. On 2015/03/28 04:00:11, ddorwin wrote: > 2015 unless you copied this file. Done. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/co... third_party/widevine/cdm/copy_with_rename.py:28: return shutil.copyfile(argv[2], argv[4]) On 2015/03/30 17:23:50, xhwang wrote: > return twice? Fixed. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:52: '../../../media/cdm/ppapi/stub/widevine_cdm_version.h', On 2015/03/28 04:00:11, ddorwin wrote: > this should be in third_party like the Android file. Done. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:161: '<(DEPTH)/media/media.gyp:stubcdm', On 2015/03/28 04:00:11, ddorwin wrote: > It appears that all except this line are common. Can we share the block above > and only add this? Done. https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:212: [ 'branding == "Chrome" or OS == "android"', { On 2015/03/28 04:00:11, ddorwin wrote: > Android doesn't copy anything. Android never runs this rule anyway. See line 124 > and 129. Done. (I think Android does run this, just that widevine_cdm_binary_files is [] so the copy is a NOP.) https://codereview.chromium.org/1043673002/diff/1/third_party/widevine/cdm/wi... third_party/widevine/cdm/widevine_cdm.gyp:220: [ 'branding != "Chrome" and OS != "android" and enable_widevine==1', { On 2015/03/28 04:00:11, ddorwin wrote: > ditto (Assume this is about Android.) For Android I don't want to run this, since stub_cdm_binary_files will be undefined (and widevine_cdm_binary_files is []).
https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm.h File media/cdm/stub/stub_cdm.h (right): https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.h:6: #define MEDIA_CDM_PPAPI_STUB_STUB_CDM_H_ drop the PPAPI part. https://codereview.chromium.org/1043673002/diff/20001/media/media_cdm.gypi File media/media_cdm.gypi (right): https://codereview.chromium.org/1043673002/diff/20001/media/media_cdm.gypi#ne... media/media_cdm.gypi:158: 'target_name': 'stubcdm', We need to carefully test this to make sure it works with the current WV CDMs. https://codereview.chromium.org/1043673002/diff/20001/media/media_cdm.gypi#ne... media/media_cdm.gypi:169: 'DYLIB_INSTALL_NAME_BASE': '@loader_path', +rkuroiwa for comments: should this be @rpath? https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/copy_with_rename.py (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/copy_with_rename.py:6: import os not used? https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/copy_with_rename.py:22: two empty lines per style guide https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/copy_with_rename.py:32: ditto https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:13: #define WIDEVINE_CDM_AVAILABLE What's the long term plan on this? Should we just define WIDEVINE_CDM_AVAILABLE based on 'enable_widevine'? https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:164: '<(DEPTH)/media/media.gyp:stubcdm', hmm, why not depend on widevine_cdm_binaries? And do we need the "libraries"?
https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:13: #define WIDEVINE_CDM_AVAILABLE On 2015/04/02 18:51:52, xhwang wrote: > What's the long term plan on this? Should we just define WIDEVINE_CDM_AVAILABLE > based on 'enable_widevine'? Yes, this file will go away. Until we have the long-term GYP/GN fix, though, we rely on the magic of including this file in various parts of Chromium.
Updated. https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm.cc File media/cdm/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.cc:55: void StubCdm::CreateSessionAndGenerateRequest( On 2015/04/01 23:27:32, ddorwin wrote: > Add a comment that we provide a dummy message to enable a bit more testing and > be consistent with existing testing without a license server. Done. https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm.h File media/cdm/stub/stub_cdm.h (right): https://codereview.chromium.org/1043673002/diff/20001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.h:6: #define MEDIA_CDM_PPAPI_STUB_STUB_CDM_H_ On 2015/04/02 18:51:52, xhwang wrote: > drop the PPAPI part. Done. https://codereview.chromium.org/1043673002/diff/20001/media/media_cdm.gypi File media/media_cdm.gypi (right): https://codereview.chromium.org/1043673002/diff/20001/media/media_cdm.gypi#ne... media/media_cdm.gypi:158: 'target_name': 'stubcdm', On 2015/04/02 18:51:52, xhwang wrote: > We need to carefully test this to make sure it works with the current WV CDMs. Acknowledged. https://codereview.chromium.org/1043673002/diff/20001/media/media_cdm.gypi#ne... media/media_cdm.gypi:169: 'DYLIB_INSTALL_NAME_BASE': '@loader_path', On 2015/04/02 18:51:52, xhwang wrote: > +rkuroiwa for comments: should this be @rpath? This is simply copied from the clearkeycdm above. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/copy_with_rename.py (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/copy_with_rename.py:6: import os On 2015/04/02 18:51:52, xhwang wrote: > not used? Done. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/copy_with_rename.py:22: On 2015/04/02 18:51:52, xhwang wrote: > two empty lines per style guide Done. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/copy_with_rename.py:32: On 2015/04/02 18:51:52, xhwang wrote: > ditto Done. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/01 23:27:32, ddorwin wrote: > Widevine file should be in t_p/wv. The Android specific one is in t_p/wv/cdm/android, and the generic one is in t_p/wv/cdm. I think it would be confusing to have it in the top directory. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:13: #define WIDEVINE_CDM_AVAILABLE On 2015/04/02 18:51:52, xhwang wrote: > What's the long term plan on this? Should we just define WIDEVINE_CDM_AVAILABLE > based on 'enable_widevine'? enable_widevine is just a gyp variable currently, and doesn't make it into the code. sandersd@ CL was an attempt to clean it up, but ran into problems changing common.gypi (where the -D would need to be specified). https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:8: 'enable_widevine%': 0, On 2015/04/01 23:27:32, ddorwin wrote: > In common.gypi, this is after the nested block and has the identical variable > name instead of 0: > 'chromeos%': '<(chromeos)', Done. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:57: 'stub/widevine_cdm_version.h', On 2015/04/01 23:27:32, ddorwin wrote: > Widevine-specific file should not be in common code. Should be in this > directory. See previous comment in widevine_cdm_version.h. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:164: '<(DEPTH)/media/media.gyp:stubcdm', On 2015/04/02 18:51:52, xhwang wrote: > hmm, why not depend on widevine_cdm_binaries? > > And do we need the "libraries"? Removed, since the widevine_cdm_binaries depends on the stub if necessary. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:196: [ 'branding != "Chrome" and OS != "android" and enable_widevine==1', { On 2015/04/01 23:27:32, ddorwin wrote: > remove the android check here too? For Android there are no stub files, so this will fail (unless I want to make the script more intelligent). Original code just did the 'copies' block, which is a NOP if files == [].
https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/02 23:47:55, jrummell wrote: > On 2015/04/01 23:27:32, ddorwin wrote: > > Widevine file should be in t_p/wv. > > The Android specific one is in t_p/wv/cdm/android, and the generic one is in > t_p/wv/cdm. I think it would be confusing to have it in the top directory. WV-specific code doesn't belong here. You can put it in a stub/ directory, name it stub_.../ or even just use the Android one. That might be the simplest. You can reference 349182, which will eliminate the need for such a file. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:196: [ 'branding != "Chrome" and OS != "android" and enable_widevine==1', { On 2015/04/02 23:47:55, jrummell wrote: > On 2015/04/01 23:27:32, ddorwin wrote: > > remove the android check here too? > > For Android there are no stub files, so this will fail (unless I want to make > the script more intelligent). Original code just did the 'copies' block, which > is a NOP if files == []. Oh, do we evaluate these rules even if there is no path to building them (as is the case on Android)? If so, you're right - you will need that check. https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:14: #define WIDEVINE_CDM_VERSION_STRING "1.4.8.0" Remove. https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:193: '<(DEPTH)/media/media.gyp:stubcdm', Regarding your build failure, perhaps this is somehow exporting a dependency to the adapter. Or maybe linking pulls the original name out of the binary.
https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/04/03 02:06:56, ddorwin wrote: > On 2015/04/02 23:47:55, jrummell wrote: > > On 2015/04/01 23:27:32, ddorwin wrote: > > > Widevine file should be in t_p/wv. > > > > The Android specific one is in t_p/wv/cdm/android, and the generic one is in > > t_p/wv/cdm. I think it would be confusing to have it in the top directory. > > WV-specific code doesn't belong here. > > You can put it in a stub/ directory, name it stub_.../ or even just use the > Android one. That might be the simplest. You can reference 349182, which will > eliminate the need for such a file. As discussed, this is fine. My original comment was incorrect, which led to a confusing conversation. Sorry about that. https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/20001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:57: 'stub/widevine_cdm_version.h', On 2015/04/02 23:47:55, jrummell wrote: > On 2015/04/01 23:27:32, ddorwin wrote: > > Widevine-specific file should not be in common code. Should be in this > > directory. > > See previous comment in widevine_cdm_version.h. Ignore this too. This is relative to widevine/cdm.
https://codereview.chromium.org/1043673002/diff/40001/media/media_cdm.gypi File media/media_cdm.gypi (right): https://codereview.chromium.org/1043673002/diff/40001/media/media_cdm.gypi#ne... media/media_cdm.gypi:159: 'type': 'none', This could become a static library, but the SO creation needs to be moved to widevine_cdm.gyp, and it needs to be linked as libwidevinecdm.so. See the comment in that file. https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:61: '<(PRODUCT_DIR)/libstubcdm.so', On Linux, at least, we cannot just rename the file because GYP *always* sets "-soname", setting the internal DT_SONAME field to the library name at compile time. When we link the adapter, it uses this field rather than the actual binary name from the rename. I can't find a way to a) prevent GYP from doing this or b) ignore the DT_SONAME. Therefore (and to be safe), we should build the stub code as the same name as the actual CDM ("widevinecdm").
Now building the stub as widevinecdm. https://codereview.chromium.org/1043673002/diff/40001/media/media_cdm.gypi File media/media_cdm.gypi (right): https://codereview.chromium.org/1043673002/diff/40001/media/media_cdm.gypi#ne... media/media_cdm.gypi:159: 'type': 'none', On 2015/04/03 19:53:02, ddorwin wrote: > This could become a static library, but the SO creation needs to be moved to > widevine_cdm.gyp, and it needs to be linked as libwidevinecdm.so. See the > comment in that file. Build the stub as widevinecdm now. https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/stub/widevine_cdm_version.h (right): https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/stub/widevine_cdm_version.h:14: #define WIDEVINE_CDM_VERSION_STRING "1.4.8.0" On 2015/04/03 02:06:56, ddorwin wrote: > Remove. Done. https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:61: '<(PRODUCT_DIR)/libstubcdm.so', On 2015/04/03 19:53:02, ddorwin wrote: > On Linux, at least, we cannot just rename the file because GYP *always* sets > "-soname", setting the internal DT_SONAME field to the library name at compile > time. When we link the adapter, it uses this field rather than the actual binary > name from the rename. I can't find a way to a) prevent GYP from doing this or b) > ignore the DT_SONAME. > > Therefore (and to be safe), we should build the stub code as the same name as > the actual CDM ("widevinecdm"). Done. https://codereview.chromium.org/1043673002/diff/40001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:193: '<(DEPTH)/media/media.gyp:stubcdm', On 2015/04/03 02:06:56, ddorwin wrote: > Regarding your build failure, perhaps this is somehow exporting a dependency to > the adapter. Or maybe linking pulls the original name out of the binary. I think linking is pulling the original name out of the binary.
third_party/widevine/ LGTM https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm.cc File media/cdm/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.cc:7: #include "base/logging.h" Do we need this? https://codereview.chromium.org/1043673002/diff/60001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/60001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:168: 'type': 'loadable_module', I wonder if this is actually necessary in this case? But it does make it consistent with the "Chrome" case. Please add some sort of comment indicating that this causes the binary to be in PRODUCT_DIR instead of lib/. (For Clear Key, we say "# Must be in PRODUCT_DIR for ASAN bot." I don't know if that's strictly true in this case.)
This much cleaner! Thanks and LGTM
https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm.cc File media/cdm/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.cc:67: cdm::kLicenseRequest, NULL, 0, NULL, 0); nit: use nullptr
Thanks for the reviews. https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm.cc File media/cdm/stub/stub_cdm.cc (right): https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.cc:7: #include "base/logging.h" On 2015/04/03 23:43:39, ddorwin wrote: > Do we need this? Logging on line 25 and the NOTREACHED() at the bottom. https://codereview.chromium.org/1043673002/diff/60001/media/cdm/stub/stub_cdm... media/cdm/stub/stub_cdm.cc:67: cdm::kLicenseRequest, NULL, 0, NULL, 0); On 2015/04/03 23:54:19, xhwang wrote: > nit: use nullptr Done. https://codereview.chromium.org/1043673002/diff/60001/third_party/widevine/cd... File third_party/widevine/cdm/widevine_cdm.gyp (right): https://codereview.chromium.org/1043673002/diff/60001/third_party/widevine/cd... third_party/widevine/cdm/widevine_cdm.gyp:168: 'type': 'loadable_module', On 2015/04/03 23:43:39, ddorwin wrote: > I wonder if this is actually necessary in this case? But it does make it > consistent with the "Chrome" case. > > Please add some sort of comment indicating that this causes the binary to be in > PRODUCT_DIR instead of lib/. > (For Clear Key, we say "# Must be in PRODUCT_DIR for ASAN bot." I don't know if > that's strictly true in this case.) Done.
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1043673002/#ps80001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1043673002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/534d59853e0803ee61e6c8a6a4ceacac44df6843 Cr-Commit-Position: refs/heads/master@{#323862} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
