Update PluginInstance for decrypt-and-decode video.

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 e6d0eeeef7bc34e270c52e5f11db130db19aee18..d266fc0b6f74875628e7b583a455e3a7a7710ec4 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -5,6 +5,7 @@
 #include "webkit/plugins/ppapi/ppapi_plugin_instance.h"
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/memory/linked_ptr.h"
@@ -16,6 +17,7 @@
 #include "base/utf_string_conversions.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/decryptor_client.h"
+#include "media/base/video_util.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"
@@ -454,7 +456,8 @@ PluginInstance::PluginInstance(
       pending_user_gesture_(0.0),
       flash_impl_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       decryptor_client_(NULL),
-      next_decryption_request_id_(1) {
+      next_decryption_request_id_(1),
+      pending_video_decode_request_id_(0) {

[ddorwin, 2012/10/08 19:05:57]

Why not avoid 0 like we do above?

[xhwang, 2012/10/08 20:50:51]

I'd like it to be initialized to an invalid request ID.

[ddorwin, 2012/10/09 02:10:48]

We should try to initialize these in a consistent way.

[xhwang, 2012/10/09 22:29:32]

As discussed offline, we need the pending_video_decode_request_id to be invalid
because there's no pending request yet.

   pp_instance_ = HostGlobals::Get()->AddInstance(this);
 
   memset(&current_print_settings_, 0, sizeof(current_print_settings_));
@@ -1527,6 +1530,8 @@ bool PluginInstance::Decrypt(
     return false;
 
   uint32_t request_id = next_decryption_request_id_++;
+  if (next_decryption_request_id_ == 0)

[ddorwin, 2012/10/08 19:05:57]

What is the purpose of this? Fixing wrapping?

[xhwang, 2012/10/08 20:50:51]

So that the request_id can never be invalid. Please see DeliverFrame how the
pending request id is cleared when there's no pending callback.

[ddorwin, 2012/10/09 02:10:48]

DeliverFrame only deals with pending_video_decode_request_id_, not this value.

[xhwang, 2012/10/09 22:29:32]

Removed this as it's not possible to hit.

+    next_decryption_request_id_++;
 
   PP_EncryptedBlockInfo block_info;
   DCHECK(encrypted_buffer->GetDecryptConfig());
@@ -1549,7 +1554,7 @@ bool PluginInstance::Decrypt(
 // Note: this method can be used with an unencrypted frame.
 bool PluginInstance::DecryptAndDecodeFrame(
     const scoped_refptr<media::DecoderBuffer>& encrypted_frame,
-    const media::Decryptor::DecryptCB& decrypt_cb) {
+    const media::Decryptor::VideoDecodeCB& video_decode_cb) {
   if (!LoadContentDecryptorInterface())
     return false;
 
@@ -1560,7 +1565,9 @@ bool PluginInstance::DecryptAndDecodeFrame(
   if (!encrypted_resource.get())
     return false;
 
-  const uint32_t request_id = next_decryption_request_id_++;
+  uint32_t request_id = next_decryption_request_id_++;
+  if (next_decryption_request_id_ == 0)
+    next_decryption_request_id_++;
 
   // TODO(tomfinegan): Need to get the video format information here somehow.
   PP_EncryptedVideoFrameInfo frame_info;
@@ -1577,8 +1584,12 @@ bool PluginInstance::DecryptAndDecodeFrame(
     return false;
   }
 
-  DCHECK(!ContainsKey(pending_decryption_cbs_, request_id));
-  pending_decryption_cbs_.insert(std::make_pair(request_id, decrypt_cb));
+  // Only one pending video decode request at any time. This is enforced by the
+  // media pipeline.
+  DCHECK_EQ(pending_video_decode_request_id_, 0u);
+  DCHECK(pending_video_decode_cb_.is_null());
+  pending_video_decode_request_id_ = request_id;

[ddorwin, 2012/10/08 19:05:57]

What purpose do these IDs serve? For decrypt, it was to coordinate the audio and
video requests, but I'm not sure that the decode requests need them. (We may
even want to get rid of them for decrypt since it's just A or V.
Another option (and related to my comment in the header) is to have a flag for
audio and a flag for video and only allow decrypt or decode once for each. This
requires adding DecryptAudio/Video.

[xhwang, 2012/10/08 20:50:51]

As discussed offline and replied here
(http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...),
the request id will be used for tracking callbacks so that we can cancel 
operations without waiting.

[ddorwin, 2012/10/09 02:10:48]

As discussed offline (and in fischman's comment), we should eventually eliminate
this. Add TODO to header or somewhere.

[xhwang, 2012/10/09 22:29:32]

Comments added in header file.

+  pending_video_decode_cb_ = video_decode_cb;
 
   plugin_decryption_interface_->DecryptAndDecodeFrame(pp_instance(),
                                                       encrypted_resource,
@@ -2294,6 +2305,7 @@ void PluginInstance::DeliverBlock(PP_Instance instance,
     decrypt_cb.Run(media::Decryptor::kNoKey, NULL);
     return;
   }
+
   if (block_info->result != PP_DECRYPTRESULT_SUCCESS) {
     decrypt_cb.Run(media::Decryptor::kError, NULL);
     return;
@@ -2323,8 +2335,70 @@ void PluginInstance::DeliverBlock(PP_Instance instance,
 void PluginInstance::DeliverFrame(PP_Instance instance,
                                   PP_Resource decrypted_frame,
                                   const PP_DecryptedFrameInfo* frame_info) {
-  // TODO(tomfinegan): To be implemented after completion of v0.1 of the
-  // EME/CDM work.
+  DCHECK(frame_info);
+  uint32_t request_id = frame_info->tracking_info.request_id;
+
+  // If the request ID is not valid or does not match what's saved, do nothing.
+  if (request_id == 0 || request_id != pending_video_decode_request_id_)
+    return;
+
+  DCHECK(!pending_video_decode_cb_.is_null());
+  pending_video_decode_request_id_ = 0;
+  media::Decryptor::VideoDecodeCB video_decode_cb =
+      base::ResetAndReturn(&pending_video_decode_cb_);
+
+  if (frame_info->result == PP_DECRYPTRESULT_DECRYPT_NOKEY) {
+    video_decode_cb.Run(media::Decryptor::kNoKey, NULL);
+    return;
+  }
+
+  if (frame_info->result != PP_DECRYPTRESULT_SUCCESS) {
+    video_decode_cb.Run(media::Decryptor::kError, NULL);
+    return;
+  }
+
+  EnterResourceNoLock<PPB_Buffer_API> enter(decrypted_frame, true);
+  if (!enter.succeeded()) {
+    video_decode_cb.Run(media::Decryptor::kError, NULL);
+    return;
+  }
+  BufferAutoMapper mapper(enter.object());
+  if (!mapper.data() || !mapper.size()) {
+    video_decode_cb.Run(media::Decryptor::kError, NULL);
+    return;
+  }
+
+  const uint8* frame_data = reinterpret_cast<uint8*>(mapper.data());
+
+  // TODO(tomfinegan): Find a way to take ownership of the shared memory
+  // managed by the PPB_Buffer_Dev, and avoid the extra copy.
+  DCHECK(frame_info->format == PP_DECRYPTEDFRAMEFORMAT_YV12);

[xhwang, 2012/10/08 17:50:36]

+tomf: can we control what the output format is? Or do we need to support
multiple formats here?

[Tom Finegan, 2012/10/09 06:25:34]

Since we support only VP8, and it outputs only I420 and YV12, I think it would
be best to handle swapping the plane offsets here if YV12 is required.

[xhwang, 2012/10/09 22:29:32]

I am asking because VideoFrame::CreateFrame only supports RGB32/YV12 right now.
Since VP8 supports YV12 we are good here (by enforcing using YV12 by the vp8
decoder)?

+  gfx::Size frame_size(frame_info->width, frame_info->height);
+  scoped_refptr<media::VideoFrame> decoded_frame(

[ddorwin, 2012/10/08 19:05:57]

I hate to add all this media code to ppapi code. Can we just pass some raw ptrs
in the callback and require the callback to copy them by stating the lifetime
may end after the callback returns?
Longer term, we need to solve tomfinegan's TODO above? How hard would it be to
create an interface whose impl holds the PPB_Buffer_Dev and frees it when the
interface is deleted/released?

brettw@: Any thoughts on retaining the shared memory behind the PPB_Buffer_Dev?

[xhwang, 2012/10/08 20:50:51]

Agreed. For now the callback will get posted onto the decoder's thread and
executed asynchronously. I can modify the PpapiDecryptor to do this. 

Actually if it's just adding too much code into this class/file, I think we have
too many methods added to the PluginInstance already. dmichael@ mentioned that
we need to have some delegate to do this for PluginInstance. Any concrete idea
how to do this? Is there any example to follow?

[ddorwin, 2012/10/09 02:10:48]

I think that's a separate issue, but we should avoid adding these media calls
here. We at least need a TODO to eliminate the video_util.h include. This is
worse than just lots of code IMO.

[xhwang, 2012/10/09 22:29:32]

TODO added before #include "media/base/video_util.h"

+      media::VideoFrame::CreateFrame(
+          media::VideoFrame::YV12, frame_size, frame_size,
+          base::TimeDelta::FromMicroseconds(
+              frame_info->tracking_info.timestamp)));
+
+  media::CopyYPlane(
+      frame_data + frame_info->plane_offsets[PP_DECRYPTEDFRAMEPLANES_Y],
+      frame_info->strides[PP_DECRYPTEDFRAMEPLANES_Y],
+      frame_info->height,
+      decoded_frame.get());
+
+  media::CopyUPlane(
+      frame_data + frame_info->plane_offsets[PP_DECRYPTEDFRAMEPLANES_U],
+      frame_info->strides[PP_DECRYPTEDFRAMEPLANES_U],
+      frame_info->height,
+      decoded_frame.get());
+
+  media::CopyVPlane(
+      frame_data + frame_info->plane_offsets[PP_DECRYPTEDFRAMEPLANES_V],
+      frame_info->strides[PP_DECRYPTEDFRAMEPLANES_V],
+      frame_info->height,
+      decoded_frame.get());
+
+  video_decode_cb.Run(media::Decryptor::kSuccess, decoded_frame);
 }
 
 void PluginInstance::DeliverSamples(PP_Instance instance,