Introduce CdmContext interface.

Introduce CdmContext interface.

CdmContext is what a media pipeline needs from a CDM to decrypt (and decode) an
encrypted stream.

BUG=401264
Committed: https://crrev.com/d4dc81df18391b86aeccac802d383e672ccd91e2
Cr-Commit-Position: refs/heads/master@{#305181}

[xhwang, 2014-11-20 19:08:05]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org

[xhwang, 2014-11-20 19:08:06]

xhwang@chromium.org changed required reviewers:
+ ddorwin@chromium.org

[xhwang, 2014-11-20 19:08:06]

This is the new interface I meant for SetCdm() refactoring work. PTAL

https://codereview.chromium.org/748473002/diff/1/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/748473002/diff/1/media/base/cdm_context.h#new...
media/base/cdm_context.h:31: #if defined(ENABLE_BROWSER_CDMS)
I kind of hate having this #ifdef in so many places around CDM ID. How about we
remove this #ifdef and make CDM ID available on all platforms? If a CDM ID is
not used it can simply return kInvalidCdmId.

[xhwang, 2014-11-20 19:10:00]

Patchset #2 (id:20001) has been deleted

[xhwang, 2014-11-20 19:32:19]

Patchset #2 (id:40001) has been deleted

[ddorwin, 2014-11-20 21:31:47]

https://codereview.chromium.org/748473002/diff/60001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/748473002/diff/60001/media/base/cdm_context.h...
media/base/cdm_context.h:16: // content decryption module (CDM) to decrypt (and
decode) encrypted buffers.
And possibly render, right?

https://codereview.chromium.org/748473002/diff/60001/media/base/cdm_context.h...
media/base/cdm_context.h:23: CdmContext();
Doesn't need to be public?

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:38: class MEDIA_EXPORT MediaKeys : public CdmContext{
Should the CDM be a Context or provide a context?

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:77: MediaKeys();
ditto

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:122: // CdmContext implementation
nit: period.

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:123: Decryptor* GetDecryptor() override;
I may have asked this before, but why isn't this just pure virtual requiring all
concrete instances to decide?

[xhwang, 2014-11-21 01:34:27]

https://codereview.chromium.org/748473002/diff/60001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/748473002/diff/60001/media/base/cdm_context.h...
media/base/cdm_context.h:16: // content decryption module (CDM) to decrypt (and
decode) encrypted buffers.
On 2014/11/20 21:31:46, ddorwin wrote:
> And possibly render, right?

We don't really use CDM to render streams. Rendering is still the responsibility
of media::Renderer, the consumer of this class.

https://codereview.chromium.org/748473002/diff/60001/media/base/cdm_context.h...
media/base/cdm_context.h:23: CdmContext();
On 2014/11/20 21:31:46, ddorwin wrote:
> Doesn't need to be public?

Done.

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:38: class MEDIA_EXPORT MediaKeys : public CdmContext{
On 2014/11/20 21:31:46, ddorwin wrote:
> Should the CDM be a Context or provide a context?

Yes, I also debated on that one. I think it makes more sense for a MediaKeys to
"provide" a CdmConetxt. Using inheritance is convenient and will result in less
code. But I agree we shouldn't sacrifice interface for implementation
convenience.

Done.

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:77: MediaKeys();
On 2014/11/20 21:31:46, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:122: // CdmContext implementation
On 2014/11/20 21:31:47, ddorwin wrote:
> nit: period.

Done.

https://codereview.chromium.org/748473002/diff/60001/media/base/media_keys.h#...
media/base/media_keys.h:123: Decryptor* GetDecryptor() override;
On 2014/11/20 21:31:46, ddorwin wrote:
> I may have asked this before, but why isn't this just pure virtual requiring
all
> concrete instances to decide?

Done, so that we won't discuss this again in the future ;)

[xhwang, 2014-11-21 04:43:25]

Patchset #4 (id:100001) has been deleted

[ddorwin, 2014-11-21 05:04:27]

LGTM with suggestions.

https://codereview.chromium.org/748473002/diff/120001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/748473002/diff/120001/media/blink/cdm_session...
media/blink/cdm_session_adapter.h:96: Decryptor* GetDecryptor();
What do we use this for? Should this class be a CdmContext? It has GetCdmId too.
Or maybe it should return a CdmContext since we just pass these functions
through to one anyway.. Do whatever makes the most sense layering-wise.

https://codereview.chromium.org/748473002/diff/120001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/748473002/diff/120001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:45: Decryptor* ProxyDecryptor::GetDecryptor() {
It doesn't matter much, but we _could_ possibly return a CdmContext instead.
Does that make the prefixed and unprefixed paths more similar?

[xhwang, 2014-11-21 05:15:41]

Thanks for the review!

https://codereview.chromium.org/748473002/diff/120001/media/blink/cdm_session...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/748473002/diff/120001/media/blink/cdm_session...
media/blink/cdm_session_adapter.h:96: Decryptor* GetDecryptor();
On 2014/11/21 05:04:27, ddorwin wrote:
> What do we use this for? Should this class be a CdmContext? It has GetCdmId
too.
> Or maybe it should return a CdmContext since we just pass these functions
> through to one anyway.. Do whatever makes the most sense layering-wise.

This CL only introduce the new interface. In the next CL this will indeed be
updated to GetCdmContext().

https://codereview.chromium.org/748473002/diff/120001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/748473002/diff/120001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:45: Decryptor* ProxyDecryptor::GetDecryptor() {
On 2014/11/21 05:04:27, ddorwin wrote:
> It doesn't matter much, but we _could_ possibly return a CdmContext instead.
> Does that make the prefixed and unprefixed paths more similar?

This will return a CdmContext in the next CL.

[xhwang, 2014-11-21 05:15:50]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-11-21 05:16:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/748473002/120001

[commit-bot: I haz the power, 2014-11-21 06:30:41]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2014-11-21 06:31:17]

Patchset 4 (id:??) landed as
https://crrev.com/d4dc81df18391b86aeccac802d383e672ccd91e2
Cr-Commit-Position: refs/heads/master@{#305181}