Add CdmWrapper to support multiple CDM versions in CdmAdapter.

Add CdmWrapper to support multiple CDM versions in CdmAdapter.

CdmWrapper wraps different versions of ContentDecryptionModule interfaces and exposes a common interface to the caller.

BUG=306647
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229571

[xhwang, 2013-10-15 20:42:32]

PTAL

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_helpers.h
File media/cdm/ppapi/cdm_helpers.h (right):

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_helper...
media/cdm/ppapi/cdm_helpers.h:1: // Copyright 2013 The Chromium Authors. All
rights reserved.
This file is copied from cdm_wrapper.cc, with small modifications commented
below.

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_helper...
media/cdm/ppapi/cdm_helpers.h:21: #include "ppapi/cpp/module.h"
Includes are trimmed to reflect what we use.

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_helper...
media/cdm/ppapi/cdm_helpers.h:115:
PP_DCHECK(pp::Module::Get()->core()->IsMainThread());
This was IsMainThread() in cdm_wrapper.cc. I am not moving IsMainThread() etc
over since I feel they are more related to the PPAPI handling code which is
mostly in cdm_wrapper.cc.

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_helper...
media/cdm/ppapi/cdm_helpers.h:298: cdm::Buffer* PassFrameBuffer() {
Added this function so that we can transfer ownership.

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/linked_ptr.h
File media/cdm/ppapi/linked_ptr.h (right):

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/linked_ptr...
media/cdm/ppapi/linked_ptr.h:118: operator T*() const { return value_; }
This is to avoid numerous cdm_.get() calls in CdmWrapper.

e.g. 
PP_DCHECK(cdm_.get());
if (cdm_.get())...

Also this matches what scoped_refptr is doing: 
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...

[DaleCurtis, 2013-10-17 01:01:57]

Copying our chat conversation: I think CdmHelpers is fine, but I'm not sure
about CdmAdapter.

The v2 methods could instead be handled by casting a void* cdm_ pointer type
where necessary based on a version check.  The version can be saved during
initialization.  Something like:

template <class T>
T* CdmWrapper::GetCDM(int version) {
  PP_DCHECK(version == version_);
  return static_cast<T>(cdm_);
}

Most v2 methods only interact with v2 CDMs, so a single cast at the top is
sufficient.  The only case where we mix v1, v2 usage currently is the new
DecryptAndDecodeSamples() method, so it seems pretty easy to just have a if
(version_ == 1) v1() else v2().

cdm_ can be wrapped in a private inner class to avoid accidents if you're so
inclined.

[xhwang, 2013-10-17 05:53:18]

On 2013/10/17 01:01:57, DaleCurtis wrote:
> Copying our chat conversation: I think CdmHelpers is fine, but I'm not sure
> about CdmAdapter.
> 
> The v2 methods could instead be handled by casting a void* cdm_ pointer type
> where necessary based on a version check.  The version can be saved during
> initialization.  Something like:
> 
> template <class T>
> T* CdmWrapper::GetCDM(int version) {
>   PP_DCHECK(version == version_);
>   return static_cast<T>(cdm_);
> }
> 
> Most v2 methods only interact with v2 CDMs, so a single cast at the top is
> sufficient.  The only case where we mix v1, v2 usage currently is the new
> DecryptAndDecodeSamples() method, so it seems pretty easy to just have a if
> (version_ == 1) v1() else v2().
> 
> cdm_ can be wrapped in a private inner class to avoid accidents if you're so
> inclined.

I thought about this. Since CdmWrapper needs to support both versions of CDM
interface, It seems we always need to use the following pattern to make calls,
even if the signature of Foo is the same in CDM_1 and CDM_2:

if (version_ == 1)
  GetCDM<CDM_1>()->Foo();
else (...)
  GetCDM<CDM_2>()->Foo();

I don't find a way to simplify this since CDM_1 and CDM_2 are two different
types. Am I missing anything?

[DaleCurtis, 2013-10-17 18:38:39]

That's correct.  Ideally this is temporary though as older versions are
deprecated.

[xhwang, 2013-10-17 19:13:54]

On 2013/10/17 18:38:39, DaleCurtis wrote:
> That's correct.  Ideally this is temporary though as older versions are
> deprecated.

Yeah, the need for this is temporary. But the code will stay (in preparation for
the next wave of update). It seems we'll have boilerplate code in some place
anyways. With that, I feel I like CdmAdapter a little bit more. I like the
layering of this CL:
- CdmWrapper concentrates on pepper stuff
- CdmAdapter deals with different CDMs

For your approach, I like the fact that we may end up adding less new code, but
I don't like the fact that CdmWrapper will be even bigger, with both pepper and
CDM handling in each function.

WDYT?

ddorwin: what's your opinion?

[DaleCurtis, 2013-10-17 20:31:05]

I find the templates slightly confusing, but if the consensus is in favor I'm
happy to roll with it.

Note: Must of your savings in CdmWrapper are from introducing CdmHelpers, which
could be done outside of this change.  Want to split that into a separate CL?

We could also move out the CdmWrapper prototypes into cdm_wrapper.h.

The GetCDM() templates could then go in that header file to avoid blowing up
cdm_wrapper.cc anymore.

[DaleCurtis, 2013-10-17 20:31:57]

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_wrappe...
File media/cdm/ppapi/cdm_wrapper.cc (right):

https://codereview.chromium.org/26155003/diff/5001/media/cdm/ppapi/cdm_wrappe...
media/cdm/ppapi/cdm_wrapper.cc:220: class CdmWrapper : public pp::Instance,
This being the prototype I'm talking about...

[xhwang, 2013-10-17 21:05:01]

On 2013/10/17 20:31:05, DaleCurtis wrote:
> I find the templates slightly confusing, but if the consensus is in favor I'm
> happy to roll with it.
> 
> Note: Must of your savings in CdmWrapper are from introducing CdmHelpers,
which
> could be done outside of this change.  Want to split that into a separate CL?
> 
> We could also move out the CdmWrapper prototypes into cdm_wrapper.h.

I'll make another CL for cdm_helpers.h and cdm_wrapper.h. Also, ddorwin and I
agreed
that CdmWrapper should be renamed to CdmAdapter to match the binary name
"xxxxcdmadapter.so".

> The GetCDM() templates could then go in that header file to avoid blowing up
> cdm_wrapper.cc anymore.

GetCDM() is not an issue. But calling the following in every function makes me
sad:

if (version_ == 1)
  GetCDM<CDM_1>()->Foo();
else (...)
  GetCDM<CDM_2>()->Foo();

[DaleCurtis, 2013-10-17 22:18:12]

OIC, yeah that's unfortunate.  It could be obscured with a macro though.

[xhwang, 2013-10-17 22:25:05]

On 2013/10/17 22:18:12, DaleCurtis wrote:
> OIC, yeah that's unfortunate.  It could be obscured with a macro though.

The cleanups are in 27774002.

I thought about using macro as well. But different calls have different number
of parameters, also we need to be able to pass the function name into the macro,
which I don't see a easy way to do...

[DaleCurtis, 2013-10-17 22:30:16]

With some liberal use of the ternary operator you can treat it as a hidden cast
I think:

#define CDM (version == 1 ? static_cast<CDM_1>(cdm_) : static_cast<CDM_2>(cdm_))

Though your templates are looking better and better :)

[xhwang, 2013-10-18 21:06:34]

Rebased. PTAL again.

[xhwang, 2013-10-18 21:07:46]

On 2013/10/18 21:06:34, xhwang wrote:
> Rebased. PTAL again.

Since we reverted the CDM DEPS roll. I removed the specialization functions in
this CL and left some examples. When we roll the DEPS again, we can put them
back.

[DaleCurtis, 2013-10-18 21:21:08]

lgtm

[ddorwin, 2013-10-18 22:17:01]

LG overall; just some minor issues.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:20: // CDM interface (i.e.
ContentDecryptionModule). If such an instance cannot be
not i.e. - it is exactly this type.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:78: // TODO(xhwang): Remove CdmVersion template
parameter after we roll CDM DEPS.
CDM.h DEPS?

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:79: template <class CdmType, int CdmVersion>
CdmType is a bit misleading. CdmInterface? CdmVersionInterface?

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:96: if (cdm_)
Always true.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/linked_ptr.h
File media/cdm/ppapi/linked_ptr.h (right):

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/linked_pt...
media/cdm/ppapi/linked_ptr.h:118: operator T*() const { return value_; }
You probably don't need this based on my comments. Also, I think it's better to
avoid modifying this in ways that base/ does not have, especially if the impact
is just a few more characters in a small number of places.

[xhwang, 2013-10-18 23:03:51]

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:20: // CDM interface (i.e.
ContentDecryptionModule). If such an instance cannot be
On 2013/10/18 22:17:01, ddorwin wrote:
> not i.e. - it is exactly this type.

Done.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:78: // TODO(xhwang): Remove CdmVersion template
parameter after we roll CDM DEPS.
On 2013/10/18 22:17:01, ddorwin wrote:
> CDM.h DEPS?

Done.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:79: template <class CdmType, int CdmVersion>
On 2013/10/18 22:17:01, ddorwin wrote:
> CdmType is a bit misleading. CdmInterface? CdmVersionInterface?

Done.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/cdm_wrapp...
media/cdm/ppapi/cdm_wrapper.h:96: if (cdm_)
On 2013/10/18 22:17:01, ddorwin wrote:
> Always true.

Done.

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/linked_ptr.h
File media/cdm/ppapi/linked_ptr.h (right):

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/linked_pt...
media/cdm/ppapi/linked_ptr.h:118: operator T*() const { return value_; }
On 2013/10/18 22:17:01, ddorwin wrote:
> You probably don't need this based on my comments. Also, I think it's better
to
> avoid modifying this in ways that base/ does not have, especially if the
impact
> is just a few more characters in a small number of places.

It's used in a lot of places in CdmAdapter. So I'd like to keep this change.
WDYT?

[ddorwin, 2013-10-18 23:45:55]

lgtm

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/linked_ptr.h
File media/cdm/ppapi/linked_ptr.h (right):

https://codereview.chromium.org/26155003/diff/26001/media/cdm/ppapi/linked_pt...
media/cdm/ppapi/linked_ptr.h:118: operator T*() const { return value_; }
On 2013/10/18 23:03:52, xhwang wrote:
> On 2013/10/18 22:17:01, ddorwin wrote:
> > You probably don't need this based on my comments. Also, I think it's better
> to
> > avoid modifying this in ways that base/ does not have, especially if the
> impact
> > is just a few more characters in a small number of places.
> 
> It's used in a lot of places in CdmAdapter. So I'd like to keep this change.
> WDYT?

As discussed, this is fine.

[commit-bot: I haz the power, 2013-10-19 02:23:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/26155003/156001

[commit-bot: I haz the power, 2013-10-19 15:49:41]

Change committed as 229571