Plumb PPAPI PlatformVerification into CDM.h

media/cdm/ppapi/cdm_wrapper.cc

Index: media/cdm/ppapi/cdm_wrapper.cc
diff --git a/media/cdm/ppapi/cdm_wrapper.cc b/media/cdm/ppapi/cdm_wrapper.cc
index 6348f1954d0e9b8c30d724e2f6bbce62a1ccb1d9..a91636546a5e3190cf75014bda339d104c6167f3 100644
--- a/media/cdm/ppapi/cdm_wrapper.cc
+++ b/media/cdm/ppapi/cdm_wrapper.cc
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "build/build_config.h"
 #include "media/cdm/ppapi/api/content_decryption_module.h"

[ddorwin, 2013/09/17 05:26:46]

You need a DEPS roll in this CL. (Make sure it's backwards compatible.)

 #include "media/cdm/ppapi/linked_ptr.h"
 #include "ppapi/c/pp_errors.h"
@@ -33,6 +34,10 @@
 #include "ppapi/cpp/instance_handle.h"
 #endif  // defined(CHECK_DOCUMENT_URL)
 
+#if defined(OS_CHROMEOS)
+#include "ppapi/cpp/private/platform_verification.h"
+#endif
+
 namespace {
 
 bool IsMainThread() {
@@ -485,7 +490,8 @@ void* GetCdmHost(int host_interface_version, void* user_data);
 // Content Decryption Module (CDM).
 class CdmWrapper : public pp::Instance,
                    public pp::ContentDecryptor_Private,
-                   public cdm::Host {
+                   public cdm::Host_1,
+                   public cdm::Host_2 {

[dmichael (off chromium), 2013/09/17 20:10:46]

Is this just temporary to deal with DEPS? If so, fine. Otherwise, I would rather
have descriptive names :-)

[DaleCurtis, 2013/09/17 20:18:51]

Temporary IIUC.  Host_1 can be removed in favor of just Host once all clients
are converted.

[ddorwin, 2013/09/17 23:24:39]

Correct. Also, _2 is a version of the Host interface struct in our
backwards-compatible API. It's not just type1 and type2. :)

  public:
   CdmWrapper(PP_Instance instance, pp::Module* module);
   virtual ~CdmWrapper();
@@ -523,7 +529,7 @@ class CdmWrapper : public pp::Instance,
       pp::Buffer_Dev encrypted_buffer,
       const PP_EncryptedBlockInfo& encrypted_block_info) OVERRIDE;
 
-  // cdm::Host implementation.
+  // cdm::Host_1 implementation.
   virtual cdm::Buffer* Allocate(int32_t capacity) OVERRIDE;
   virtual void SetTimer(int64_t delay_ms, void* context) OVERRIDE;
   virtual double GetCurrentWallTimeInSeconds() OVERRIDE;
@@ -538,6 +544,14 @@ class CdmWrapper : public pp::Instance,
   virtual void GetPrivateData(int32_t* instance,
                               GetPrivateInterface* get_interface) OVERRIDE;
 
+  // cdm::Host_2 implementation.
+  virtual void SendPlatformChallenge(
+      const char* service_id, int32_t service_id_length,
+      const char* challenge, int32_t challenge_length) OVERRIDE;
+  virtual void EnableOutputProtection(
+      uint32_t desired_protection_mask) OVERRIDE;
+  virtual void QueryOutputProtectionStatus() OVERRIDE;
+
  private:
   struct SessionInfo {
     SessionInfo(const std::string& key_system_in,
@@ -605,10 +619,40 @@ class CdmWrapper : public pp::Instance,
 
   bool IsValidVideoFrame(const LinkedVideoFrame& video_frame);
 
+#if defined(OS_CHROMEOS)
+  bool CanChallengePlatform();
+  void CanChallengePlatformDone(int32_t result, bool can_challenge_platform);

[ddorwin, 2013/09/17 05:26:46]

OnCan... ?

[DaleCurtis, 2013/09/17 18:49:33]

Style elsewhere in the file is XxxDone; i.e., DecoderInitializeDone(), etc.  I'm
happy to change it if that's what you prefer though.

+  void SendPlatformChallengeDone(int32_t result);
+
+  // Since CanChallengePlatform() needs to return its value immediately, we need
+  // to delay GenerateKeyRequest() until its value is available.
+  // TODO(dalecurtis): This stinks.  We need to find a way to deliver the value
+  // synchronously.  Potentially as part of the new Initialize(), see
+  // http://crbug.com/250049
+  void DelayedGenerateKeyRequest(int32_t result,
+                                 const std::string& key_system,
+                                 const std::string& type,
+                                 pp::VarArrayBuffer init_data);
+
+  pp::PlatformVerification platform_verification_;
+  bool is_can_challenge_platform_set_;

[ddorwin, 2013/09/17 05:26:46]

It's not clear from the name what this does. _initialized_? _pending_? In the
latter case, it would be true in the constructor then cleared.

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+  pp::CompletionCallback delayed_gkr_;
+
+  // Since PPAPI doesn't provide handlers for CompleteCallbacks w/ more than one

[ddorwin, 2013/09/17 05:26:46]

CompletionC...

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+  // output we need to manage our own.  These values are read only by

[ddorwin, 2013/09/17 05:26:46]

nit: since this order could be ambiguous, swap read & only: only read

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+  // SendPlatformChallengeDone().
+  // TODO(dalecurtis): CompletionCallbackFactory mentions this may not be safe
+  // since the module could be destructed before callbacks complete... True?

[dmichael (off chromium), 2013/09/17 20:10:46]

Can you point me to the comment you saw that indicates this? I don't *think*
that can happen here.

[DaleCurtis, 2013/09/17 20:18:51]

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/utility/comp...

[dmichael (off chromium), 2013/09/17 20:36:58]

Ah, thanks. This stuff is confusing, and I still forget about that problem.

Your case is a little different than what it's talking about. Your callbacks are
scoped to platform_verification_, and when it gets destroyed, those will get
aborted. Your Vars are scoped to the instance, so they get destroyed about the
same time...  and in the abort case, you don't write to the Vars. So you should
be safe. It would be dangerous if you could get a "successful" callback
invocation after the Vars are destroyed, but that doesn't seem possible here.

[DaleCurtis, 2013/09/23 18:30:57]

Thanks for the explanation.  Comment removed.

+  pp::Var signed_data_output_;
+  pp::Var signed_data_signature_output_;
+  pp::Var platform_key_certificate_output_;
+#endif
+
   PpbBufferAllocator allocator_;
   pp::CompletionCallbackFactory<CdmWrapper> callback_factory_;
   cdm::ContentDecryptionModule* cdm_;

[Rintaro Kuroiwa, 2013/09/20 01:55:06]

When OS_CHROMEOS is defined, this will error at 1264.

[DaleCurtis, 2013/09/23 18:30:57]

Yeah I have another change to the CDM.h that switches over to v2 as the default.
 I'll get that uploaded separately.

   std::string key_system_;
+  bool can_challenge_platform_;

[ddorwin, 2013/09/17 05:26:46]

We don't really need this other than OS_CHROMEOS. It adds one more ifdef, but
it's less confusing when grouped I think.

[DaleCurtis, 2013/09/23 18:30:57]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(CdmWrapper);
 };
@@ -616,9 +660,20 @@ class CdmWrapper : public pp::Instance,
 CdmWrapper::CdmWrapper(PP_Instance instance, pp::Module* module)
     : pp::Instance(instance),
       pp::ContentDecryptor_Private(this),
+#if defined(OS_CHROMEOS)
+      platform_verification_(this),
+      is_can_challenge_platform_set_(false),
+#endif
       allocator_(this),
-      cdm_(NULL) {
+      cdm_(NULL),
+      can_challenge_platform_(false) {
   callback_factory_.Initialize(this);
+#if defined(OS_CHROMEOS)
+  // Preemptively retrieve the platform challenge status.  It will not change.
+  platform_verification_.CanChallengePlatform(

[ddorwin, 2013/09/17 05:26:46]

Re-entrancy from an Instance constructor seems like a bad idea. Maybe
PostOnMain. That's also re-entrant, but at least I think that operates at a
module level.

[DaleCurtis, 2013/09/17 18:49:33]

Hmm, it will end up as a post anyways when the IPC message is sent, but in an
error state the completion callback might be called prior to returning. 
dmichael@ WDYT?

[dmichael (off chromium), 2013/09/17 20:10:46]

I'm not sure what re-entrancy you're worried about.

The default completion callback is "required", meaning it will run
asynchronously.
(https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_complet...)

So CdmWrapper won't be re-entered.

Re-entering in to Pepper is par for the course...  we have to deal with it; it's
just normal to call Pepper functions when Pepper calls you.

So this looks fine to me.

[ddorwin, 2013/09/17 23:24:39]

I was just worried about re-entrancy when we're in the middle of creating a
pp::Instance. CdmWrapper is a pp::Instance, and we're invoking a completely
separate PPB interface from the constructor of the pp::Instance.

+      callback_factory_.NewCallbackWithOutput(
+          &CdmWrapper::CanChallengePlatformDone));
+#endif
 }
 
 CdmWrapper::~CdmWrapper() {
@@ -652,6 +707,16 @@ void CdmWrapper::GenerateKeyRequest(const std::string& key_system,
   PP_DCHECK(0 < url_components.host.len);
 #endif  // defined(CHECK_DOCUMENT_URL)
 
+#if defined(OS_CHROMEOS)
+  // Ensure CanChallengePlatform() has valid data before initializing the CDM;
+  // delaying the GenerateKeyRequest if necessary.
+  if (!is_can_challenge_platform_set_) {
+    delayed_gkr_ = callback_factory_.NewCallback(

[ddorwin, 2013/09/17 05:26:46]

What if you get multiple GKRs at the same time?

[DaleCurtis, 2013/09/17 18:49:33]

Ugh, I didn't realize that was a thing.  To handle delay I'll need to use a list
/ deque of structs containing the initialization data.  

Note, this also unblocks the renderer thread when it returns, allowing other
decoding tasks to start processing.  In testing if the delay is too long the
tests will fail since playback reaches > 1 second without a keyAdded event.

[DaleCurtis, 2013/09/23 18:30:57]

Yeah, delay isn't going to work.  I've instead changed it to err on the side of
the most common case if the value isn't available for now... It's not great, but
until we figure out whats happening with the Initialize() parameter, it's the
best I can come up with.

+        &CdmWrapper::DelayedGenerateKeyRequest, key_system, type, init_data);
+    return;
+  }
+#endif
+
   if (!cdm_) {
     if (!CreateCdmInstance(key_system)) {
       SendUnknownKeyError(key_system, std::string());
@@ -938,7 +1003,7 @@ void CdmWrapper::SendKeyError(const char* session_id,
 }
 
 void CdmWrapper::GetPrivateData(int32_t* instance,
-                                cdm::Host::GetPrivateInterface* get_interface) {
+                                GetPrivateInterface* get_interface) {
   *instance = pp_instance();
   *get_interface = pp::Module::Get()->get_browser_interface();
 }
@@ -1157,15 +1222,99 @@ bool CdmWrapper::IsValidVideoFrame(const LinkedVideoFrame& video_frame) {
   return true;
 }
 
+bool CdmWrapper::CanChallengePlatform() {
+  return can_challenge_platform_;
+}
+
+void CdmWrapper::SendPlatformChallenge(
+    const char* service_id, int32_t service_id_length,
+    const char* challenge, int32_t challenge_length) {
+#if defined(OS_CHROMEOS)
+  // TODO(dalecurtis): Reinitialize member variables to PP_Undefined?
+
+  pp::VarArrayBuffer challenge_var(challenge_length);
+  uint8_t* var_data = static_cast<uint8_t*>(challenge_var.Map());
+  memcpy(var_data, challenge, challenge_length);
+
+  std::string service_id_str(service_id, service_id_length);
+  int32_t result = platform_verification_.ChallengePlatform(
+      pp::Var(service_id_str), challenge_var, &signed_data_output_,
+      &signed_data_signature_output_, &platform_key_certificate_output_,

[ddorwin, 2013/09/17 05:26:46]

Why are these passed as pointers? Aren't all pp::Vars modifiable?

[DaleCurtis, 2013/09/17 18:49:33]

[out] type variables end up as Var* per the PPAPI generator, so I'm just going
with the flow.  dmichael@ can probably elaborate.

[dmichael (off chromium), 2013/09/17 20:10:46]

ddorwin: I'm not sure what you mean...  we try to follow Chromium/Google style
as much as possible. So we use pointers for out-params, so we can modify the
members. In this case, the Vars will be set to VarArrayBuffers. We have to be
able to set the "type" and the "id" of the vars.

+      callback_factory_.NewCallback(&CdmWrapper::SendPlatformChallengeDone));
+  challenge_var.Unmap();
+#else
+  PlatformChallengeResponse pcr = {};

[ddorwin, 2013/09/17 05:26:46]

suggest: s/pcr/response/

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+  cdm_->OnPlatformChallengeResponse(pcr);
+#endif
+}
+
+#if defined(OS_CHROMEOS)
+void CdmWrapper::CanChallengePlatformDone(int32_t result,
+                                          bool can_challenge_platform) {
+  PP_DCHECK(!is_can_challenge_platform_set_);
+  can_challenge_platform_ = (result == PP_OK) ? can_challenge_platform : false;
+  is_can_challenge_platform_set_ = true;
+  if (!delayed_gkr_.IsOptional())

[ddorwin, 2013/09/17 05:26:46]

? Is the gkr ever optional?

[DaleCurtis, 2013/09/17 18:49:33]

I'm just using it to check if the callback is uninitialized.  In this case it's
equivalent to is_null().

[DaleCurtis, 2013/09/23 18:30:57]

Removed since now I can just store the key system.

+    delayed_gkr_.RunAndClear(PP_OK);
+}
+
+void CdmWrapper::SendPlatformChallengeDone(int32_t result) {
+  if (result != PP_OK) {
+    cdm::PlatformChallengeResponse pcr = {};

[ddorwin, 2013/09/17 05:26:46]

same

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+    cdm_->OnPlatformChallengeResponse(pcr);
+    return;
+  }
+
+  pp::VarArrayBuffer signed_data_var(signed_data_output_);
+  pp::VarArrayBuffer signed_data_signature_var(signed_data_signature_output_);

[dmichael (off chromium), 2013/09/17 20:10:46]

signed_data_output_ = pp::Var();
signed_data_signature_output_ = pp::Var();
?

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+  std::string pkc_string = platform_key_certificate_output_.AsString();

[ddorwin, 2013/09/17 05:26:46]

nit: "pkc" is not descriptive

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+
+  cdm::PlatformChallengeResponse pcr = {
+    static_cast<uint8_t*>(signed_data_var.Map()),
+    signed_data_var.ByteLength(),
+
+    static_cast<uint8_t*>(signed_data_signature_var.Map()),
+    signed_data_signature_var.ByteLength(),
+
+    reinterpret_cast<const uint8_t*>(pkc_string.c_str()),

[dmichael (off chromium), 2013/09/17 20:10:46]

static_cast?

[DaleCurtis, 2013/09/17 20:18:51]

Requires reinterpret_cast for char* -> uint8_t*

[Rintaro Kuroiwa, 2013/09/20 01:55:06]

Also need const_cast. The struct needs a non-const pointer.

[DaleCurtis, 2013/09/23 18:30:57]

I have another patch which makes these fields const in CDM.h. 

+    pkc_string.length()
+  };
+  cdm_->OnPlatformChallengeResponse(pcr);
+
+  signed_data_var.Unmap();
+  signed_data_signature_var.Unmap();
+}
+
+void CdmWrapper::DelayedGenerateKeyRequest(int32_t result,

[ddorwin, 2013/09/17 05:26:46]

nit: The function name is a noun not an action

[DaleCurtis, 2013/09/23 18:30:57]

Removed.

+                                           const std::string& key_system,
+                                           const std::string& type,
+                                           pp::VarArrayBuffer init_data) {

[dmichael (off chromium), 2013/09/17 20:10:46]

It's probably best to pass Var by const-reference, to avoid reference counting
(which has to acquire a lock on both the AddRef and the Release).

[DaleCurtis, 2013/09/23 18:30:57]

Removed.

+  GenerateKeyRequest(key_system, type, init_data);
+}
+#endif
+
+void CdmWrapper::EnableOutputProtection(uint32_t desired_protection_mask) {
+  // TODO(dalecurtis): Add implementation once PPAPI has landed.
+  cdm_->OnQueryOutputProtectionStatus(0, 0);
+}
+
+void CdmWrapper::QueryOutputProtectionStatus() {
+  // TODO(dalecurtis): Add implementation once PPAPI has landed.
+  cdm_->OnQueryOutputProtectionStatus(0, 0);
+}
+
 void* GetCdmHost(int host_interface_version, void* user_data) {
   if (!host_interface_version || !user_data)
     return NULL;
 
-  if (host_interface_version != cdm::kHostInterfaceVersion)
-    return NULL;
-
-  CdmWrapper* cdm_wrapper = static_cast<CdmWrapper*>(user_data);
-  return static_cast<cdm::Host*>(cdm_wrapper);
+  switch (host_interface_version) {
+    case cdm::kHostInterfaceVersion_1:
+      return static_cast<cdm::Host_1*>(static_cast<CdmWrapper*>(user_data));

[ddorwin, 2013/09/17 05:26:46]

tiny optional suggestion: It's an extra line but might be clearer to do the
CdmWrapper cast on a separate line.

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+    case cdm::kHostInterfaceVersion_2:
+      return static_cast<cdm::Host_2*>(static_cast<CdmWrapper*>(user_data));
+    default:
+      return NULL;

[ddorwin, 2013/09/17 05:26:46]

NOTREACHED()

[DaleCurtis, 2013/09/23 18:30:57]

Done.

+  }
 }
 
 // This object is the global object representing this plugin library as long