Refactor WritableBuffer interface for more useful ptr management.

BUG=16011
TEST=DataBuffer unittest
Review URL: http://codereview.chromium.org/149573

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20821 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 95 insertions, 70 deletions

diff --git a/media/base/buffer_queue_unittest.cc b/media/base/buffer_queue_unittest.cc
index 91e10e0..8ba60be 100644
--- a/media/base/buffer_queue_unittest.cc
+++ b/media/base/buffer_queue_unittest.cc
@@ -14,14 +14,17 @@ namespace media {
 class BufferQueueTest : public testing::Test {
  protected:
   BufferQueueTest() {
-    buffer1 = new DataBuffer();
-    data1 = buffer1->GetWritableData(kDataSize);
+    buffer1 = new DataBuffer(kDataSize);
+    buffer1->SetDataSize(kDataSize);
+    data1 = buffer1->GetWritableData();
 
-    buffer2 = new DataBuffer();
-    data2 = buffer2->GetWritableData(kNewDataSize);
+    buffer2 = new DataBuffer(kNewDataSize);
+    buffer2->SetDataSize(kNewDataSize);
+    data2 = buffer2->GetWritableData();
 
-    bufferBig = new DataBuffer();
-    dataBig = bufferBig->GetWritableData(2 * (kDataSize + kNewDataSize));
+    bufferBig = new DataBuffer(2 * (kDataSize + kNewDataSize));
+    bufferBig->SetDataSize(2 * (kDataSize + kNewDataSize));
+    dataBig = bufferBig->GetWritableData();
 
     memcpy(data1, kData, kDataSize);
     memcpy(data2, kNewData, kNewDataSize);
diff --git a/media/base/buffers.h b/media/base/buffers.h
index def573e..2b4f25b 100644
--- a/media/base/buffers.h
+++ b/media/base/buffers.h
@@ -102,18 +102,15 @@ class Buffer : public StreamSample {
 
 class WritableBuffer : public Buffer  {
  public:
-  // Returns a read-write pointer to the buffer data.  When this method is
-  // called, any pointers previously returned from this method are invalid, and
-  // any data previously written to the buffer is invalid.  The buffer size
-  // is guaranteed to be at least the size of |buffer_size|.  The size
-  // that the GetDataSize() method will return is set to |buffer_size|.
-  // If, after filling the buffer, the caller wants to set the size to a smaller
-  // value then they can call the SetDataSize() method.
-  virtual uint8* GetWritableData(size_t buffer_size) = 0;
+  // Returns a read-write pointer to the buffer data.
+  virtual uint8* GetWritableData() = 0;
 
   // Updates the size of valid data in bytes, which must be less than or equal
-  // to the |buffer_size| passed to GetWritableData().
+  // to GetBufferSize().
   virtual void SetDataSize(size_t data_size) = 0;
+
+  // Returns the size of the underlying buffer.
+  virtual size_t GetBufferSize() const = 0;
 };
 
 
diff --git a/media/base/data_buffer.cc b/media/base/data_buffer.cc
index b88790f..a78cf3d 100644
--- a/media/base/data_buffer.cc
+++ b/media/base/data_buffer.cc
@@ -7,36 +7,36 @@
 
 namespace media {
 
-DataBuffer::DataBuffer()
-    : data_(NULL),
-      buffer_size_(0),
+DataBuffer::DataBuffer(uint8* buffer, size_t buffer_size)
+    : data_(buffer),
+      buffer_size_(buffer_size),
+      data_size_(buffer_size) {
+}
+
+DataBuffer::DataBuffer(size_t buffer_size)
+    : data_(new uint8[buffer_size]),
+      buffer_size_(buffer_size),
       data_size_(0) {
+  CHECK(data_.get()) << "DataBuffer ctor failed to allocate memory";
+
+  // Prevent arbitrary pointers.
+  if (buffer_size == 0)
+    data_.reset(NULL);
 }
 
 DataBuffer::~DataBuffer() {
-  delete [] data_;
 }
 
 const uint8* DataBuffer::GetData() const {
-  return data_;
+  return data_.get();
 }
 
 size_t DataBuffer::GetDataSize() const {
   return data_size_;
 }
 
-uint8* DataBuffer::GetWritableData(size_t buffer_size) {
-  if (buffer_size > buffer_size_) {
-    delete [] data_;
-    data_ = new uint8[buffer_size];
-    if (!data_) {
-      NOTREACHED();
-      buffer_size = 0;
-    }
-    buffer_size_ = buffer_size;
-  }
-  data_size_ = buffer_size;
-  return data_;
+uint8* DataBuffer::GetWritableData() {
+  return data_.get();
 }
 
 
@@ -45,4 +45,8 @@ void DataBuffer::SetDataSize(size_t data_size) {
   data_size_ = data_size;
 }
 
+size_t DataBuffer::GetBufferSize() const {
+  return buffer_size_;
+}
+
 }  // namespace media
diff --git a/media/base/data_buffer.h b/media/base/data_buffer.h
index 8099d14..26ecb8a 100644
--- a/media/base/data_buffer.h
+++ b/media/base/data_buffer.h
@@ -5,32 +5,40 @@
 // A simple implementation of WritableBuffer that takes ownership of
 // the given data pointer.
 //
-// DataBuffer assumes that memory was allocated with new char[].
+// DataBuffer assumes that memory was allocated with new uint8[].
 
 #ifndef MEDIA_BASE_DATA_BUFFER_H_
 #define MEDIA_BASE_DATA_BUFFER_H_
 
+#include "base/scoped_ptr.h"
 #include "media/base/buffers.h"
 
 namespace media {
 
 class DataBuffer : public WritableBuffer {
  public:
-  DataBuffer();
+  // Takes ownership of the passed |buffer|, assumes valid data of size
+  // |buffer_size|.
+  DataBuffer(uint8* buffer, size_t buffer_size);
+
+  // Allocates buffer of size |buffer_size|. If |buffer_size| is 0, |data_| is
+  // set to a NULL ptr.
+  explicit DataBuffer(size_t buffer_size);
 
   // Buffer implementation.
   virtual const uint8* GetData() const;
   virtual size_t GetDataSize() const;
 
   // WritableBuffer implementation.
-  virtual uint8* GetWritableData(size_t buffer_size);
+  virtual uint8* GetWritableData();
   virtual void SetDataSize(size_t data_size);
+  virtual size_t GetBufferSize() const;
 
  protected:
   virtual ~DataBuffer();
 
  private:
-  uint8* data_;
+  scoped_array<uint8> data_;
   size_t buffer_size_;
   size_t data_size_;
 };
diff --git a/media/base/data_buffer_unittest.cc b/media/base/data_buffer_unittest.cc
index 161e018..2d014ffa 100644
--- a/media/base/data_buffer_unittest.cc
+++ b/media/base/data_buffer_unittest.cc
@@ -8,28 +8,21 @@
 
 using media::DataBuffer;
 
-TEST(DataBufferTest, Basic) {
-  const char kData[] = "hello";
-  const size_t kDataSize = arraysize(kData);
-  const char kNewData[] = "chromium";
-  const size_t kNewDataSize = arraysize(kNewData);
+TEST(DataBufferTest, StreamSampleImpl) {
   const base::TimeDelta kTimestampA = base::TimeDelta::FromMicroseconds(1337);
   const base::TimeDelta kDurationA = base::TimeDelta::FromMicroseconds(1667);
   const base::TimeDelta kTimestampB = base::TimeDelta::FromMicroseconds(1234);
   const base::TimeDelta kDurationB = base::TimeDelta::FromMicroseconds(5678);
 
-  // Create our buffer and copy some data into it.
   // Create a DataBuffer.
-  scoped_refptr<DataBuffer> buffer = new DataBuffer();
+  scoped_refptr<DataBuffer> buffer = new DataBuffer(0);
   ASSERT_TRUE(buffer);
 
-  // Test StreamSample implementation.
   buffer->SetTimestamp(kTimestampA);
   buffer->SetDuration(kDurationA);
   EXPECT_TRUE(kTimestampA == buffer->GetTimestamp());
   EXPECT_TRUE(kDurationA == buffer->GetDuration());
   EXPECT_TRUE(buffer->IsEndOfStream());
-  EXPECT_FALSE(buffer->GetData());
   EXPECT_FALSE(buffer->IsDiscontinuous());
   buffer->SetTimestamp(kTimestampB);
   buffer->SetDuration(kDurationB);
@@ -40,25 +33,47 @@ TEST(DataBufferTest, Basic) {
   EXPECT_TRUE(buffer->IsDiscontinuous());
   buffer->SetDiscontinuous(false);
   EXPECT_FALSE(buffer->IsDiscontinuous());
+}
+
+TEST(DataBufferTest, Ctors) {
+  const size_t kTestSize = 10;
 
-  // Test WritableBuffer implementation.
+  scoped_refptr<DataBuffer> buffer = new DataBuffer(0);
   EXPECT_FALSE(buffer->GetData());
-  uint8* data = buffer->GetWritableData(kDataSize);
+
+  scoped_refptr<DataBuffer> buffer2 = new DataBuffer(kTestSize);
+  EXPECT_EQ(0u, buffer2->GetDataSize());
+  EXPECT_EQ(kTestSize, buffer2->GetBufferSize());
+}
+
+TEST(DataBufferTest, WritableBufferImpl) {
+  const char kData[] = "hello";
+  const size_t kDataSize = arraysize(kData);
+  const char kNewData[] = "chromium";
+  const size_t kNewDataSize = arraysize(kNewData);
+
+  // Create a DataBuffer.
+  scoped_refptr<DataBuffer> buffer = new DataBuffer(kDataSize);
+  ASSERT_TRUE(buffer);
+
+  uint8* data = buffer->GetWritableData();
   ASSERT_TRUE(data);
-  ASSERT_EQ(buffer->GetDataSize(), kDataSize);
+  ASSERT_EQ(kDataSize, buffer->GetBufferSize());
   memcpy(data, kData, kDataSize);
+  buffer->SetDataSize(kDataSize);
   const uint8* read_only_data = buffer->GetData();
   ASSERT_EQ(data, read_only_data);
   ASSERT_EQ(0, memcmp(read_only_data, kData, kDataSize));
   EXPECT_FALSE(buffer->IsEndOfStream());
 
-  data = buffer->GetWritableData(kNewDataSize + 10);
+  scoped_refptr<DataBuffer> buffer2 = new DataBuffer(kNewDataSize + 10);
+  data = buffer2->GetWritableData();
   ASSERT_TRUE(data);
-  ASSERT_EQ(buffer->GetDataSize(), kNewDataSize + 10);
+  ASSERT_EQ(kNewDataSize + 10, buffer2->GetBufferSize());
   memcpy(data, kNewData, kNewDataSize);
-  read_only_data = buffer->GetData();
-  buffer->SetDataSize(kNewDataSize);
-  EXPECT_EQ(buffer->GetDataSize(), kNewDataSize);
+  buffer2->SetDataSize(kNewDataSize);
+  read_only_data = buffer2->GetData();
+  EXPECT_EQ(kNewDataSize, buffer2->GetDataSize());
   ASSERT_EQ(data, read_only_data);
   EXPECT_EQ(0, memcmp(read_only_data, kNewData, kNewDataSize));
 }
diff --git a/media/filters/audio_renderer_algorithm_default.cc b/media/filters/audio_renderer_algorithm_default.cc
index d13cbf0..7a437d2 100644
--- a/media/filters/audio_renderer_algorithm_default.cc
+++ b/media/filters/audio_renderer_algorithm_default.cc
@@ -24,7 +24,7 @@ size_t AudioRendererAlgorithmDefault::FillBuffer(DataBuffer* buffer_out) {
 
   if (playback_rate() == 1.0f) {
     size_t dest_length = buffer_out->GetDataSize();
-    uint8* dest = buffer_out->GetWritableData(dest_length);
+    uint8* dest = buffer_out->GetWritableData();
 
     // If we don't have enough data, copy what we have.
     if (QueueSize() < dest_length)
@@ -36,7 +36,7 @@ size_t AudioRendererAlgorithmDefault::FillBuffer(DataBuffer* buffer_out) {
     // Mute (we will write to the whole buffer, so set |dest_written| to the
     // requested size).
     dest_written = buffer_out->GetDataSize();
-    memset(buffer_out->GetWritableData(dest_written), 0, dest_written);
+    memset(buffer_out->GetWritableData(), 0, dest_written);
 
     // Calculate the number of bytes we "used".
     size_t scaled_dest_length =
diff --git a/media/filters/audio_renderer_algorithm_ola.cc b/media/filters/audio_renderer_algorithm_ola.cc
index b32b27a..0081068 100644
--- a/media/filters/audio_renderer_algorithm_ola.cc
+++ b/media/filters/audio_renderer_algorithm_ola.cc
@@ -32,7 +32,7 @@ size_t AudioRendererAlgorithmOLA::FillBuffer(DataBuffer* buffer_out) {
 
   // Grab info from |buffer_out| and handle the simple case of normal playback.
   size_t dest_remaining = buffer_out->GetDataSize();
-  uint8* dest = buffer_out->GetWritableData(dest_remaining);
+  uint8* dest = buffer_out->GetWritableData();
   size_t dest_written = 0;
   if (playback_rate() == 1.0f) {
     if (QueueSize() < dest_remaining)
diff --git a/media/filters/ffmpeg_audio_decoder.cc b/media/filters/ffmpeg_audio_decoder.cc
index 5d9e3c1..edb7a02 100644
--- a/media/filters/ffmpeg_audio_decoder.cc
+++ b/media/filters/ffmpeg_audio_decoder.cc
@@ -107,9 +107,9 @@ void FFmpegAudioDecoder::OnDecode(Buffer* input) {
 
   // If we have decoded something, enqueue the result.
   if (output_buffer_size) {
-    DataBuffer* result_buffer = new DataBuffer();
-    memcpy(result_buffer->GetWritableData(output_buffer_size),
-           output_buffer, output_buffer_size);
+    DataBuffer* result_buffer = new DataBuffer(output_buffer_size);
+    uint8* data = result_buffer->GetWritableData();
+    memcpy(data, output_buffer, output_buffer_size);
 
     // Determine the duration if the demuxer couldn't figure it out, otherwise
     // copy it over.
@@ -131,7 +131,7 @@ void FFmpegAudioDecoder::OnDecode(Buffer* input) {
   // 2. FFmpeg didn't output anything.
   // 3. An end of stream buffer is received.
   if (result == 0 && output_buffer_size == 0 && input->IsEndOfStream()) {
-    DataBuffer* result_buffer = new DataBuffer();
+    DataBuffer* result_buffer = new DataBuffer(0);
     result_buffer->SetTimestamp(input->GetTimestamp());
     result_buffer->SetDuration(input->GetDuration());
     EnqueueResult(result_buffer);
diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc
index b4d4ace..3178725 100644
--- a/media/filters/ffmpeg_video_decoder_unittest.cc
+++ b/media/filters/ffmpeg_video_decoder_unittest.cc
@@ -92,9 +92,8 @@ class FFmpegVideoDecoderTest : public testing::Test {
     stream_.codec = &codec_context_;
     codec_context_.width = kWidth;
     codec_context_.height = kHeight;
-    buffer_ = new DataBuffer();
-    buffer_->GetWritableData(1);
-    end_of_stream_buffer_ = new DataBuffer();
+    buffer_ = new DataBuffer(1);
+    end_of_stream_buffer_ = new DataBuffer(0);
 
     // Initialize MockFFmpeg.
     MockFFmpeg::set(&mock_ffmpeg_);
@@ -352,7 +351,7 @@ TEST_F(FFmpegVideoDecoderTest, FindPtsAndDuration) {
   EXPECT_EQ(116, result_pts.timestamp.InMicroseconds());
   EXPECT_EQ(500000, result_pts.duration.InMicroseconds());
 
-  // Test that having pts == 0 in the frame also behaves like the pts is not 
+  // Test that having pts == 0 in the frame also behaves like the pts is not
   // provided.  This is because FFmpeg set the pts to zero when there is no
   // data for the frame, which means that value is useless to us.
   yuv_frame_.pts = 0;
@@ -499,7 +498,6 @@ TEST_F(FFmpegVideoDecoderTest, OnDecode_EnqueueVideoFrameError) {
   EXPECT_CALL(mock_ffmpeg_, AVFree(&yuv_frame_));
 
   // Attempt the decode.
-  buffer_->GetWritableData(1);
   mock_decoder->codec_context_ = &codec_context_;
   mock_decoder->OnDecode(buffer_);
 }
diff --git a/media/tools/wav_ola_test.cc b/media/tools/wav_ola_test.cc
index 5835bf2..64a8672 100644
--- a/media/tools/wav_ola_test.cc
+++ b/media/tools/wav_ola_test.cc
@@ -49,10 +49,10 @@ class Dummy {
   }
 
   void ReadDataForAlg() {
-    scoped_refptr<DataBuffer> b(new DataBuffer());
-    uint8* buf = b->GetWritableData(window_size_);
+    scoped_refptr<DataBuffer> buffer(new DataBuffer(window_size_));
+    uint8* buf = buffer->GetWritableData();
     if (fread(buf, 1, window_size_, input_) > 0) {
-      ola_->EnqueueBuffer(b.get());
+      ola_->EnqueueBuffer(buffer.get());
     }
   }
 
@@ -136,8 +136,8 @@ int main(int argc, const char** argv) {
   }
 
   // Create buffer to be filled by |ola|.
-  scoped_refptr<DataBuffer> buffer(new DataBuffer());
-  uint8* buf = buffer->GetWritableData(window_size);
+  scoped_refptr<DataBuffer> buffer(new DataBuffer(window_size));
+  const uint8* buf = buffer->GetData();
 
   // Keep track of bytes written to disk and bytes copied to |b|.
   size_t bytes_written = 0;