Reuse PPB_Buffer_Impl in ContentDecryptorDelegate.

webkit/plugins/ppapi/content_decryptor_delegate.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/plugins/ppapi/content_decryptor_delegate.h"
 
 #include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #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"
 #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"
 #include "ppapi/thunk/ppb_buffer_api.h"
 #include "webkit/plugins/ppapi/ppb_buffer_impl.h"
 
 using ppapi::PpapiGlobals;
 using ppapi::ScopedPPResource;
 using ppapi::StringVar;
 using ppapi::thunk::EnterResourceNoLock;
 using ppapi::thunk::PPB_Buffer_API;
 
 namespace webkit {
 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.
 // Returns true upon success and false if any error happened.
 bool MakeBufferResource(PP_Instance instance,
                         const uint8* data, int size,
                         ScopedPPResource* resource) {
+  TRACE_EVENT0("eme", "ContentDecryptorDelegate - MakeBufferResource");
   DCHECK(resource);
 
   if (!data || !size) {
     DCHECK(!data && !size);
     resource->Release();
     return true;
   }
 
   ScopedPPResource scoped_resource(ScopedPPResource::PassRef(),
                                    PPB_Buffer_Impl::Create(instance, size));
@@ skipping 215 matching lines @@
     const PPP_ContentDecryptor_Private* plugin_decryption_interface)
     : pp_instance_(pp_instance),
       plugin_decryption_interface_(plugin_decryption_interface),
       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) {
+      pending_video_decode_request_id_(0),
+      audio_input_resource_size_(0),
+      video_input_resource_size_(0) {
 }
 
 void ContentDecryptorDelegate::set_decrypt_client(
     media::DecryptorClient* decryptor_client) {
   decryptor_client_ = decryptor_client;
 }
 
 bool ContentDecryptorDelegate::GenerateKeyRequest(const std::string& key_system,
                                                   const std::string& type,
                                                   const uint8* init_data,
@@ skipping 40 matching lines @@
   return true;
 }
 
 // TODO(xhwang): Remove duplication of code in Decrypt(),
 // DecryptAndDecodeAudio() and DecryptAndDecodeVideo().
 bool ContentDecryptorDelegate::Decrypt(
     media::Decryptor::StreamType stream_type,
     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;
   }
 
   const uint32_t request_id = next_decryption_request_id_++;
   DVLOG(2) << "Decrypt() - request_id " << request_id;
 
   PP_EncryptedBlockInfo block_info;
   DCHECK(encrypted_buffer->GetDecryptConfig());
   if (!MakeEncryptedBlockInfo(encrypted_buffer->GetDecryptConfig(),
@@ skipping 139 matching lines @@
       pp_instance_, MediaDecryptorStreamTypeToPpStreamType(stream_type), 0);
   return true;
 }
 
 bool ContentDecryptorDelegate::DecryptAndDecodeAudio(
     const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
     const media::Decryptor::AudioDecodeCB& audio_decode_cb) {
   // 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;
   }
 
   // The resource should not be 0 for non-EOS buffer.
   if (!encrypted_buffer->IsEndOfStream() && !encrypted_resource.get())
     return false;
 
   const uint32_t request_id = next_decryption_request_id_++;
   DVLOG(2) << "DecryptAndDecodeAudio() - request_id " << request_id;
 
@@ skipping 19 matching lines @@
                                                  &block_info);
   return true;
 }
 
 bool ContentDecryptorDelegate::DecryptAndDecodeVideo(
     const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
     const media::Decryptor::VideoDecodeCB& video_decode_cb) {
   // 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;
   }
 
   // The resource should not be 0 for non-EOS buffer.
   if (!encrypted_buffer->IsEndOfStream() && !encrypted_resource.get())
     return false;
 
   const uint32_t request_id = next_decryption_request_id_++;
   DVLOG(2) << "DecryptAndDecodeVideo() - request_id " << request_id;
   TRACE_EVENT_ASYNC_BEGIN0(
@@ skipping 322 matching lines @@
       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 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