Connect PpapiDecryptor and PluginInstance.

webkit/plugins/ppapi/ppapi_plugin_instance.cc

Index: webkit/plugins/ppapi/ppapi_plugin_instance.cc
diff --git a/webkit/plugins/ppapi/ppapi_plugin_instance.cc b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
index 11378904d637a7e86fd18a7db0a6c42dd43b9cca..ce7f7cf7eb3e0a3b574aa883a0bf024162137f44 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -9,10 +9,13 @@
 #include "base/logging.h"
 #include "base/memory/linked_ptr.h"
 #include "base/message_loop.h"
+#include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/utf_offset_string_conversions.h"
 #include "base/utf_string_conversions.h"
+#include "media/base/decoder_buffer.h"
+#include "media/base/decryptor_client.h"
 #include "ppapi/c/dev/ppb_find_dev.h"
 #include "ppapi/c/dev/ppb_zoom_dev.h"
 #include "ppapi/c/dev/ppp_find_dev.h"
@@ -301,11 +304,11 @@ scoped_array<const char*> StringVectorToArgArray(
 // of 0 on failure. Upon success, the returned Buffer resource has a reference
 // count of 1.
 PP_Resource MakeBufferResource(PP_Instance instance,
-                               const base::StringPiece& data) {
-  if (data.empty())
+                               const uint8* data, int size) {

[Tom Finegan, 2012/08/22 01:05:47]

nit: seems to be consistently one arg per line in here, but maybe type* and size
of pointer on the same line is fine in this code. (I assume that's the case in
media code, now that I've seen this CL :))

[dmichael (off chromium), 2012/08/22 20:30:15]

We usually do 1-per-line, but since those two are so closely related, this makes
sense to me as-is.

[xhwang, 2012/08/24 00:51:51]

Yeah, this is actually in chromium code style guide:
// Only legal when |second_argument| and |third_argument| are intimately
// related, e.g. (x, y) coords.
void SpecialCase(TypeA first_argument,
                 TypeB second_argument, TypeC third_argument,
                 TypeD fourth_argument);

+  if (!data || size == 0)
     return 0;
 
-  ScopedPPResource resource(PPB_Buffer_Impl::Create(instance, data.size()));
+  ScopedPPResource resource(PPB_Buffer_Impl::Create(instance, size));
   if (!resource.get())
     return 0;
 
@@ -314,8 +317,10 @@ PP_Resource MakeBufferResource(PP_Instance instance,
     return 0;
 
   BufferAutoMapper mapper(enter.object());
-  memcpy(mapper.data(), data.data(), data.size());
+  if (!mapper.data() || mapper.size() < static_cast<size_t>(size))
+    return 0;
 
+  memcpy(mapper.data(), data, size);
   return resource.get();
 }
 
@@ -377,7 +382,9 @@ PluginInstance::PluginInstance(
       selection_caret_(0),
       selection_anchor_(0),
       pending_user_gesture_(0.0),
-      flash_impl_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      flash_impl_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      decryptor_client_(NULL),
+      next_decryption_request_id_(0) {

[ddorwin, 2012/08/22 01:26:26]

Should we avoid using 0?

[xhwang, 2012/08/24 00:51:51]

Done.

[dmichael (off chromium), 2012/08/27 20:14:48]

Don't know if you care, but 0 is still possible on roll-over. Not sure how many
bundles you expect per second, but I expect roll-over is going to be rare at
best.

[xhwang, 2012/08/27 22:58:14]

Session ID is not likely to roll-over at all. Not using 0 here so that an
uninitialized variable (likely to be zero) can't coincidentally matches a valid
session ID. 

   pp_instance_ = HostGlobals::Get()->AddInstance(this);
 
   memset(&current_print_settings_, 0, sizeof(current_print_settings_));
@@ -1318,12 +1325,19 @@ void PluginInstance::RotateView(WebPlugin::RotationType type) {
   // NOTE: plugin instance may have been deleted.
 }
 
+void PluginInstance::SetDecryptClient(

[ddorwin, 2012/08/22 04:09:21]

Why not set_decryptor_client()?

[xhwang, 2012/08/24 00:51:51]

Done.

+    media::DecryptorClient* decryptor_client) {
+  DCHECK(decryptor_client);
+  decryptor_client_ = decryptor_client;

[ddorwin, 2012/08/22 01:26:26]

Does this class assume it's only called on the main thread?

[dmichael (off chromium), 2012/08/22 20:30:15]

Yes

+}
+
 bool PluginInstance::GenerateKeyRequest(const std::string& key_system,
                                         const std::string& init_data) {
   if (!LoadContentDecryptorInterface())
     return false;
   if (key_system.empty())
     return false;
+
   PP_Var init_data_array =
       PpapiGlobals::Get()->GetVarTracker()->MakeArrayBufferPPVar(
           init_data.size(), init_data.data());
@@ -1363,29 +1377,88 @@ bool PluginInstance::CancelKeyRequest(const std::string& session_id) {
       StringVar::StringToPPVar(session_id)));
 }
 
-bool PluginInstance::Decrypt(const base::StringPiece& encrypted_block,
-                             const DecryptedDataCB& callback) {
+namespace {
+

[Tom Finegan, 2012/08/22 01:05:47]

This probably belongs in the anonymous namespace w/MakeBufferResource.

[ddorwin, 2012/08/22 01:26:26]

Right, anonymous namespaces should not be in the middle of a file.

[xhwang, 2012/08/24 00:51:51]

Done.

[xhwang, 2012/08/24 00:51:51]

Done.

+template <uint32_t array_size>
+void CopyStringToArray(const std::string& str,
+                       uint8 (&array)[array_size], uint32_t* data_size) {
+  DCHECK_LE(str.size(), array_size);

[ddorwin, 2012/08/22 04:09:21]

CHECK_LE? Crash rather than overrun.

[xhwang, 2012/08/24 00:51:51]

Done.

+  DCHECK(data_size);
+  memcpy(array, str.data(), str.size());
+  *data_size = str.size();
+}
+
+void MakePPEncryptedBlockInfo(

[Tom Finegan, 2012/08/22 01:05:47]

s/MakePPEncryptedBlockInfo/MakeEncryptedBlockInfo/ ?

More consistent w/MakeBufferResource, but I'm not sure how great a name that was
to begin with. :)

[xhwang, 2012/08/24 00:51:51]

Done.

+    const media::DecryptConfig& decrypt_config,
+    int64_t timestamp,
+    uint64_t request_id,
+    PP_EncryptedBlockInfo* block_info) {
+  DCHECK(block_info);
+
+  block_info->tracking_info.request_id = request_id;
+  block_info->tracking_info.timestamp = timestamp;
+  block_info->data_offset = decrypt_config.data_offset();
+
+  CopyStringToArray(decrypt_config.key_id(),

[ddorwin, 2012/08/22 04:09:21]

Just looking at the parameters here, the implementation does not do anything
like I would expect. IOW, it's a weird signature.

[xhwang, 2012/08/24 00:51:51]

Done.

+                    block_info->key_id, &block_info->key_id_size);
+  CopyStringToArray(decrypt_config.iv(), block_info->iv, &block_info->iv_size);
+  CopyStringToArray(decrypt_config.checksum(),
+                    block_info->checksum, &block_info->checksum_size);
+
+  block_info->num_subsamples = decrypt_config.subsamples().size();
+  CHECK(block_info->num_subsamples < arraysize(block_info->subsamples));

[ddorwin, 2012/08/22 04:09:21]

Do we check nicely somewhere else? We shouldn't crash due to user/page input.

[xhwang, 2012/08/24 00:51:51]

Done.

+  for (uint32_t i = 0; i < block_info->num_subsamples; ++i) {
+    block_info->subsamples[i].clear_bytes =
+        decrypt_config.subsamples()[i].clear_bytes;
+    block_info->subsamples[i].cipher_bytes =
+        decrypt_config.subsamples()[i].cypher_bytes;
+  }
+}
+
+}  // namespace
+
+bool PluginInstance::Decrypt(
+    const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
+    const media::Decryptor::DecryptCB& decrypt_cb) {

[dmichael (off chromium), 2012/08/22 20:30:15]

Does it still make sense to have a callback instead of just talking to the
decrypt_client_?

[xhwang, 2012/08/24 00:51:51]

Unfortunately, decrypt_client_ connects the decryptor to the WebKit, and this
decrypt_cb connects the decryptor back to the media pipeline. So we still need a
separate cb here.

   if (!LoadContentDecryptorInterface())
     return false;
-  ScopedPPResource encrypted_resource(MakeBufferResource(pp_instance(),
-                                                         encrypted_block));
+
+  ScopedPPResource encrypted_resource(MakeBufferResource(
+      pp_instance(),
+      encrypted_buffer->GetData(),
+      encrypted_buffer->GetDataSize()));
   if (!encrypted_resource.get())
     return false;
 
+  uint64_t request_id = next_decryption_request_id_++;
+
   PP_EncryptedBlockInfo block_info;
+  memset(&block_info, 0, sizeof(block_info));

[ddorwin, 2012/08/22 04:09:21]

TODO - fix initialization here and in Tom's CL.

[xhwang, 2012/08/24 00:51:51]

Done.

+
+  DCHECK(encrypted_buffer->GetDecryptConfig());
+  MakePPEncryptedBlockInfo(*encrypted_buffer->GetDecryptConfig(),
+                           encrypted_buffer->GetTimestamp().InMicroseconds(),
+                           request_id,
+                           &block_info);

[Tom Finegan, 2012/08/22 01:05:47]

Wish there was a way to avoid this copy...

[xhwang, 2012/08/24 00:51:51]

Yeah, it's ugly, fortunately we are not actually copying too much data.

+
+  DCHECK(!ContainsKey(pending_decryption_cbs_, request_id));
+  pending_decryption_cbs_.insert(std::make_pair(request_id, decrypt_cb));
 
-  // TODO(tomfinegan): Store callback and ID in a map, and pass ID to decryptor.
   return PP_ToBool(plugin_decryption_interface_->Decrypt(pp_instance(),
                                                          encrypted_resource,
                                                          &block_info));
 }
 
-bool PluginInstance::DecryptAndDecode(const base::StringPiece& encrypted_block,
-                                      const DecryptedDataCB& callback) {
+bool PluginInstance::DecryptAndDecode(
+    const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
+    const media::Decryptor::DecryptCB& decrypt_cb) {
   if (!LoadContentDecryptorInterface())
     return false;
-  ScopedPPResource encrypted_resource(MakeBufferResource(pp_instance(),
-                                                         encrypted_block));
+
+  ScopedPPResource encrypted_resource(MakeBufferResource(
+      pp_instance(),
+      encrypted_buffer->GetData(),
+      encrypted_buffer->GetDataSize()));
   if (!encrypted_resource.get())
     return false;
 
@@ -2016,7 +2089,12 @@ void PluginInstance::NeedKey(PP_Instance instance,
 void PluginInstance::KeyAdded(PP_Instance instance,
                               PP_Var key_system_var,
                               PP_Var session_id_var) {
-  // TODO(tomfinegan): send the data to media stack.
+  StringVar* key_system_string = StringVar::FromPPVar(key_system_var);
+  StringVar* session_id_string = StringVar::FromPPVar(session_id_var);
+
+  DCHECK(decryptor_client_);
+  decryptor_client_->KeyAdded(key_system_string->value(),
+                              session_id_string->value());
 }
 
 void PluginInstance::KeyMessage(PP_Instance instance,
@@ -2024,7 +2102,29 @@ void PluginInstance::KeyMessage(PP_Instance instance,
                                 PP_Var session_id_var,
                                 PP_Resource message_resource,
                                 PP_Var default_url_var) {
-  // TODO(tomfinegan): send the data to media stack.
+  StringVar* key_system_string = StringVar::FromPPVar(key_system_var);
+  StringVar* session_id_string = StringVar::FromPPVar(session_id_var);
+  StringVar* default_url_string = StringVar::FromPPVar(default_url_var);

[dmichael (off chromium), 2012/08/22 20:30:15]

You have to check these pointers before calling value(). You shouldn't trust
here that the PP_Var is a valid string var. (same elsewhere)

[xhwang, 2012/08/24 00:51:51]

Done.

+
+  EnterResourceNoLock<PPB_Buffer_API> enter(message_resource, true);
+  if (!enter.succeeded()) {
+    decryptor_client_->KeyError(key_system_string->value(),
+                                session_id_string->value(),
+                                media::Decryptor::kUnknownError,
+                                0);
+  }
+
+  BufferAutoMapper mapper(enter.object());
+  scoped_array<uint8> message_array(new uint8[mapper.size()]);
+  if (mapper.data() || mapper.size())

[ddorwin, 2012/08/22 04:09:21]

&& ?

[xhwang, 2012/08/24 00:51:51]

Done.

+    memcpy(message_array.get(), mapper.data(), mapper.size());
+
+  DCHECK(decryptor_client_);
+  decryptor_client_->KeyMessage(key_system_string->value(),
+                                session_id_string->value(),
+                                message_array.Pass(),
+                                mapper.size(),
+                                default_url_string->value());
 }
 
 void PluginInstance::KeyError(PP_Instance instance,
@@ -2032,13 +2132,63 @@ void PluginInstance::KeyError(PP_Instance instance,
                               PP_Var session_id_var,
                               int32_t media_error,
                               int32_t system_code) {
-  // TODO(tomfinegan): send the data to media stack.
+  StringVar* key_system_string = StringVar::FromPPVar(key_system_var);
+  StringVar* session_id_string = StringVar::FromPPVar(session_id_var);
+
+  DCHECK(decryptor_client_);
+  decryptor_client_->KeyError(
+      key_system_string->value(),
+      session_id_string->value(),
+      static_cast<media::Decryptor::KeyError>(media_error),
+      system_code);
 }
 
 void PluginInstance::DeliverBlock(PP_Instance instance,
                                   PP_Resource decrypted_block,
                                   const PP_DecryptedBlockInfo* block_info) {
-  // TODO(xhwang): Pass the decrypted block back to media stack.
+  DCHECK(block_info);
+
+  DecryptionCBMap::iterator found = pending_decryption_cbs_.find(
+      block_info->tracking_info.request_id);
+
+  if (found == pending_decryption_cbs_.end()) {
+    LOG(WARNING) << "DeliverBlock(): request_id: "
+        << block_info->tracking_info.request_id << " not found.";

[ddorwin, 2012/08/22 04:09:21]

The ID isn't really useful to anyone not debugging this code. Will a DLOG do? Is
there any reason to think this will happen?

[xhwang, 2012/08/24 00:51:51]

Suppose the CDM is implemented by a third party. They can just return us blocks
with invalid request ID. 

Removed log.

+    return;
+  }
+
+  media::Decryptor::DecryptCB decrypt_cb = found->second;
+  pending_decryption_cbs_.erase(found);
+
+  if (block_info->result == PP_DECRYPTRESULT_DECRYPT_ERROR) {
+    decrypt_cb.Run(media::Decryptor::kError, NULL);
+    return;
+  }
+
+  if (block_info->result == PP_DECRYPTRESULT_DECRYPT_NOKEY) {
+    decrypt_cb.Run(media::Decryptor::kNoKey, NULL);
+    return;
+  }
+

[dmichael (off chromium), 2012/08/22 20:30:15]

nit: It seems like there are a few excess lines here (2159, 2167, 2172 at least)

+  DCHECK_EQ(block_info->result, PP_DECRYPTRESULT_SUCCESS);

[ddorwin, 2012/08/22 04:09:21]

Is there a DECODE_ERROR yet or is that another CL?

[xhwang, 2012/08/24 00:51:51]

DeliverBlock should not handle decode_error. Changed the code a little to make
the error handling better organized.

+  EnterResourceNoLock<PPB_Buffer_API> enter(decrypted_block, true);
+  if (!enter.succeeded()) {
+    decrypt_cb.Run(media::Decryptor::kError, NULL);
+    return;
+  }
+
+  BufferAutoMapper mapper(enter.object());
+  if (mapper.data() == NULL || mapper.size() == 0) {

[ddorwin, 2012/08/22 04:09:21]

Inconsistent use of == vs. 2119.

[xhwang, 2012/08/24 00:51:51]

Done.

+    decrypt_cb.Run(media::Decryptor::kError, NULL);
+    return;
+  }
+
+  scoped_refptr<media::DecoderBuffer> decrypted_buffer(
+      media::DecoderBuffer::CopyFrom(
+          reinterpret_cast<const uint8*>(mapper.data()), mapper.size()));
+  decrypted_buffer->SetTimestamp(base::TimeDelta::FromMicroseconds(
+      block_info->tracking_info.timestamp));
+  decrypt_cb.Run(media::Decryptor::kSuccess, decrypted_buffer);
 }
 
 void PluginInstance::DeliverFrame(PP_Instance instance,
@@ -2055,7 +2205,6 @@ void PluginInstance::DeliverSamples(PP_Instance instance,
   // EME/CDM work.
 }
 
-
 void PluginInstance::NumberOfFindResultsChanged(PP_Instance instance,
                                                 int32_t total,
                                                 PP_Bool final_result) {