Update media::DataBuffer API to match media::DecoderBuffer.

In short:
  * Zero-sized buffers are now permitted (they're harmless)
  * The buffer copying constructor has been made private and CopyFrom() has been introduced
  * End of stream buffers must be explicitly created via CreateEOSBuffer()
  * DCHECKs added to forbid calling DataBuffer methods on end of stream buffers

TBR=dmichael

Review URL: https://codereview.chromium.org/11902003

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

13 files changed, 118 insertions, 77 deletions

diff --git a/media/base/audio_splicer_unittest.cc b/media/base/audio_splicer_unittest.cc
index 20c923f..7c2e001 100644
--- a/media/base/audio_splicer_unittest.cc
+++ b/media/base/audio_splicer_unittest.cc
@@ -115,7 +115,7 @@ TEST_F(AudioSplicerTest, Reset) {
 
 TEST_F(AudioSplicerTest, EndOfStream) {
   scoped_refptr<DataBuffer> input_1 = GetNextInputBuffer(1);
-  scoped_refptr<DataBuffer> input_2 = new DataBuffer(0);  // End of stream.
+  scoped_refptr<DataBuffer> input_2 = DataBuffer::CreateEOSBuffer();
   scoped_refptr<DataBuffer> input_3 = GetNextInputBuffer(2);
   EXPECT_TRUE(input_2->IsEndOfStream());
 
diff --git a/media/base/data_buffer.cc b/media/base/data_buffer.cc
index a342023..898a477 100644
--- a/media/base/data_buffer.cc
+++ b/media/base/data_buffer.cc
@@ -8,51 +8,65 @@
 
 namespace media {
 
+DataBuffer::DataBuffer(int buffer_size)
+    : buffer_size_(buffer_size),
+      data_size_(0) {
+  CHECK_GE(buffer_size, 0);
+  data_.reset(new uint8[buffer_size_]);
+}
+
 DataBuffer::DataBuffer(scoped_array<uint8> buffer, int buffer_size)
     : data_(buffer.Pass()),
       buffer_size_(buffer_size),
       data_size_(buffer_size) {
-}
-
-DataBuffer::DataBuffer(int buffer_size)
-    : buffer_size_(buffer_size),
-      data_size_(0) {
-  Initialize();
+  CHECK(data_.get());
+  CHECK_GE(buffer_size, 0);
 }
 
 DataBuffer::DataBuffer(const uint8* data, int data_size)
     : buffer_size_(data_size),
       data_size_(data_size) {
-  Initialize();
+  if (!data) {
+    CHECK_EQ(data_size, 0);
+    return;
+  }
+
+  CHECK_GE(data_size, 0);
+  data_.reset(new uint8[buffer_size_]);
   memcpy(data_.get(), data, data_size_);
 }
 
 DataBuffer::~DataBuffer() {}
 
-void DataBuffer::Initialize() {
-  // Prevent arbitrary pointers.
-  if (buffer_size_ <= 0) {
-    buffer_size_ = data_size_ = 0;
-    data_.reset();
-    return;
-  }
+// static
+scoped_refptr<DataBuffer> DataBuffer::CopyFrom(const uint8* data, int size) {
+  // If you hit this CHECK you likely have a bug in a demuxer. Go fix it.
+  CHECK(data);
+  return make_scoped_refptr(new DataBuffer(data, size));
+}
 
-  data_.reset(new uint8[buffer_size_]);
+// static
+scoped_refptr<DataBuffer> DataBuffer::CreateEOSBuffer() {
+  return make_scoped_refptr(new DataBuffer(NULL, 0));
 }
 
 base::TimeDelta DataBuffer::GetTimestamp() const {
+  DCHECK(!IsEndOfStream());
   return timestamp_;
 }
 
 void DataBuffer::SetTimestamp(const base::TimeDelta& timestamp) {
+  DCHECK(!IsEndOfStream());
   timestamp_ = timestamp;
 }
 
 base::TimeDelta DataBuffer::GetDuration() const {
+  DCHECK(!IsEndOfStream());
   return duration_;
 }
 
 void DataBuffer::SetDuration(const base::TimeDelta& duration) {
+  DCHECK(!IsEndOfStream());
   duration_ = duration;
 }
 
@@ -61,24 +75,24 @@ bool DataBuffer::IsEndOfStream() const {
 }
 
 const uint8* DataBuffer::GetData() const {
+  DCHECK(!IsEndOfStream());
   return data_.get();
 }
 
 uint8* DataBuffer::GetWritableData() {
+  DCHECK(!IsEndOfStream());
   return data_.get();
 }
 
 int DataBuffer::GetDataSize() const {
+  DCHECK(!IsEndOfStream());
   return data_size_;
 }
 
 void DataBuffer::SetDataSize(int data_size) {
-  DCHECK_LE(data_size, buffer_size_);
+  DCHECK(!IsEndOfStream());
+  CHECK_LE(data_size, buffer_size_);
   data_size_ = data_size;
 }
 
-int DataBuffer::GetBufferSize() const {
-  return buffer_size_;
-}
-
 }  // namespace media
diff --git a/media/base/data_buffer.h b/media/base/data_buffer.h
index d3ccd37..76215c8 100644
--- a/media/base/data_buffer.h
+++ b/media/base/data_buffer.h
@@ -19,19 +19,22 @@ namespace media {
 // default memory allocator (i.e., new uint8[]).
 class MEDIA_EXPORT DataBuffer : public base::RefCountedThreadSafe<DataBuffer> {
  public:
+  // Allocates buffer of size |buffer_size| >= 0.
+  explicit DataBuffer(int buffer_size);
+
   // Assumes valid data of size |buffer_size|.
   DataBuffer(scoped_array<uint8> buffer, int buffer_size);
 
-  // Allocates buffer of size |buffer_size|. If |buffer_size| is 0, |data_| is
-  // set to NULL and this becomes an end of stream buffer.
+  // Create a DataBuffer whose |data_| is copied from |data|.
   //
-  // TODO(scherkus): Enforce calling CreateEOSBuffer() instead of passing 0 and
-  // sprinkle DCHECK()s everywhere.
-  explicit DataBuffer(int buffer_size);
+  // |data| must not be null and |size| must be >= 0.
+  static scoped_refptr<DataBuffer> CopyFrom(const uint8* data, int size);
 
-  // Allocates buffer of size |data_size|, copies [data,data+data_size) to
-  // the allocated buffer and sets data size to |data_size|.
-  DataBuffer(const uint8* data, int data_size);
+  // Create a DataBuffer indicating we've reached end of stream.
+  //
+  // Calling any method other than IsEndOfStream() on the resulting buffer
+  // is disallowed.
+  static scoped_refptr<DataBuffer> CreateEOSBuffer();
 
   base::TimeDelta GetTimestamp() const;
   void SetTimestamp(const base::TimeDelta& timestamp);
@@ -42,25 +45,27 @@ class MEDIA_EXPORT DataBuffer : public base::RefCountedThreadSafe<DataBuffer> {
   const uint8* GetData() const;
   uint8* GetWritableData();
 
-  // The size of valid data in bytes, which must be less than or equal
-  // to GetBufferSize().
+  // The size of valid data in bytes.
+  //
+  // Setting this value beyond the buffer size is disallowed.
   int GetDataSize() const;
   void SetDataSize(int data_size);
 
-  // Returns the size of the underlying buffer.
-  int GetBufferSize() const;
-
   // If there's no data in this buffer, it represents end of stream.
   bool IsEndOfStream() const;
 
  protected:
   friend class base::RefCountedThreadSafe<DataBuffer>;
+
+  // Allocates buffer of size |data_size|, copies [data,data+data_size) to
+  // the allocated buffer and sets data size to |data_size|.
+  //
+  // If |data| is null an end of stream buffer is created.
+  DataBuffer(const uint8* data, int data_size);
+
   virtual ~DataBuffer();
 
  private:
-  // Constructor helper method for memory allocations.
-  void Initialize();
-
   base::TimeDelta timestamp_;
   base::TimeDelta duration_;
 
diff --git a/media/base/data_buffer_unittest.cc b/media/base/data_buffer_unittest.cc
index 0daa5f6..a6800a8 100644
--- a/media/base/data_buffer_unittest.cc
+++ b/media/base/data_buffer_unittest.cc
@@ -8,24 +8,57 @@
 
 namespace media {
 
-TEST(DataBufferTest, Constructors) {
+TEST(DataBufferTest, Constructor_ZeroSize) {
+  // Zero-sized buffers are valid. In practice they aren't used very much but it
+  // eliminates clients from worrying about null data pointers.
+  scoped_refptr<DataBuffer> buffer = new DataBuffer(0);
+  EXPECT_TRUE(buffer->GetData());
+  EXPECT_TRUE(buffer->GetWritableData());
+  EXPECT_EQ(0, buffer->GetDataSize());
+  EXPECT_FALSE(buffer->IsEndOfStream());
+}
+
+TEST(DataBufferTest, Constructor_NonZeroSize) {
+  // Buffer size should be set.
+  scoped_refptr<DataBuffer> buffer = new DataBuffer(10);
+  EXPECT_TRUE(buffer->GetData());
+  EXPECT_TRUE(buffer->GetWritableData());
+  EXPECT_EQ(0, buffer->GetDataSize());
+  EXPECT_FALSE(buffer->IsEndOfStream());
+}
+
+TEST(DataBufferTest, Constructor_ScopedArray) {
+  // Data should be passed and both data and buffer size should be set.
+  const int kSize = 8;
+  scoped_array<uint8> data(new uint8[kSize]);
+  const uint8* kData = data.get();
+
+  scoped_refptr<DataBuffer> buffer = new DataBuffer(data.Pass(), kSize);
+  EXPECT_TRUE(buffer->GetData());
+  EXPECT_TRUE(buffer->GetWritableData());
+  EXPECT_EQ(kData, buffer->GetData());
+  EXPECT_EQ(kSize, buffer->GetDataSize());
+  EXPECT_FALSE(buffer->IsEndOfStream());
+}
+
+TEST(DataBufferTest, CopyFrom) {
   const uint8 kTestData[] = { 0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77 };
   const int kTestDataSize = arraysize(kTestData);
 
-  scoped_refptr<DataBuffer> buffer(new DataBuffer(0));
-  EXPECT_FALSE(buffer->GetData());
-
-  scoped_refptr<DataBuffer> buffer2(new DataBuffer(kTestDataSize));
-  EXPECT_EQ(0, buffer2->GetDataSize());
-  EXPECT_EQ(kTestDataSize, buffer2->GetBufferSize());
+  scoped_refptr<DataBuffer> buffer =
+      DataBuffer::CopyFrom(kTestData, kTestDataSize);
+  EXPECT_EQ(kTestDataSize, buffer->GetDataSize());
+  EXPECT_FALSE(buffer->IsEndOfStream());
 
-  scoped_refptr<DataBuffer> buffer3(new DataBuffer(kTestData, kTestDataSize));
-  EXPECT_EQ(kTestDataSize, buffer3->GetDataSize());
-  EXPECT_EQ(kTestDataSize, buffer3->GetBufferSize());
-  ASSERT_EQ(0, memcmp(buffer3->GetData(), kTestData, kTestDataSize));
   // Ensure we are copying the data, not just pointing to the original data.
-  buffer3->GetWritableData()[0] = 0xFF;
-  ASSERT_NE(0, memcmp(buffer3->GetData(), kTestData, kTestDataSize));
+  EXPECT_EQ(0, memcmp(buffer->GetData(), kTestData, kTestDataSize));
+  buffer->GetWritableData()[0] = 0xFF;
+  EXPECT_NE(0, memcmp(buffer->GetData(), kTestData, kTestDataSize));
+}
+
+TEST(DataBufferTest, CreateEOSBuffer) {
+  scoped_refptr<DataBuffer> buffer = DataBuffer::CreateEOSBuffer();
+  EXPECT_TRUE(buffer->IsEndOfStream());
 }
 
 TEST(DataBufferTest, Timestamp) {
@@ -58,17 +91,6 @@ TEST(DataBufferTest, Duration) {
   EXPECT_TRUE(buffer->GetDuration() == kDurationB);
 }
 
-TEST(DataBufferTest, IsEndOfStream) {
-  const uint8 kData[] = { 0x00, 0xFF };
-  const int kDataSize = arraysize(kData);
-
-  scoped_refptr<DataBuffer> buffer = new DataBuffer(0);
-  EXPECT_TRUE(buffer->IsEndOfStream());
-
-  buffer = new DataBuffer(kData, kDataSize);
-  EXPECT_FALSE(buffer->IsEndOfStream());
-}
-
 TEST(DataBufferTest, ReadingWriting) {
   const char kData[] = "hello";
   const int kDataSize = arraysize(kData);
@@ -81,7 +103,6 @@ TEST(DataBufferTest, ReadingWriting) {
 
   uint8* data = buffer->GetWritableData();
   ASSERT_TRUE(data);
-  ASSERT_EQ(kDataSize, buffer->GetBufferSize());
   memcpy(data, kData, kDataSize);
   buffer->SetDataSize(kDataSize);
   const uint8* read_only_data = buffer->GetData();
@@ -92,7 +113,6 @@ TEST(DataBufferTest, ReadingWriting) {
   scoped_refptr<DataBuffer> buffer2(new DataBuffer(kNewDataSize + 10));
   data = buffer2->GetWritableData();
   ASSERT_TRUE(data);
-  ASSERT_EQ(kNewDataSize + 10, buffer2->GetBufferSize());
   memcpy(data, kNewData, kNewDataSize);
   buffer2->SetDataSize(kNewDataSize);
   read_only_data = buffer2->GetData();
diff --git a/media/base/decoder_buffer.cc b/media/base/decoder_buffer.cc
index f7e79eb..a07ffac 100644
--- a/media/base/decoder_buffer.cc
+++ b/media/base/decoder_buffer.cc
@@ -34,13 +34,15 @@ void DecoderBuffer::Initialize() {
   memset(data_.get() + size_, 0, kPaddingSize);
 }
 
+// static
 scoped_refptr<DecoderBuffer> DecoderBuffer::CopyFrom(const uint8* data,
                                                      int data_size) {
-  // If you hit this checks you likely have a bug in a demuxer. Go fix it.
+  // If you hit this CHECK you likely have a bug in a demuxer. Go fix it.
   CHECK(data);
   return make_scoped_refptr(new DecoderBuffer(data, data_size));
 }
 
+// static
 scoped_refptr<DecoderBuffer> DecoderBuffer::CreateEOSBuffer() {
   return make_scoped_refptr(new DecoderBuffer(NULL, 0));
 }
diff --git a/media/base/seekable_buffer.cc b/media/base/seekable_buffer.cc
index b23457e..6ad3b02 100644
--- a/media/base/seekable_buffer.cc
+++ b/media/base/seekable_buffer.cc
@@ -89,7 +89,7 @@ bool SeekableBuffer::Append(const scoped_refptr<DataBuffer>& buffer_in) {
 
 bool SeekableBuffer::Append(const uint8* data, int size) {
   if (size > 0) {
-    DataBuffer* data_buffer = new DataBuffer(data, size);
+    scoped_refptr<DataBuffer> data_buffer = DataBuffer::CopyFrom(data, size);
     return Append(data_buffer);
   } else {
     // Return true if we have forward capacity.
diff --git a/media/base/seekable_buffer_unittest.cc b/media/base/seekable_buffer_unittest.cc
index 38d36a6..06a9477 100644
--- a/media/base/seekable_buffer_unittest.cc
+++ b/media/base/seekable_buffer_unittest.cc
@@ -225,7 +225,7 @@ TEST_F(SeekableBufferTest, SeekBackward) {
 TEST_F(SeekableBufferTest, GetCurrentChunk) {
   const int kSeekSize = kWriteSize / 3;
 
-  scoped_refptr<DataBuffer> buffer(new DataBuffer(data_, kWriteSize));
+  scoped_refptr<DataBuffer> buffer = DataBuffer::CopyFrom(data_, kWriteSize);
 
   const uint8* data;
   int size;
@@ -328,7 +328,7 @@ TEST_F(SeekableBufferTest, GetTime) {
   EXPECT_EQ(kNoTimestamp().ToInternalValue(),
             buffer_.current_time().ToInternalValue());
 
-  scoped_refptr<DataBuffer> buffer(new DataBuffer(data_, kWriteSize));
+  scoped_refptr<DataBuffer> buffer = DataBuffer::CopyFrom(data_, kWriteSize);
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     buffer->SetTimestamp(base::TimeDelta::FromMicroseconds(
diff --git a/media/filters/audio_renderer_impl_unittest.cc b/media/filters/audio_renderer_impl_unittest.cc
index 58a27df..195da03 100644
--- a/media/filters/audio_renderer_impl_unittest.cc
+++ b/media/filters/audio_renderer_impl_unittest.cc
@@ -183,7 +183,9 @@ class AudioRendererImplTest : public ::testing::Test {
   //
   // There must be a pending read callback.
   void DeliverEndOfStream() {
-    FulfillPendingRead(0);
+    CHECK(!read_cb_.is_null());
+    base::ResetAndReturn(&read_cb_).Run(
+        AudioDecoder::kOk, DataBuffer::CreateEOSBuffer());
   }
 
   // Delivers bytes until |renderer_|'s internal buffer is full and no longer
diff --git a/media/filters/decrypting_audio_decoder.cc b/media/filters/decrypting_audio_decoder.cc
index 9775bb9..1bb04b3 100644
--- a/media/filters/decrypting_audio_decoder.cc
+++ b/media/filters/decrypting_audio_decoder.cc
@@ -250,7 +250,7 @@ void DecryptingAudioDecoder::DoRead(const ReadCB& read_cb) {
 
   // Return empty (end-of-stream) frames if decoding has finished.
   if (state_ == kDecodeFinished) {
-    read_cb.Run(kOk, new DataBuffer(0));
+    read_cb.Run(kOk, DataBuffer::CreateEOSBuffer());
     return;
   }
 
@@ -421,7 +421,7 @@ void DecryptingAudioDecoder::DoDeliverFrame(
     DVLOG(2) << "DoDeliverFrame() - kNeedMoreData";
     if (scoped_pending_buffer_to_decode->IsEndOfStream()) {
       state_ = kDecodeFinished;
-      base::ResetAndReturn(&read_cb_).Run(kOk, (new DataBuffer(0)));
+      base::ResetAndReturn(&read_cb_).Run(kOk, DataBuffer::CreateEOSBuffer());
       return;
     }
 
diff --git a/media/filters/decrypting_audio_decoder_unittest.cc b/media/filters/decrypting_audio_decoder_unittest.cc
index d493481..5b70d52 100644
--- a/media/filters/decrypting_audio_decoder_unittest.cc
+++ b/media/filters/decrypting_audio_decoder_unittest.cc
@@ -81,7 +81,7 @@ class DecryptingAudioDecoderTest : public testing::Test {
         demuxer_(new StrictMock<MockDemuxerStream>()),
         encrypted_buffer_(CreateFakeEncryptedBuffer()),
         decoded_frame_(NULL),
-        end_of_stream_frame_(new DataBuffer(0)),
+        end_of_stream_frame_(DataBuffer::CreateEOSBuffer()),
         decoded_frame_list_() {
     scoped_refptr<DataBuffer> data_buffer = new DataBuffer(kFakeAudioFrameSize);
     data_buffer->SetDataSize(kFakeAudioFrameSize);
diff --git a/media/filters/ffmpeg_audio_decoder.cc b/media/filters/ffmpeg_audio_decoder.cc
index 912aa42..941bd43 100644
--- a/media/filters/ffmpeg_audio_decoder.cc
+++ b/media/filters/ffmpeg_audio_decoder.cc
@@ -468,7 +468,7 @@ void FFmpegAudioDecoder::RunDecodeLoop(
             converter_bus_->frames(), bits_per_channel_ / 8,
             output->GetWritableData());
       } else {
-        output = new DataBuffer(
+        output = DataBuffer::CopyFrom(
             av_frame_->extended_data[0] + start_sample * bytes_per_frame_,
             decoded_audio_size);
       }
@@ -479,8 +479,7 @@ void FFmpegAudioDecoder::RunDecodeLoop(
     } else if (IsEndOfStream(result, decoded_audio_size, input) &&
                !skip_eos_append) {
       DCHECK_EQ(packet.size, 0);
-      // Create an end of stream output buffer.
-      output = new DataBuffer(0);
+      output = DataBuffer::CreateEOSBuffer();
     }
 
     if (output) {
diff --git a/media/filters/ffmpeg_audio_decoder_unittest.cc b/media/filters/ffmpeg_audio_decoder_unittest.cc
index 605e007..bb1ed39 100644
--- a/media/filters/ffmpeg_audio_decoder_unittest.cc
+++ b/media/filters/ffmpeg_audio_decoder_unittest.cc
@@ -108,8 +108,6 @@ class FFmpegAudioDecoderTest : public testing::Test {
 
   void ExpectEndOfStream(size_t i) {
     EXPECT_LT(i, decoded_audio_.size());
-    EXPECT_EQ(0, decoded_audio_[i]->GetTimestamp().InMicroseconds());
-    EXPECT_EQ(0, decoded_audio_[i]->GetDuration().InMicroseconds());
     EXPECT_TRUE(decoded_audio_[i]->IsEndOfStream());
   }
 
diff --git a/media/filters/opus_audio_decoder.cc b/media/filters/opus_audio_decoder.cc
index 0e7e89f..a94a38c 100644
--- a/media/filters/opus_audio_decoder.cc
+++ b/media/filters/opus_audio_decoder.cc
@@ -542,7 +542,8 @@ bool OpusAudioDecoder::Decode(const scoped_refptr<DecoderBuffer>& input,
 
   if (decoded_audio_size > 0) {
     // Copy the audio samples into an output buffer.
-    *output_buffer = new DataBuffer(decoded_audio_data, decoded_audio_size);
+    *output_buffer = DataBuffer::CopyFrom(
+        decoded_audio_data, decoded_audio_size);
     (*output_buffer)->SetTimestamp(output_timestamp_helper_->GetTimestamp());
     (*output_buffer)->SetDuration(
         output_timestamp_helper_->GetDuration(decoded_audio_size));