BufferedDataSource: don't reallocate buffer unnecessarily

media/blink/buffered_data_source.cc

 // Copyright 2013 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 "media/blink/buffered_data_source.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/single_thread_task_runner.h"
 #include "media/base/media_log.h"
@@ skipping 72 matching lines @@
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     WebFrame* frame,
     MediaLog* media_log,
     BufferedDataSourceHost* host,
     const DownloadingCB& downloading_cb)
     : url_(url),
       cors_mode_(cors_mode),
       total_bytes_(kPositionNotSpecified),
       streaming_(false),
       frame_(frame),
-      intermediate_read_buffer_(new uint8[kInitialReadBufferSize]),
-      intermediate_read_buffer_size_(kInitialReadBufferSize),
       render_task_runner_(task_runner),
       stop_signal_received_(false),
       media_has_played_(false),
       preload_(AUTO),
       bitrate_(0),
       playback_rate_(0.0),
       media_log_(media_log),
       host_(host),
       downloading_cb_(downloading_cb),
       weak_factory_(this) {
   DCHECK(host_);
   DCHECK(!downloading_cb_.is_null());
+  intermediate_read_buffer_.resize(kInitialReadBufferSize);

[xhwang, 2014/09/24 16:27:16]

You can do this in the initialization list by using vector's ctor:

explicit vector (size_type n, const value_type& val = value_type(),
                 const allocator_type& alloc = allocator_type());

 }
 
 BufferedDataSource::~BufferedDataSource() {}
 
 // A factory method to create BufferedResourceLoader using the read parameters.
 // This method can be overridden to inject mock BufferedResourceLoader object
 // for testing purpose.
 BufferedResourceLoader* BufferedDataSource::CreateResourceLoader(
     int64 first_byte_position, int64 last_byte_position) {
   DCHECK(render_task_runner_->BelongsToCurrentThread());
@@ skipping 178 matching lines @@
 
   bitrate_ = bitrate;
   loader_->SetBitrate(bitrate);
 }
 
 // This method is the place where actual read happens, |loader_| must be valid
 // prior to make this method call.
 void BufferedDataSource::ReadInternal() {
   DCHECK(render_task_runner_->BelongsToCurrentThread());
   int64 position = 0;
   int size = 0;

[xhwang, 2014/09/24 16:27:16]

You can change the type to size_t, then you don't need to static_cast on l.316.

   {
     base::AutoLock auto_lock(lock_);
     if (stop_signal_received_)
       return;
 
     position = read_op_->position();
     size = read_op_->size();
   }
 
   // First we prepare the intermediate read buffer for BufferedResourceLoader
   // to write to.
-  if (size > intermediate_read_buffer_size_) {
-    intermediate_read_buffer_.reset(new uint8[size]);
-  }
+  if (static_cast<int>(intermediate_read_buffer_.size()) < size)
+    intermediate_read_buffer_.resize(size);

[xhwang, 2014/09/24 16:27:16]

So the buffer size can only grow and never shrinks... I am not super familiar
with how this is used. Not sure what's the typical size of this buffer, and
whether we could be holding too much memory... 

[xhwang, 2014/09/24 16:27:16]

For the record: This has another benefit. If the new size is less than the
capacity of the vector, then we don't actually allocate a new buffer.

[xhwang, 2014/09/24 16:39:27]

I did a quick test on Youtube and a test clip and it seems the read size is
relatively stable. So this shouldn't be a concern.

 
   // Perform the actual read with BufferedResourceLoader.
   loader_->Read(position,
                 size,
-                intermediate_read_buffer_.get(),
+                &intermediate_read_buffer_[0],

[xhwang, 2014/09/24 16:27:16]

This is safe since the intermediate_read_buffer_ is never empty. But add a
DCHECK(!intermediate_read_buffer_.empty()) at l.319 to make this explicit?

                 base::Bind(&BufferedDataSource::ReadCallback,
                            weak_factory_.GetWeakPtr()));
 }
 
 
 /////////////////////////////////////////////////////////////////////////////
 // BufferedResourceLoader callback methods.
 void BufferedDataSource::StartCallback(
     BufferedResourceLoader::Status status) {
   DCHECK(render_task_runner_->BelongsToCurrentThread());
@@ skipping 106 matching lines @@
           base::Bind(&BufferedDataSource::ProgressCallback, weak_this),
           frame_);
       return;
     }
 
     ReadOperation::Run(read_op_.Pass(), kReadError);
     return;
   }
 
   if (bytes_read > 0) {
-    memcpy(read_op_->data(), intermediate_read_buffer_.get(), bytes_read);
+    memcpy(read_op_->data(), &intermediate_read_buffer_[0], bytes_read);
   } else if (bytes_read == 0 && total_bytes_ == kPositionNotSpecified) {
     // We've reached the end of the file and we didn't know the total size
     // before. Update the total size so Read()s past the end of the file will
     // fail like they would if we had known the file size at the beginning.
     total_bytes_ = loader_->instance_size();
 
     if (total_bytes_ != kPositionNotSpecified) {
       host_->SetTotalBytes(total_bytes_);
       host_->AddBufferedByteRange(loader_->first_byte_position(),
                                   total_bytes_);
@@ skipping 64 matching lines @@
   }
 
   // If media is currently playing or the page indicated preload=auto or the
   // the server does not support the byte range request or we do not want to go
   // too far ahead of the read head, use threshold strategy to enable/disable
   // deferring when the buffer is full/depleted.
   loader_->UpdateDeferStrategy(BufferedResourceLoader::kCapacityDefer);
 }
 
 }  // namespace media