Connect PpapiDecryptor and PluginInstance.

webkit/media/crypto/ppapi_decryptor.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "webkit/media/crypto/ppapi_decryptor.h"
 
+#include <string>
+
+#include "base/bind.h"
+#include "base/location.h"

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

What is this used for?

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

For FROM_HERE in PostTask.

 #include "base/logging.h"
+#include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
+#include "content/public/renderer/render_thread.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/decryptor_client.h"
 #include "webkit/media/crypto/key_systems.h"
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 
 namespace webkit_media {
 
 PpapiDecryptor::PpapiDecryptor(
   media::DecryptorClient* client,
   const scoped_refptr<webkit::ppapi::PluginInstance>& plugin_instance)
     : client_(client),
-      cdm_plugin_(plugin_instance) {
+      cdm_plugin_(plugin_instance),
+      render_loop_proxy_(content::RenderThread::Get()->GetMessageLoop()->
+          message_loop_proxy()) {
+  DCHECK(client_);
+  DCHECK(cdm_plugin_);
+  cdm_plugin_->SetDecryptClient(client);
 }
 
 PpapiDecryptor::~PpapiDecryptor() {
 }
 
 void PpapiDecryptor::GenerateKeyRequest(const std::string& key_system,
                                         const uint8* init_data,
                                         int init_data_length) {
+  DVLOG(1) << "GenerateKeyRequest()";

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

Probably worth providing context (PPAPI) since AesDecryptor or WMPI might have
similar logging.

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

In the logging, it will always show the file name so I thought function name
here would be clear enough. An example in media code:
https://cs.corp.google.com/#chrome/src/webkit/media/webmediaplayer_impl.cc&q=...

But I can change for sure if the convention is otherwise.

+  DCHECK(render_loop_proxy_->BelongsToCurrentThread());

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

It's not obvious why this needs to be the case in this class. It may be true,
but do we need to check it? Are these checks redundant with other checks?

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

I guess this is just another (better) way to comment that this call should
always be called on the renderer thread. If we don't DCHECK here, we'd need to
comment it.

   DCHECK(cdm_plugin_);
-  // TODO(xhwang): Enable the following once we have updated PluginInstance.
-  // if (!cdm_plugin_->GenerateKeyRequest(key_system,
-  //                                      init_data, init_data_length)) {
-  //   client_->KeyError(key_system, "", media::Decryptor::kUnknownError, 0);
-  // }
+
+  if (!cdm_plugin_->GenerateKeyRequest(
+      key_system,
+      std::string(reinterpret_cast<const char*>(init_data),

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

It's unfortunate we have to do this cast in multiple files. Should we just do it
in WMPI?

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

It's not really clear to me why we pass a string in to PluginInstance anyway.
Aren't you forcing an unnecessary copy? Why not just pass the same
pointer+length?

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

We discussed this before, and decided to use:
- string for key, iv, key_id etc since they are short (and copying a short
string doesn't hurt performance). 
- pointer/size for initdata, key message, or any other large binary data.

This is why we have mixed type, e.g. in KeyMessage and NeedKey:
https://cs.corp.google.com/#chrome/src/webkit/media/webmediaplayer_proxy.cc&t...

But really, how big could initdata or key message be?

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

Yes, if we decided to use pointer/size for init_data, we might just pass
pointer/size to PluginInstance (PI) also. There're some inconsistency here. But
we need to decide on what data to use in media pipeline first.

[dmichael (off chromium), 2012/08/24 17:18:34]

The performance wouldn't bother me if the code was cleaner this way, but it
seems odd to convert from pointer+size to a string, then back to pointer+size,
with associated reinterpret_casts, if there's no benefit to readability or
performance. Even a vector of uint8_t would seem better to me, since it better
conveys that we're talking about binary data.

Ultimately, I suspect you'll want some kind of ArrayBuffer or ArrayBufferView
all the way through the stack. You can leave it this way if you think it's
appropriate, but maybe a TODO would be good, if you're still figuring out what
type it should be.

+                  init_data_length))) {
+    client_->KeyError(key_system, "", kUnknownError, 0);

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

nit: one line statement in the if, not sure about media code view on whether
curly braces are needed here or not.

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

You might move this to a helper function and log "failed calling plugin" since
there is no other way to distinguish this failure.

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

Done.

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

I remember we should use curly braces when the if() statement takes more than
one line... but not 100% sure either.

+  }
 }
 
 void PpapiDecryptor::AddKey(const std::string& key_system,
                             const uint8* key,
                             int key_length,
                             const uint8* init_data,
                             int init_data_length,
                             const std::string& session_id) {
+  DVLOG(1) << "AddKey()";
+  DCHECK(render_loop_proxy_->BelongsToCurrentThread());
   DCHECK(cdm_plugin_);
-  // TODO(xhwang): Enable the following once we have updated PluginInstance.
-  // if (!cdm_plugin_->AddKey(key_system, key, key_length,
-  //                          init_data, init_data_length, session_id)) {
-  //   client_->KeyError(key_system, session_id, Decryptor::kUnknownError, 0);
-  // }
+
+  if (!cdm_plugin_->AddKey(
+      session_id,

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

nit: It seems this would look better if indented further.

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

Agreed...  I think it should be 4 from the open paren on 62

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

Done. But not sure if this is what you meant.

+      std::string(reinterpret_cast<const char*>(key), key_length),
+      std::string(reinterpret_cast<const char*>(init_data),
+                  init_data_length))) {
+    client_->KeyError(key_system, session_id, kUnknownError, 0);

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

Does client always post to ensure we never report events on the same callstack
as the WebKit call? I don't remember where we do that.

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

Yes, the client is the webmediaplayer_proxy, which will always post:
https://cs.corp.google.com/#chrome/src/webkit/media/webmediaplayer_proxy.cc&t...

+  }
 }
 
 void PpapiDecryptor::CancelKeyRequest(const std::string& key_system,
                                       const std::string& session_id) {
+  DVLOG(1) << "CancelKeyRequest()";
+  DCHECK(render_loop_proxy_->BelongsToCurrentThread());
   DCHECK(cdm_plugin_);
-  // TODO(xhwang): Enable the following once we have updated PluginInstance.
-  // if (!cdm_plugin_->CancelKeyRequest(key_system, session_id))
-  //   client_->KeyError(key_system, session_id, Decryptor::kUnknownError, 0);
+
+  if (!cdm_plugin_->CancelKeyRequest(session_id))
+    client_->KeyError(key_system, session_id, kUnknownError, 0);
 }
 
 void PpapiDecryptor::Decrypt(
     const scoped_refptr<media::DecoderBuffer>& encrypted,

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

Is it possible to avoid refptr for this?

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

I think the rule is if |encrypted| is a scoped_refptr, it should always be
passed as a scoped_refptr, not, e.g., a raw pointer.

     const DecryptCB& decrypt_cb) {
-  DCHECK(cdm_plugin_);
-  // TODO(xhwang): Enable the following once we have updated PluginInstance.
-  // TODO(xhwang): Need to figure out thread safety about PPP calls.
-  // if (!cdm_plugin_->Decrypt(
-  //     encrypted, base::Bind(&PpapiDecryptor::DataReady, this, decrypt_cb))) {
-  //   decrypt_cb.Run(kError, NULL);
-  // }
+  DVLOG(1) << "Decrypt()";
+  if (!render_loop_proxy_->BelongsToCurrentThread()) {
+    render_loop_proxy_->PostTask(
+        FROM_HERE,
+        base::Bind(&PpapiDecryptor::Decrypt, base::Unretained(this),
+                   encrypted, decrypt_cb));
+    return;
+  }
+
+  if (cdm_plugin_->Decrypt(encrypted, decrypt_cb)) {

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

Missing a '!' ?

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

Oops, thanks for catching this. Done.

+    decrypt_cb.Run(kError, NULL);
+  }
 }
 
 void PpapiDecryptor::Stop() {
 }
 
-void PpapiDecryptor::DataReady(const DecryptCB& decrypt_cb,
-                               const uint8* data, int data_size ) {
-  DCHECK(!decrypt_cb.is_null());
-  scoped_refptr<media::DecoderBuffer> decrypted_data =
-      media::DecoderBuffer::CopyFrom(data, data_size);
-  decrypt_cb.Run(kSuccess, decrypted_data);
-}
-
 }  // namespace webkit_media