Tighten up media::DecoderBuffer API contract for end of stream buffers (round 2).

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 "base/message_loop_proxy.h"
 #include "media/base/audio_decoder_config.h"
@@ skipping 60 matching lines @@
 // |array_size| is smaller than the |str| length.
 template <uint32_t array_size>
 bool CopyStringToArray(const std::string& str, uint8 (&array)[array_size]) {
   if (array_size < str.size())
     return false;
 
   memcpy(array, str.data(), str.size());
   return true;
 }
 
-// Fills the |block_info| with information from |decrypt_config|, |timestamp|
-// and |request_id|. |decrypt_config| can be NULL if the block is not encrypted.
-// This is useful for end-of-stream blocks.
+// Fills the |block_info| with information from |encrypted_buffer|.
+//
 // Returns true if |block_info| is successfully filled. Returns false
 // otherwise.
-bool MakeEncryptedBlockInfo(int data_size,
-                            const media::DecryptConfig* decrypt_config,
-                            int64_t timestamp,
-                            uint32_t request_id,
-                            PP_EncryptedBlockInfo* block_info) {
-  DCHECK(block_info);
-
+static bool MakeEncryptedBlockInfo(
+    const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
+    uint32_t request_id,
+    PP_EncryptedBlockInfo* block_info) {
   // TODO(xhwang): Fix initialization of PP_EncryptedBlockInfo here and
   // anywhere else.
   memset(block_info, 0, sizeof(*block_info));
+  block_info->tracking_info.request_id = request_id;
 
-  block_info->tracking_info.request_id = request_id;
-  block_info->tracking_info.timestamp = timestamp;
-
-  if (!decrypt_config)
+  // EOS buffers need a request ID and nothing more.
+  if (encrypted_buffer->IsEndOfStream())
     return true;
 
-  DCHECK(data_size) << "DecryptConfig is set on an empty buffer";

[xhwang, 2013/01/28 16:35:20]

can we keep this DCHECK so that we won't miss 0-byte length buffer?

[scherkus (not reviewing), 2013/01/28 18:28:37]

Done.

-  block_info->data_size = data_size;
+  block_info->tracking_info.timestamp =
+      encrypted_buffer->GetTimestamp().InMicroseconds();
+  block_info->data_size = encrypted_buffer->GetDataSize();
+
+  const media::DecryptConfig* decrypt_config =
+      encrypted_buffer->GetDecryptConfig();
   block_info->data_offset = decrypt_config->data_offset();
 
   if (!CopyStringToArray(decrypt_config->key_id(), block_info->key_id) ||
       !CopyStringToArray(decrypt_config->iv(), block_info->iv))
     return false;
 
   block_info->key_id_size = decrypt_config->key_id().size();
   block_info->iv_size = decrypt_config->iv().size();
 
   if (decrypt_config->subsamples().size() > arraysize(block_info->subsamples))
@@ skipping 230 matching lines @@
 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_| is not being used by the plugin
   // now because there is only one pending audio/video decrypt request at any
   // time. This is enforced by the media pipeline.
   scoped_refptr<PPB_Buffer_Impl> encrypted_resource;
   if (!MakeMediaBufferResource(stream_type,
-                               encrypted_buffer->GetData(),
-                               encrypted_buffer->GetDataSize(),
+                               encrypted_buffer,
                                &encrypted_resource) ||
       !encrypted_resource) {
     return false;
   }
   ScopedPPResource pp_resource(encrypted_resource.get());
 
   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->GetDataSize(),
-                              encrypted_buffer->GetDecryptConfig(),
-                              encrypted_buffer->GetTimestamp().InMicroseconds(),
-                              request_id,
-                              &block_info)) {
+  if (!MakeEncryptedBlockInfo(encrypted_buffer, request_id, &block_info)) {
     return false;
   }
 
   // There is only one pending decrypt request at any time per stream. This is
   // enforced by the media pipeline.
   switch (stream_type) {
     case media::Decryptor::kAudio:
       DCHECK_EQ(pending_audio_decrypt_request_id_, 0u);
       DCHECK(pending_audio_decrypt_cb_.is_null());
       pending_audio_decrypt_request_id_ = request_id;
@@ skipping 138 matching lines @@
 
   // TODO(tomfinegan): Add decoder reset request tracking.
   plugin_decryption_interface_->ResetDecoder(
       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 NULL
-  // resource.
   // |audio_input_resource_| is not being used by the plugin now
   // because there is only one pending audio decode request at any time.
   // This is enforced by the media pipeline.
   scoped_refptr<PPB_Buffer_Impl> encrypted_resource;
   if (!MakeMediaBufferResource(media::Decryptor::kAudio,
-                               encrypted_buffer->GetData(),
-                               encrypted_buffer->GetDataSize(),
+                               encrypted_buffer,
                                &encrypted_resource)) {
     return false;
   }
-  ScopedPPResource pp_resource(encrypted_resource.get());
 
   // The resource should not be NULL for non-EOS buffer.
   if (!encrypted_buffer->IsEndOfStream() && !encrypted_resource)
     return false;
 
   const uint32_t request_id = next_decryption_request_id_++;
   DVLOG(2) << "DecryptAndDecodeAudio() - request_id " << request_id;
 
   PP_EncryptedBlockInfo block_info = {};
-  if (!MakeEncryptedBlockInfo(
-      encrypted_buffer->GetDataSize(),
-      encrypted_buffer->GetDecryptConfig(),
-      encrypted_buffer->GetTimestamp().InMicroseconds(),
-      request_id,
-      &block_info)) {
+  if (!MakeEncryptedBlockInfo(encrypted_buffer, request_id, &block_info)) {
     return false;
   }
 
   SetBufferToFreeInTrackingInfo(&block_info.tracking_info);
 
   // There is only one pending audio decode request at any time. This is
   // enforced by the media pipeline. If this DCHECK is violated, our buffer
   // reuse policy is not valid, and we may have race problems for the shared
   // buffer.
   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;
 
+  ScopedPPResource pp_resource(encrypted_resource.get());
   plugin_decryption_interface_->DecryptAndDecode(pp_instance_,
                                                  PP_DECRYPTORSTREAMTYPE_AUDIO,
                                                  pp_resource,
                                                  &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 get a NULL
-  // resource.
   // |video_input_resource_| is not being used by the plugin now
   // because there is only one pending video decode request at any time.
   // This is enforced by the media pipeline.
   scoped_refptr<PPB_Buffer_Impl> encrypted_resource;
   if (!MakeMediaBufferResource(media::Decryptor::kVideo,
-                               encrypted_buffer->GetData(),
-                               encrypted_buffer->GetDataSize(),
+                               encrypted_buffer,
                                &encrypted_resource)) {
     return false;
   }
-  ScopedPPResource pp_resource(encrypted_resource.get());
 
   // The resource should not be 0 for non-EOS buffer.
   if (!encrypted_buffer->IsEndOfStream() && !encrypted_resource)
     return false;
 
   const uint32_t request_id = next_decryption_request_id_++;
   DVLOG(2) << "DecryptAndDecodeVideo() - request_id " << request_id;
   TRACE_EVENT_ASYNC_BEGIN0(
       "eme", "ContentDecryptorDelegate::DecryptAndDecodeVideo", request_id);
 
   PP_EncryptedBlockInfo block_info = {};
-  if (!MakeEncryptedBlockInfo(
-      encrypted_buffer->GetDataSize(),
-      encrypted_buffer->GetDecryptConfig(),
-      encrypted_buffer->GetTimestamp().InMicroseconds(),
-      request_id,
-      &block_info)) {
+  if (!MakeEncryptedBlockInfo(encrypted_buffer, request_id, &block_info)) {
     return false;
   }
 
   SetBufferToFreeInTrackingInfo(&block_info.tracking_info);
 
   // Only one pending video decode request at any time. This is enforced by the
   // media pipeline. If this DCHECK is violated, our buffer
   // reuse policy is not valid, and we may have race problems for the shared
   // buffer.
   DCHECK_EQ(pending_video_decode_request_id_, 0u);
   DCHECK(pending_video_decode_cb_.is_null());
   pending_video_decode_request_id_ = request_id;
   pending_video_decode_cb_ = video_decode_cb;
 
   // TODO(tomfinegan): Need to get stream type from media stack.
+  ScopedPPResource pp_resource(encrypted_resource.get());
   plugin_decryption_interface_->DecryptAndDecode(pp_instance_,
                                                  PP_DECRYPTORSTREAMTYPE_VIDEO,
                                                  pp_resource,
                                                  &block_info);
   return true;
 }
 
 void ContentDecryptorDelegate::NeedKey(PP_Var key_system_var,
                                        PP_Var session_id_var,
                                        PP_Var init_data_var) {
@@ skipping 332 matching lines @@
         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, uint32_t size,
+    const scoped_refptr<media::DecoderBuffer>& encrypted_buffer,
     scoped_refptr<PPB_Buffer_Impl>* resource) {
   TRACE_EVENT0("eme", "ContentDecryptorDelegate::MakeMediaBufferResource");
 
-  DCHECK(resource);
-
-  if (!data || !size) {
-    DCHECK(!data && !size);
-    resource = NULL;
+  // End of stream buffers are represented as null resources.
+  if (encrypted_buffer->IsEndOfStream()) {
+    *resource = NULL;

[xhwang, 2013/01/28 16:35:20]

looks like you found a bug! thanks!

[scherkus (not reviewing), 2013/01/28 18:28:37]

:)

     return true;
   }
 
+  const size_t data_size = static_cast<size_t>(encrypted_buffer->GetDataSize());

[xhwang, 2013/01/28 16:35:20]

move this down to 972 or 975?

[scherkus (not reviewing), 2013/01/28 18:28:37]

Done.

+
   DCHECK(stream_type == media::Decryptor::kAudio ||
          stream_type == media::Decryptor::kVideo);
   scoped_refptr<PPB_Buffer_Impl>& media_resource =
       (stream_type == media::Decryptor::kAudio) ? audio_input_resource_ :
                                                   video_input_resource_;
-  if (!media_resource || (media_resource->size() < size)) {
+  if (!media_resource || media_resource->size() < data_size) {
     // Either the buffer hasn't been created yet, or we have one that isn't big
     // enough to fit |size| bytes.
 
     // Media resource size starts from |kMinimumMediaBufferSize| and grows
     // exponentially to avoid frequent re-allocation of PPB_Buffer_Impl,
     // which is usually expensive. Since input media buffers are compressed,
     // they are usually small (compared to outputs). The over-allocated memory
     // should be negligible.
     const uint32_t kMinimumMediaBufferSize = 1024;
     uint32_t media_resource_size = media_resource ? media_resource->size() :
                                                     kMinimumMediaBufferSize;
-    while (media_resource_size < size)
+    while (media_resource_size < data_size)
       media_resource_size *= 2;
 
     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 = PPB_Buffer_Impl::CreateResource(pp_instance_,
                                                      media_resource_size);
     if (!media_resource)
       return false;
   }
 
   BufferAutoMapper mapper(media_resource);
-  if (!mapper.data() || mapper.size() < static_cast<size_t>(size)) {
+  if (!mapper.data() || mapper.size() < data_size) {
     media_resource = NULL;
     return false;
   }
-  memcpy(mapper.data(), data, size);
+  memcpy(mapper.data(), encrypted_buffer->GetData(), data_size);
 
   *resource = media_resource;
   return true;
 }
 
 void ContentDecryptorDelegate::FreeBuffer(uint32_t buffer_id) {
   if (buffer_id)
     free_buffers_.push(buffer_id);
 }
 
 void ContentDecryptorDelegate::SetBufferToFreeInTrackingInfo(
     PP_DecryptTrackingInfo* tracking_info) {
   DCHECK_EQ(tracking_info->buffer_id, 0u);
 
   if (free_buffers_.empty())
     return;
 
   tracking_info->buffer_id = free_buffers_.front();
   free_buffers_.pop();
 }
 
 }  // namespace ppapi
 }  // namespace webkit