Reuse PPB_Buffer_Impl in ContentDecryptorDelegate.

webkit/plugins/ppapi/content_decryptor_delegate.cc

Index: webkit/plugins/ppapi/content_decryptor_delegate.cc
diff --git a/webkit/plugins/ppapi/content_decryptor_delegate.cc b/webkit/plugins/ppapi/content_decryptor_delegate.cc
index a799cc0523f704f81d18ca1d361a19a082de4c2c..cc01eee89d21fbc81564dbe87f49ef4f9863bac5 100644
--- a/webkit/plugins/ppapi/content_decryptor_delegate.cc
+++ b/webkit/plugins/ppapi/content_decryptor_delegate.cc
@@ -14,7 +14,6 @@
 #include "media/base/video_decoder_config.h"
 #include "media/base/video_frame.h"
 #include "media/base/video_util.h"
-#include "ppapi/shared_impl/scoped_pp_resource.h"
 #include "ppapi/shared_impl/var.h"
 #include "ppapi/shared_impl/var_tracker.h"
 #include "ppapi/thunk/enter.h"
@@ -32,6 +31,8 @@ namespace ppapi {
 
 namespace {
 
+const int kInitialMediaBufferSize = 1024;
+
 // Fills |resource| with a PP_Resource containing a PPB_Buffer_Impl and copies
 // |data| into the buffer resource. The |resource| has a reference count of 1.
 // If |data| is NULL, fills |resource| with a PP_Resource with ID of 0.
@@ -39,6 +40,7 @@ namespace {
 bool MakeBufferResource(PP_Instance instance,
                         const uint8* data, int size,
                         ScopedPPResource* resource) {
+  TRACE_EVENT0("eme", "ContentDecryptorDelegate - MakeBufferResource");
   DCHECK(resource);
 
   if (!data || !size) {
@@ -274,7 +276,9 @@ ContentDecryptorDelegate::ContentDecryptorDelegate(
       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) {
+      pending_video_decode_request_id_(0),
+      audio_input_resource_size_(0),
+      video_input_resource_size_(0) {
 }
 
 void ContentDecryptorDelegate::set_decrypt_client(
@@ -335,11 +339,14 @@ bool ContentDecryptorDelegate::Decrypt(
     const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
     const media::Decryptor::DecryptCB& decrypt_cb) {
   DVLOG(3) << "Decrypt() - stream_type: " << stream_type;
+  // |{audio|video}_input_resource_size_| must be available here because there

[ddorwin, 2012/11/17 02:07:38]

What do you mean by "available"?

[xhwang, 2012/11/18 00:10:33]

Done here and elsewhere.

+  // is only one pending audio/video decrypt request at any time. This is
+  // enforced by the media pipeline.
   ScopedPPResource encrypted_resource;
-  if (!MakeBufferResource(pp_instance_,
-                          encrypted_buffer->GetData(),
-                          encrypted_buffer->GetDataSize(),
-                          &encrypted_resource) ||
+  if (!MakeMediaBufferResource(stream_type,
+                               encrypted_buffer->GetData(),
+                               encrypted_buffer->GetDataSize(),
+                               &encrypted_resource) ||
       !encrypted_resource.get()) {
     return false;
   }
@@ -499,11 +506,14 @@ bool ContentDecryptorDelegate::DecryptAndDecodeAudio(
   // 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.
+  // |audio_input_resource_size_| must be available here because there is only
+  // one pending audio decode request at any time. This is enforced by the media
+  // pipeline.
   ScopedPPResource encrypted_resource;
-  if (!MakeBufferResource(pp_instance_,
-                          encrypted_buffer->GetData(),
-                          encrypted_buffer->GetDataSize(),
-                          &encrypted_resource)) {
+  if (!MakeMediaBufferResource(media::Decryptor::kAudio,
+                               encrypted_buffer->GetData(),
+                               encrypted_buffer->GetDataSize(),
+                               &encrypted_resource)) {
     return false;
   }
 
@@ -543,11 +553,14 @@ bool ContentDecryptorDelegate::DecryptAndDecodeVideo(
   // 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.
+  // |video_input_resource_size_| must be available here because there is only
+  // one pending video decode request at any time. This is enforced by the media
+  // pipeline.
   ScopedPPResource encrypted_resource;
-  if (!MakeBufferResource(pp_instance_,
-                          encrypted_buffer->GetData(),
-                          encrypted_buffer->GetDataSize(),
-                          &encrypted_resource)) {
+  if (!MakeBufferResource(pp_instance_,//media::Decryptor::kVideo,

[ddorwin, 2012/11/17 02:07:38]

Huh? Why not MakeMediaBufferResource?

[xhwang, 2012/11/18 00:10:33]

Oops, I used this to test how much gain I have and forgot to change it back.
Thanks for catching it. Done.

+                               encrypted_buffer->GetData(),
+                               encrypted_buffer->GetDataSize(),
+                               &encrypted_resource)) {
     return false;
   }
 
@@ -890,5 +903,59 @@ void ContentDecryptorDelegate::CancelDecode(
   }
 }
 
+bool ContentDecryptorDelegate::MakeMediaBufferResource(
+    media::Decryptor::StreamType stream_type,
+    const uint8* data, int size,
+    ScopedPPResource* resource) {
+  TRACE_EVENT0("eme", "ContentDecryptorDelegate::MakeMediaBufferResource");
+
+  DCHECK(resource);
+
+  if (!data || !size) {
+    DCHECK(!data && !size);
+    resource->Release();
+    return true;
+  }
+
+  DCHECK(stream_type == media::Decryptor::kAudio ||
+         stream_type == media::Decryptor::kVideo);
+  ScopedPPResource& media_resource = (stream_type == media::Decryptor::kAudio) ?
+      audio_input_resource_ : video_input_resource_;
+  int& media_resource_size = (stream_type == media::Decryptor::kAudio) ?
+      audio_input_resource_size_ : video_input_resource_size_;
+
+  if (media_resource_size < size) {
+    if (media_resource_size == 0)
+      media_resource_size = kInitialMediaBufferSize;

[ddorwin, 2012/11/17 02:07:38]

Are you just trying to make it a power of 2? If so, comment that. Also, maybe
s/Initial/Minimum since it may not be the initial size.

[xhwang, 2012/11/18 00:10:33]

It can really just be any size. Use power of 2 here only for potential alignment
benefits. Also, double the size below every time to make it grow exponentially.
Comments added.

+
+    while (media_resource_size < size) {
+      media_resource_size *= 2;

[ddorwin, 2012/11/17 02:07:38]

Should we just do pow(ceiling(log(size)))?

[xhwang, 2012/11/18 00:10:33]

Using a while loop and double the size (shift one bit) should be more efficient
than introducing math functions.

+    }
+
+    DVLOG(2) << "Size of media buffer for "
+             << ((stream_type == media::Decryptor::kAudio) ? "audio" : "video")
+             << " stream bumped to " << media_resource_size
+             << " bytes to fit input.";
+    media_resource = ScopedPPResource(
+        ScopedPPResource::PassRef(),
+        PPB_Buffer_Impl::Create(pp_instance_, media_resource_size));
+  }

[ddorwin, 2012/11/17 02:07:38]

If any of the above fails, we need to reset the appropriate media_resource_size
(to 0 since we destroyed the old buffer).

[xhwang, 2012/11/18 00:10:33]

Added reset of media_resource and media_resource_size in the following failure
cases. But I feel this is a little overkill because if we return false in this
function, the playback will be terminated and everything will be cleared later.
WDYT?

[ddorwin, 2012/11/18 00:37:19]

It's better to have a unit of code function correctly without making assumptions
about the caller.

Another option is to make a copy of the value and have a scoped resource then
only assign them to the members at 968.

[xhwang, 2012/11/19 21:16:34]

okay

+
+  if (!media_resource.get())

[ddorwin, 2012/11/17 02:07:38]

This can never fail outside the bracket at 942 unless we have a bug. Suggest
moving it up and DCHECKing if you want.

[xhwang, 2012/11/18 00:10:33]

This can happen for new created buffer. Move this block after 941.

+    return false;
+
+  EnterResourceNoLock<PPB_Buffer_API> enter(media_resource, true);
+  if (enter.failed())
+    return false;
+
+  BufferAutoMapper mapper(enter.object());
+  if (!mapper.data() || mapper.size() < static_cast<size_t>(size))
+    return false;
+  memcpy(mapper.data(), data, size);
+
+  *resource = media_resource;
+  return true;
+}
+
 }  // namespace ppapi
 }  // namespace webkit