Connect CdmAdapter to auxiliary services [chromium/src : master]

John Rummell (Gerrit)

unread,

Jul 27, 2017, 9:22:13 PM7/27/17

to John Rummell, eme-r...@chromium.org, feature-me...@chromium.org, Xiaohan Wang, chromium...@chromium.org

PTAL.

Patch set 1:Commit-Queue +1

View Change

2 comments:

File media/cdm/cdm_adapter.cc:
- Patch Set #1, Line 954: platform_challenge_response
  I forgot to add stuff here.
- Patch Set #1, Line 1106: cdm_platform_verification_
  Other choice is !!cdm_platform_verification_; Which one looks better?

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Xiaohan Wang (Gerrit)

unread,

Jul 28, 2017, 12:51:54 AM7/28/17

to John Rummell, eme-r...@chromium.org, feature-me...@chromium.org, Xiaohan Wang, Commit Bot, chromium...@chromium.org

View Change

10 comments:

File media/base/cdm_output_protection.h:
- Patch Set #1, Line 5: #ifndef MEDIA_BASE_CDM_OUTPUT_PROTECTION_H_
  This is CDM specific, let's put it under media/cdm? Then we don't need "CDM" in the file name, it can just be output_protection.h.
- Patch Set #1, Line 31: // the LinkType values.
  LinkType is not defined.
- Patch Set #1, Line 33: ProtectionType
  ProtectionType is not defined.
File media/base/cdm_platform_verification.h:
- Patch Set #1, Line 6: #define MEDIA_BASE_CDM_PLATFORM_VERIFICATION_H_
  ditto about location and file name
File media/cdm/cdm_adapter.h:
- Patch Set #1, Line 43: // operations. Returns NULL if a FileIO object cannot be obtained. Once a
  Are FileIO and CdmFileIO the same thing?
- Patch Set #1, Line 53: base::OnceCallback<std::unique_ptr<CdmPlatformVerification>()>;
  ditto: s/Cdm// to make the names a bit shorter
- Patch Set #1, Line 58: base::OnceCallback<std::unique_ptr<CdmOutputProtection>()>;
  ditto: s/Cdm//
- Patch Set #1, Line 79: CreateCdmOutputProtectionCB create_cdm_output_protection_cb,
  This makes me wonder whether having a helper class here would be cleaner than having multiple callbacks. Note that we also have a bug to group the 4 callbacks below into a class.
  It'll also make the tests easier to write since you can mock the helper class directly.
  WDYT?
- Patch Set #1, Line 244: bool IsOutputProtectionAvailable();
  Typically we have something like
  std::unique_ptr<PlatformVerification> GetPlatformVerification()
  which will lazy-create |platform_verification_| if it's not created yet.
  It's a bit odd to create objects in an IsFoo() call.
File media/cdm/cdm_adapter_unittest.cc:
- Patch Set #1, Line 368: ExternalClearKeyLibrary(), SUCCESS);
  Thoughts for discussion: I wonder whether we should decouple CdmAdapterTest from ECK. Currently there are some duplication between this test and the browser test. Also, using ECK makes fine control of this test much harder. In theory, we can just mock a cdm::ContentDecryptionModule, and test CdmAdapter against it. Then we can do anything we like in this test.

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 9, 2017, 4:33:22 PM8/9/17

to Dan Sanders, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, John Rummell, Xiaohan Wang

John Rummell would like Dan Sanders to review this change.

View Change

Connect CdmAdapter to auxiliary services

Provide callbacks when creating a CdmAdapter so that it can lazily initialize
OutputProtection and PlatformVerification functionality as needed.

As ExternalClearKey triggers additional tests based on the key system name,
add additional tests that use the different names and verify that they work
as expected.

BUG=749372
TEST=new media_unittests pass

Change-Id: I7e4e22af89e71662636c5f57ce9fe94c1523dbb7
---
M content/utility/utility_service_factory.cc
M media/BUILD.gn
M media/cdm/BUILD.gn
M media/cdm/aes_decryptor_unittest.cc
M media/cdm/cdm_adapter.cc
M media/cdm/cdm_adapter.h
M media/cdm/cdm_adapter_factory.cc
M media/cdm/cdm_adapter_factory.h
M media/cdm/cdm_adapter_unittest.cc
A media/cdm/cdm_auxiliary_helper.cc
A media/cdm/cdm_auxiliary_helper.h
A media/cdm/mock_helpers.cc
A media/cdm/mock_helpers.h
A media/cdm/output_protection.h
A media/cdm/platform_verification.h
M media/mojo/interfaces/BUILD.gn
16 files changed, 587 insertions(+), 87 deletions(-)

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 9, 2017, 4:33:22 PM8/9/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

Updated. Adding sandersd@ as xhwang@ is out for the next couple of weeks.

View Change

13 comments:

File media/base/cdm_output_protection.h:

- Patch Set #1, Line 5:

- This is CDM specific, let's put it under media/cdm? Then we don't need "CDM
  Done

- Patch Set #1, Line 31:

- LinkType is not defined.
  Done

- Patch Set #1, Line 33:

- ProtectionType is not defined.
  Done
File media/base/cdm_platform_verification.h:

- Patch Set #1, Line 6:

- ditto about location and file name
  Done
File media/cdm/cdm_adapter.h:
- Patch Set #1, Line 43: NON_EXPORTED_BASE(public cdm::Host_9) {

- Are FileIO and CdmFileIO the same thing?

- Sort of. CdmFileIO is a cdm::FileIO.
- Patch Set #1, Line 53: const base::FilePath& cdm_path,

- ditto: s/Cdm// to make the names a bit shorter

- Done
- Patch Set #1, Line 58: const SessionKeysChangeCB& session_keys_change_cb,
  ditto: s/Cdm//
  Done
- Patch Set #1, Line 79: std::unique_ptr<SimpleCdmPromise> promise) final;

- This makes me wonder whether having a helper class here would be cleaner th

- Done (including mock class for testing).
- Patch Set #1, Line 244: std::unique_ptr<CdmAuxiliaryHelper> helper_;

- Typically we have something like

- Removed.
File media/cdm/cdm_adapter.cc:

- Patch Set #1, Line 954:

- I forgot to add stuff here.
  Done

- Patch Set #1, Line 1106:

- Other choice is !!cdm_platform_verification_; Which one looks better?
  No longer needed.
File media/cdm/cdm_adapter_unittest.cc:

- Patch Set #1, Line 368:

- Thoughts for discussion: I wonder whether we should decouple CdmAdapterTest
  Removed new tests.
File media/cdm/cdm_auxiliary_helper.cc:
- Patch Set #2, Line 38: callback
  These should call |callback| with default values, I'll update in next pass.

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 9, 2017, 9:10:05 PM8/9/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

rebase + fix missing callback calls.

Patch set 3:Commit-Queue +1

View Change

1 comment:

File media/cdm/cdm_auxiliary_helper.cc:

- Patch Set #2, Line 38: callback
  These should call |callback| with default values, I'll update in next pass.

- Done

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Dan Sanders (Gerrit)

unread,

Aug 14, 2017, 4:06:07 PM8/14/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

Patch set 3:Code-Review +1

View Change

2 comments:

File content/utility/utility_service_factory.cc:
- Patch Set #3, Line 125: CreateAllocator
  Nit: These should probably be named with a 'Get' prefix, since they may or may not actually create something (the public methods using GetAllocator() would probably look a little better too).
File media/cdm/cdm_adapter.h:
- Patch Set #3, Line 194: ChallengePlatformDone
  Any particular reason not to use an 'On' prefix for these?

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 15, 2017, 4:53:50 PM8/15/17

to John Rummell, Xiaohan Wang, Dan Sanders, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Darin Fisher, chromium...@chromium.org, John Abd-El-Malek, Commit Bot, Adam Barth, Aaron Boodman

John Rummell uploaded patch set #5 to this change.

View Change

Connect CdmAdapter to auxiliary services

Provide a helper when creating a CdmAdapter so that it can lazily initialize
auxiliary services (memory allocation, file IO, output protection, and platform
verification) as needed. Previously this was done by providing callbacks for
each service, but adding 2 more (output protection and platform verification)
would make things unnecessarily complex.

BUG=749372
TEST=media_unittests and EME browser_tests pass



Change-Id: I7e4e22af89e71662636c5f57ce9fe94c1523dbb7
---
M content/utility/utility_service_factory.cc

M media/cdm/BUILD.gn
M media/cdm/aes_decryptor_unittest.cc
M media/cdm/cdm_adapter.cc
M media/cdm/cdm_adapter.h
M media/cdm/cdm_adapter_factory.cc
M media/cdm/cdm_adapter_factory.h
M media/cdm/cdm_adapter_unittest.cc
A media/cdm/cdm_auxiliary_helper.cc
A media/cdm/cdm_auxiliary_helper.h
A media/cdm/mock_helpers.cc
A media/cdm/mock_helpers.h
A media/cdm/output_protection.h
A media/cdm/platform_verification.h
M media/mojo/interfaces/BUILD.gn

15 files changed, 591 insertions(+), 83 deletions(-)

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 15, 2017, 4:54:40 PM8/15/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

+nasko@ for OWNERS review of content/utility/utility_service_factory.cc

View Change

2 comments:

File content/utility/utility_service_factory.cc:
- Patch Set #3, Line 125: GetAllocator()
  Nit: These should probably be named with a 'Get' prefix, since they may or may not actually create s […]
  Done
File media/cdm/cdm_adapter.h:
- Patch Set #3, Line 194: OnChallengePlatformDo

- Any particular reason not to use an 'On' prefix for these?

- Nope. Done.

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Nasko Oskov (Gerrit)

unread,

Aug 16, 2017, 7:52:11 PM8/16/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

View Change

4 comments:

File content/utility/utility_service_factory.cc:
- Patch Set #5, Line 75: GetAllocator();
  Why not return the allocator out of GetAllocator, then the code becomes a bit more readable:
  return GetAllocator()->CreateCmdBuffer(capacity);
  Very easy to follow one liner.
- Patch Set #5, Line 87: GetOutputProtection();
  I think code here can be made easier to follow too.
```
if (GetOutputProtection()) {
  // make the call
}
```
  And GetOutputProtection() can return nullptr if it was attempted but didn't succeed getting one.
- Patch Set #5, Line 149: MOJO
  nit: s/MOJO/Mojo/
- Patch Set #5, Line 150: // The service may not exist, or may fail later.
  If it fails later, is the interface pointer nullified? Where does that happen?

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 18, 2017, 8:27:28 PM8/18/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

Updated.

View Change

4 comments:

File content/utility/utility_service_factory.cc:
- Patch Set #5, Line 75: return GetAllocator()->CreateCdmBuffer(capacity);
  Why not return the allocator out of GetAllocator, then the code becomes a bit more readable: […]
  Done

- Patch Set #5, Line 87: }

- I think code here can be made easier to follow too. […]
  Changed to "ConnectedToOutputProtection()", so that using |output_protection_| next makes sense.
- Patch Set #5, Line 149: fal
  nit: s/MOJO/Mojo/
  Done
- Patch Set #5, Line 150: bool platform_verification_attempted_ = false;

- If it fails later, is the interface pointer nullified? Where does that happen?

- Changed to using is_bound() as the test. I would think that communication failures or the other end going away would cause the connection to no longer be bound.

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 23, 2017, 7:11:40 PM8/23/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Kinuko Yasuda, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

+kinuko@ for OWNERS review of content/utility/utility_service_factory.cc as nasko@ is OOO.

View Change

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Kinuko Yasuda (Gerrit)

unread,

Aug 24, 2017, 12:29:37 AM8/24/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

looking good, some nits

View Change

4 comments:

File content/utility/utility_service_factory.cc:
- Patch Set #6, Line 63: MojoCdmHelper(service_manager::mojom::InterfaceProvider* interface_provider)
  nit: explicit
- Patch Set #6, Line 65: ~MojoCdmHelper() override {}
  nit: = default
- Patch Set #6, Line 84: media::ScopedCallbackRunner(std::move(callback), false, 0, 0);
  Let me make sure-- so this would make sure the callback fires when it goes out of scope if it's not passed to another owner?
- Patch Set #6, Line 94: if (ConnectToOutputProtection()) {
  nit/optional: I slightly prefer early return pattern (here and below)
```
if (!ConnectToOutputProtection())
  return false;
output_protection_->...
```

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 24, 2017, 7:39:57 PM8/24/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Kinuko Yasuda, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

Updated.

View Change

4 comments:

File content/utility/utility_service_factory.cc:
- Patch Set #6, Line 63: explicit MojoCdmHelper(
  nit: explicit
  Done
- Patch Set #6, Line 65: : interface_provider_(interface_provider) {}
  nit: = default
  Done
- Patch Set #6, Line 84: QueryStatusCB scoped_callback =
  Let me make sure-- so this would make sure the callback fires when it goes out of scope if it's not […]
  Correct. If it's not passed on, when going out of scope it calls |callback| with the other parameters.
- Patch Set #6, Line 94: EnableProtectionCB scoped_callback =
  nit/optional: I slightly prefer early return pattern (here and below) […]
  Done

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Kinuko Yasuda (Gerrit)

unread,

Aug 29, 2017, 6:54:25 PM8/29/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

Sorry for the delay!

Patch set 7:Code-Review +1

View Change

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

John Rummell (Gerrit)

unread,

Aug 29, 2017, 7:11:56 PM8/29/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Kinuko Yasuda, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, Commit Bot, chromium...@chromium.org

Thanks for the reviews.

Patch set 7:Commit-Queue +2

View Change

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Aug 29, 2017, 9:29:42 PM8/29/17

to John Rummell, alokp...@chromium.org, dari...@chromium.org, eme-r...@chromium.org, feature-me...@chromium.org, qsr+...@chromium.org, viettrung...@chromium.org, yzshen...@chromium.org, Kinuko Yasuda, Dan Sanders, Aaron Boodman, Adam Barth, Darin Fisher, John Abd-El-Malek, Xiaohan Wang, chromium...@chromium.org

Commit Bot merged this change.

View Change

Approvals:
  Kinuko Yasuda: Looks good to me
  Dan Sanders: Looks good to me
  John Rummell: Commit

Connect CdmAdapter to auxiliary services

Provide a helper when creating a CdmAdapter so that it can lazily initialize
auxiliary services (memory allocation, file IO, output protection, and platform
verification) as needed. Previously this was done by providing callbacks for
each service, but adding 2 more (output protection and platform verification)
would make things unnecessarily complex.

BUG=749372
TEST=media_unittests and EME browser_tests pass

Change-Id: I7e4e22af89e71662636c5f57ce9fe94c1523dbb7

Reviewed-on: https://chromium-review.googlesource.com/590333
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Reviewed-by: Dan Sanders <sand...@chromium.org>
Commit-Queue: John Rummell <jrum...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498331}
---
M content/utility/utility_service_factory.cc


M media/cdm/BUILD.gn
M media/cdm/aes_decryptor_unittest.cc
M media/cdm/cdm_adapter.cc
M media/cdm/cdm_adapter.h
M media/cdm/cdm_adapter_factory.cc
M media/cdm/cdm_adapter_factory.h
M media/cdm/cdm_adapter_unittest.cc
A media/cdm/cdm_auxiliary_helper.cc
A media/cdm/cdm_auxiliary_helper.h
A media/cdm/mock_helpers.cc
A media/cdm/mock_helpers.h
A media/cdm/output_protection.h
A media/cdm/platform_verification.h
M media/mojo/interfaces/BUILD.gn

15 files changed, 593 insertions(+), 83 deletions(-)

To view, visit change 590333. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward