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

5 files changed, 80 insertions, 53 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));
 }