Update PluginInstance for audio support for content decryption.

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 0452e855e57f948a43de77a8b90572e671ad27a5..dcee8a66350e685dc7adbf86d5389577d622cbba 100644
--- a/webkit/plugins/ppapi/ppapi_plugin_instance.cc
+++ b/webkit/plugins/ppapi/ppapi_plugin_instance.cc
@@ -17,6 +17,8 @@
 #include "base/utf_string_conversions.h"
 // TODO(xhwang): Move media specific code out of this class.
 #include "media/base/audio_decoder_config.h"
+#include "media/base/channel_layout.h"
+#include "media/base/data_buffer.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/decryptor_client.h"
 #include "media/base/video_decoder_config.h"
@@ -392,6 +394,47 @@ bool MakeEncryptedBlockInfo(
   return true;
 }
 
+bool MakeAudioFrames(PP_Resource audio_frames,

[xhwang, 2012/10/19 17:07:33]

probably need comments here.

[Tom Finegan, 2012/10/19 22:53:52]

Agreed, but I don't think it needs anything too big. How about:

Deserializes audio data stored in |audio_frames| into individual audio buffers
in |frames|. Returns true upon success.

I don't know.... writing that makes me think the function should be named
DeserializeAudioFrames(). WDYT?

[ddorwin, 2012/10/20 00:14:36]

Especially since the out parameter type is "buffers". :)

[xhwang, 2012/10/21 19:05:06]

Done.

[xhwang, 2012/10/21 19:05:06]

Hmm, maybe we should rename media::Decryptor::AudioBuffers to
media::Decryptor::AudioFrames ?

[ddorwin, 2012/10/22 17:47:11]

SGTM (in another CL).

+                     media::Decryptor::AudioBuffers* frames) {
+  DCHECK(frames);
+  EnterResourceNoLock<PPB_Buffer_API> enter(audio_frames, true);
+  if (!enter.succeeded())
+    return false;
+
+  BufferAutoMapper mapper(enter.object());
+  if (!mapper.data() || !mapper.size())
+    return false;
+
+  const uint8* cur = static_cast<uint8*>(mapper.data());
+  int bytes_left = mapper.size();
+
+  do {
+    if (bytes_left < 16)

[ddorwin, 2012/10/20 00:14:36]

why?
16 is a magic number here and below. 8 too. sizeof()?

[xhwang, 2012/10/21 19:05:06]

Done.

+      return false;
+
+    int64 timestamp = *(reinterpret_cast<const int64*>(cur));
+    cur += 8;
+    int64 frame_size = *(reinterpret_cast<const int64*>(cur));
+    cur += 8;
+    bytes_left -= 16;

[ddorwin, 2012/10/20 00:14:36]

(8 + 8) would be clearer, esp. when using sizeof().

[xhwang, 2012/10/21 19:05:06]

Done.

+
+    if (frame_size < 0 || bytes_left < frame_size)

[ddorwin, 2012/10/20 00:14:36]

DCHECK? Is this a bug if we see it?

[xhwang, 2012/10/21 19:05:06]

As I said, I don't trust CDMs. The renderer should not crash if the CDM returns
garbage. WDYT?

[ddorwin, 2012/10/22 17:47:11]

A DCHECK just tells us something is wrong in a debug build. So we can
investigate. We should still have the error handling for prod builds.

+      return false;
+
+    scoped_refptr<media::DataBuffer> frame(new media::DataBuffer(frame_size));
+    if (frame_size > 0) {
+      memcpy(frame->GetWritableData(), cur, frame_size);
+      frame->SetTimestamp(base::TimeDelta::FromMicroseconds(timestamp));
+      cur += frame_size;
+      bytes_left -= frame_size;
+    }
+    frames->push_back(frame);
+  }
+  while (bytes_left > 0);

[Tom Finegan, 2012/10/19 22:53:52]

Move the while up on the same line with the closing brace for the do.

[xhwang, 2012/10/21 19:05:06]

Done.

+
+  return true;
+}
+
 PP_AudioCodec MediaAudioCodecToPpAudioCodec(media::AudioCodec codec) {
   if (codec == media::kCodecVorbis)
     return PP_AUDIOCODEC_VORBIS;
@@ -500,8 +543,11 @@ PluginInstance::PluginInstance(
       flash_impl_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
       decryptor_client_(NULL),
       next_decryption_request_id_(1),
+      pending_audio_decrypt_request_id_(0),
+      pending_video_decrypt_request_id_(0),
       pending_audio_decoder_init_request_id_(0),
       pending_video_decoder_init_request_id_(0),
+      pending_audio_decode_request_id_(0),
       pending_video_decode_request_id_(0) {
   pp_instance_ = HostGlobals::Get()->AddInstance(this);
 
@@ -1508,7 +1554,6 @@ void PluginInstance::RotateView(WebPlugin::RotationType type) {
 
 void PluginInstance::set_decrypt_client(
     media::DecryptorClient* decryptor_client) {
-  DCHECK(decryptor_client);
   decryptor_client_ = decryptor_client;
 }
 
@@ -1560,10 +1605,13 @@ bool PluginInstance::CancelKeyRequest(const std::string& session_id) {
   return true;
 }
 
+// TODO(xhwang): Remove duplication of code in Decrypt(),
+// DecryptAndDecodeAudio() and DecryptAndDecodeVideo().

[Tom Finegan, 2012/10/19 22:53:52]

We should do this when we refactor and add the delegate.

[xhwang, 2012/10/21 19:05:06]

Agreed.

 bool PluginInstance::Decrypt(
+    media::Decryptor::StreamType stream_type,
     const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
     const media::Decryptor::DecryptCB& decrypt_cb) {
-  DVLOG(3) << "Decrypt()";
+  DVLOG(3) << "Decrypt() - stream_type: " << stream_type;
   if (!LoadContentDecryptorInterface())
     return false;
 
@@ -1587,8 +1635,19 @@ bool PluginInstance::Decrypt(
     return false;
   }
 
-  DCHECK(!ContainsKey(pending_decryption_cbs_, request_id));
-  pending_decryption_cbs_.insert(std::make_pair(request_id, decrypt_cb));
+  uint32_t& pending_decrypt_request_id =

[ddorwin, 2012/10/20 00:14:36]

DCHECK stream_type,
Would a switch statement be equiv or less code and less confusing?

[xhwang, 2012/10/21 19:05:06]

Done.

+      (stream_type == media::Decryptor::kAudio) ?
+      pending_audio_decrypt_request_id_ : pending_video_decrypt_request_id_;
+  media::Decryptor::DecryptCB& pending_decrypt_cb =
+      (stream_type == media::Decryptor::kAudio) ?
+      pending_audio_decrypt_cb_ : pending_video_decrypt_cb_;
+
+  // There is only one pending decrypt request at any time per stream. This is
+  // enforced by the media pipeline.
+  DCHECK_EQ(pending_decrypt_request_id, 0u);
+  DCHECK(pending_decrypt_cb.is_null());
+  pending_decrypt_request_id = request_id;
+  pending_decrypt_cb = decrypt_cb;
 
   plugin_decryption_interface_->Decrypt(pp_instance(),
                                         encrypted_resource,
@@ -1596,6 +1655,30 @@ bool PluginInstance::Decrypt(
   return true;
 }
 
+bool PluginInstance::CancelDecrypt(media::Decryptor::StreamType stream_type) {

[ddorwin, 2012/10/20 00:14:36]

Is it easy for the client to know whether to call CancelDecrypt vs.
DeinitializeDecoder (and thus CancelDecode)? Do we really need separate
functions or could we just cancel whatever we are doing for audio/video based on
stream_type? If we can't do that, should we at least standardize on Cancel or
Deinitialize?

[xhwang, 2012/10/21 19:05:06]

I agree it's confusing.

For decoder, we have ResetDecoder and DeinitializeDecoder. For decryptor, we
choose "Cancel" because the decryptor is stateless, so we don't really need to
reset the decryptor other than canceling the cb. Also, we call it
Cancel*Decrypt* so it's clear that we are cancelling the decryption operation
scheduled by Decrypt(). 

It would be nice to consolidate CancelDecryptor and ResetDecoder. To do that,
first we need a proper name, e.g. Reset(stream_type). Not sure if it's clearer
for the caller though. Second, we need two extra variables to remember which
path (DO or D&D) the PluginInstance has chosen. 

One thing I don't like about consolidated functions for DO and D&D is that we
have to document and enforce in PI that DO and D&D methods can't live together.
For example, if Decrypt() is called, InitializeVideoDecoder() should not be
called. For now, we don't need to worry about this.

WDYT?

+  DVLOG(3) << "CancelDecrypt() - stream_type: " << stream_type;
+
+  media::Decryptor::DecryptCB decrypt_cb;
+  switch (stream_type) {
+    case media::Decryptor::kAudio:
+      pending_audio_decrypt_request_id_ = 0;
+      decrypt_cb = base::ResetAndReturn(&pending_audio_decrypt_cb_);

[ddorwin, 2012/10/20 00:14:36]

Does this work if pending_audio_decrypt_cb_ is null/empty?

[xhwang, 2012/10/21 19:05:06]

Yes. Line 1676 checks this condition.

+      break;
+    case media::Decryptor::kVideo:
+      pending_video_decrypt_request_id_ = 0;
+      decrypt_cb = base::ResetAndReturn(&pending_video_decrypt_cb_);
+      break;
+    default:
+      NOTREACHED();
+      return false;
+  }
+
+  if (!decrypt_cb.is_null())
+    decrypt_cb.Run(media::Decryptor::kSuccess, NULL);
+
+  return true;
+}
+
 bool PluginInstance::InitializeAudioDecoder(
     const media::AudioDecoderConfig& decoder_config,
     const media::Decryptor::DecoderInitCB& init_cb) {
@@ -1654,10 +1737,32 @@ bool PluginInstance::InitializeVideoDecoder(
   return true;
 }
 
-bool PluginInstance::DeinitializeDecoder() {
+void PluginInstance::CancelDecode(media::Decryptor::StreamType stream_type) {
+  switch (stream_type) {
+    case media::Decryptor::kAudio:
+      pending_audio_decode_request_id_ = 0;
+      if (!pending_audio_decode_cb_.is_null())
+        base::ResetAndReturn(&pending_audio_decode_cb_).Run(
+            media::Decryptor::kSuccess, media::Decryptor::AudioBuffers());

[ddorwin, 2012/10/20 00:14:36]

odd that video passes NULL but this doesn't. I assume there is a reason.

[xhwang, 2012/10/21 19:05:06]

VideoDecodeCB takes scoped_refptr<VideoFrame>, so we use NULL.
AudioDecodeCB takes std::list<Buffer>, so we pass empty list.

+      break;
+    case media::Decryptor::kVideo:
+      pending_video_decode_request_id_ = 0;
+      if (!pending_video_decode_cb_.is_null())
+        base::ResetAndReturn(&pending_video_decode_cb_).Run(
+            media::Decryptor::kSuccess, NULL);
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+bool PluginInstance::DeinitializeDecoder(
+    media::Decryptor::StreamType stream_type) {
   if (!LoadContentDecryptorInterface())
     return false;
 
+  CancelDecode(stream_type);
+
   // TODO(tomfinegan): Add decoder deinitialize request tracking, and get
   // stream type from media stack.
   plugin_decryption_interface_->DeinitializeDecoder(
@@ -1667,10 +1772,12 @@ bool PluginInstance::DeinitializeDecoder() {
   return true;
 }
 
-bool PluginInstance::ResetDecoder() {
+bool PluginInstance::ResetDecoder(media::Decryptor::StreamType stream_type) {

[ddorwin, 2012/10/20 00:14:36]

Does just a Reset() for decrypt & D&D make sense?

[xhwang, 2012/10/21 19:05:06]

We can discuss about this. See above comment.

   if (!LoadContentDecryptorInterface())
     return false;
 
+  CancelDecode(stream_type);
+
   // TODO(tomfinegan): Add decoder reset request tracking, and get
   // stream type from media stack.
   plugin_decryption_interface_->ResetDecoder(pp_instance(),
@@ -1679,7 +1786,50 @@ bool PluginInstance::ResetDecoder() {
   return true;
 }
 
-bool PluginInstance::DecryptAndDecode(
+bool PluginInstance::DecryptAndDecodeAudio(
+    const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
+    const media::Decryptor::AudioDecodeCB& audio_decode_cb) {
+  if (!LoadContentDecryptorInterface())
+    return false;
+
+  // If |encrypted_buffer| is end-of-stream buffer, GetData() and GetDataSize()
+  // return NULL and 0 respectively. In that case, we'll just create a 0
+  // resource.
+  ScopedPPResource encrypted_resource(
+      ScopedPPResource::PassRef(),
+      MakeBufferResource(pp_instance(),
+                         encrypted_buffer->GetData(),
+                         encrypted_buffer->GetDataSize()));
+  if (!encrypted_buffer->IsEndOfStream() && !encrypted_resource.get())
+    return false;
+
+  const uint32_t request_id = next_decryption_request_id_++;
+  DVLOG(2) << "DecryptAndDecode() - request_id " << request_id;

[Tom Finegan, 2012/10/19 22:53:52]

s/DecryptAndDecode/DecryptAndDecodeAudio/

(Here and below)

[xhwang, 2012/10/21 19:05:06]

Done.

+
+  PP_EncryptedBlockInfo block_info;
+  if (!MakeEncryptedBlockInfo(
+      encrypted_buffer->GetDecryptConfig(),
+      encrypted_buffer->GetTimestamp().InMicroseconds(),
+      request_id,
+      &block_info)) {
+    return false;
+  }
+
+  // There is only one pending audio decode request at any time. This is
+  // enforced by the media pipeline.
+  DCHECK_EQ(pending_audio_decode_request_id_, 0u);
+  DCHECK(pending_audio_decode_cb_.is_null());
+  pending_audio_decode_request_id_ = request_id;
+  pending_audio_decode_cb_ = audio_decode_cb;
+
+  plugin_decryption_interface_->DecryptAndDecode(pp_instance(),
+                                                 PP_DECRYPTORSTREAMTYPE_AUDIO,
+                                                 encrypted_resource,
+                                                 &block_info);
+  return true;
+}
+
+bool PluginInstance::DecryptAndDecodeVideo(
     const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
     const media::Decryptor::VideoDecodeCB& video_decode_cb) {
   if (!LoadContentDecryptorInterface())
@@ -2348,6 +2498,9 @@ void PluginInstance::NeedKey(PP_Instance instance,
 void PluginInstance::KeyAdded(PP_Instance instance,
                               PP_Var key_system_var,
                               PP_Var session_id_var) {
+  if (!decryptor_client_)
+    return;
+
   StringVar* key_system_string = StringVar::FromPPVar(key_system_var);
   StringVar* session_id_string = StringVar::FromPPVar(session_id_var);
   if (!key_system_string || !session_id_string) {
@@ -2355,7 +2508,6 @@ void PluginInstance::KeyAdded(PP_Instance instance,
     return;
   }
 
-  DCHECK(decryptor_client_);
   decryptor_client_->KeyAdded(key_system_string->value(),
                               session_id_string->value());
 }
@@ -2365,6 +2517,9 @@ void PluginInstance::KeyMessage(PP_Instance instance,
                                 PP_Var session_id_var,
                                 PP_Resource message_resource,
                                 PP_Var default_url_var) {
+  if (!decryptor_client_)
+    return;
+
   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);
@@ -2388,7 +2543,6 @@ void PluginInstance::KeyMessage(PP_Instance instance,
   if (mapper.data() && mapper.size())
     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(),
@@ -2401,6 +2555,9 @@ void PluginInstance::KeyError(PP_Instance instance,
                               PP_Var session_id_var,
                               int32_t media_error,
                               int32_t system_code) {
+  if (!decryptor_client_)
+    return;
+
   StringVar* key_system_string = StringVar::FromPPVar(key_system_var);
   StringVar* session_id_string = StringVar::FromPPVar(session_id_var);
   if (!key_system_string || !session_id_string) {
@@ -2408,7 +2565,6 @@ void PluginInstance::KeyError(PP_Instance instance,
     return;
   }
 
-  DCHECK(decryptor_client_);
   decryptor_client_->KeyError(
       key_system_string->value(),
       session_id_string->value(),
@@ -2462,12 +2618,26 @@ void PluginInstance::DeliverBlock(PP_Instance instance,
   DVLOG(2) << "DeliverBlock() - request_id: "
            << block_info->tracking_info.request_id;
   DCHECK(block_info);
-  DecryptionCBMap::iterator found = pending_decryption_cbs_.find(
-      block_info->tracking_info.request_id);
-  if (found == pending_decryption_cbs_.end())
+
+  // If the request ID is not valid or does not match what's saved, do nothing.
+  const uint32_t request_id = block_info->tracking_info.request_id;
+  if (request_id == 0)
     return;
-  media::Decryptor::DecryptCB decrypt_cb = found->second;
-  pending_decryption_cbs_.erase(found);
+
+  media::Decryptor::DecryptCB decrypt_cb;
+  if (request_id == pending_audio_decrypt_request_id_) {
+    DCHECK(!pending_audio_decrypt_cb_.is_null());
+    pending_audio_decrypt_request_id_ = 0;
+    decrypt_cb = base::ResetAndReturn(&pending_audio_decrypt_cb_);
+  } else if (request_id == pending_video_decrypt_request_id_) {
+    DCHECK(!pending_video_decrypt_cb_.is_null());
+    pending_video_decrypt_request_id_ = 0;
+    decrypt_cb = base::ResetAndReturn(&pending_video_decrypt_cb_);
+  } else {
+    DVLOG(1) << "DeliverBlock() - request_id "
+             << block_info->tracking_info.request_id << " no found";

[ddorwin, 2012/10/20 00:14:36]

s/no/not/

[ddorwin, 2012/10/20 00:14:36]

you already have request_id local var you can use.

[xhwang, 2012/10/21 19:05:06]

Done.

+    return;
+  }
 
   if (block_info->result == PP_DECRYPTRESULT_DECRYPT_NOKEY) {
     decrypt_cb.Run(media::Decryptor::kNoKey, NULL);
@@ -2587,8 +2757,45 @@ void PluginInstance::DeliverSamples(PP_Instance instance,
                                     const PP_DecryptedBlockInfo* block_info) {
   DVLOG(2) << "DeliverSamples() - request_id: "
            << block_info->tracking_info.request_id;
-  // TODO(tomfinegan): To be implemented after completion of v0.1 of the
-  // EME/CDM work.
+  DCHECK(audio_frames);

[ddorwin, 2012/10/20 00:14:36]

decrypted_block can be null above but this can't? or just a missing DCHECK
above?

[xhwang, 2012/10/21 19:05:06]

Done.

+
+  // If the request ID is not valid or does not match what's saved, do nothing.
+  if (block_info->tracking_info.request_id == 0 ||
+      block_info->tracking_info.request_id !=
+          pending_audio_decode_request_id_) {
+    DCHECK(pending_audio_decode_cb_.is_null());
+    return;
+  }
+
+  DCHECK(!pending_audio_decode_cb_.is_null());
+  pending_audio_decode_request_id_ = 0;
+  media::Decryptor::AudioDecodeCB audio_decode_cb =
+      base::ResetAndReturn(&pending_audio_decode_cb_);
+
+  const media::Decryptor::AudioBuffers empty_frames;
+
+  if (block_info->result == PP_DECRYPTRESULT_DECRYPT_NOKEY) {
+    audio_decode_cb.Run(media::Decryptor::kNoKey, empty_frames);
+    return;
+  }
+
+  if (block_info->result != PP_DECRYPTRESULT_SUCCESS) {
+    audio_decode_cb.Run(media::Decryptor::kError, empty_frames);
+    return;
+  }
+
+  if (audio_frames == 0) {  // End-of-stream frame.

[xhwang, 2012/10/19 17:07:33]

We can remove this check if we all agree that EOS frame should always be a
zero-size buffer in audio_frames.

[Tom Finegan, 2012/10/19 22:53:52]

This isn't necessarily and end of stream frame. The decoder might need more data
to produce output.

It might be simpler to just give in and add PP_DECRYPTRESULT_NEEDMOREDATA.

[xhwang, 2012/10/21 19:05:06]

Yeah, done in another CL: 11234019

+    audio_decode_cb.Run(media::Decryptor::kSuccess, empty_frames);
+    return;
+  }
+
+  media::Decryptor::AudioBuffers audio_frame_list;
+  if (!MakeAudioFrames(audio_frames, &audio_frame_list)) {
+    audio_decode_cb.Run(media::Decryptor::kError, empty_frames);
+    return;
+  }
+
+  audio_decode_cb.Run(media::Decryptor::kSuccess, audio_frame_list);
 }
 
 void PluginInstance::NumberOfFindResultsChanged(PP_Instance instance,