Change ChunkDemuxerStream/SourceBufferStream memory limit to size_t type

media/filters/source_buffer_range.cc

 // Copyright 2014 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/filters/source_buffer_range.h"
 
 #include <algorithm>
 
 namespace media {
 
@@ skipping 38 matching lines @@
          media_segment_start_time_ <=
              new_buffers.front()->GetDecodeTimestamp());
 
   AdjustEstimatedDurationForNewAppend(new_buffers);
 
   for (BufferQueue::const_iterator itr = new_buffers.begin();
        itr != new_buffers.end();
        ++itr) {
     DCHECK((*itr)->GetDecodeTimestamp() != kNoDecodeTimestamp());
     buffers_.push_back(*itr);
     size_in_bytes_ += (*itr)->data_size();

[wolenetz, 2015/07/20 19:29:42]

nit: ISTM the DCHECK would be good here, too.

Fixing StreamParserBuffer::data_size() to be a size_t would be a next step,
though I'm fine if that's a separate CL since StreamParserBuffer is involved in
many many places in our pipeline.

Please add a TODO in @ DecoderBuffer::data_size() and side_data_size() to make
them size_t and rework any negative value signalling (if any) to some other
mechanism. IIUC, per the comments in decoder_buffer.h, these values shouldn't
ever be negative, but we'd need to verify that contract has been adhered to, and
also make sure users of DB::*_data_size() work with size_t.

[servolk, 2015/07/22 19:02:11]

Yes, DecoderBuffer::Initialize already has CHECK_GE(size_, 0), so I think it
should be safe to change DecoderBuffer::size_ and
StreamParserBuffer::data_size() type to size_t as well. But I agree, it's
probably better done as a separate CL, as it would require updating a lot more
places. I've added a TODO.

 
     if ((*itr)->is_key_frame()) {
       keyframe_map_.insert(
           std::make_pair((*itr)->GetDecodeTimestamp(),
                          buffers_.size() - 1 + keyframe_map_index_base_));
     }
   }
 }
 
 void SourceBufferRange::AdjustEstimatedDurationForNewAppend(
@@ skipping 148 matching lines @@
 
 bool SourceBufferRange::TruncateAt(
     DecodeTimestamp timestamp, BufferQueue* removed_buffers,
     bool is_exclusive) {
   // Find the place in |buffers_| where we will begin deleting data.
   BufferQueue::iterator starting_point =
       GetBufferItrAt(timestamp, is_exclusive);
   return TruncateAt(starting_point, removed_buffers);
 }
 
-int SourceBufferRange::DeleteGOPFromFront(BufferQueue* deleted_buffers) {
+size_t SourceBufferRange::DeleteGOPFromFront(BufferQueue* deleted_buffers) {
   DCHECK(!FirstGOPContainsNextBufferPosition());
   DCHECK(deleted_buffers);
 
   int buffers_deleted = 0;
-  int total_bytes_deleted = 0;
+  size_t total_bytes_deleted = 0;
 
   KeyframeMap::iterator front = keyframe_map_.begin();
   DCHECK(front != keyframe_map_.end());
 
   // Delete the keyframe at the start of |keyframe_map_|.
   keyframe_map_.erase(front);
 
   // Now we need to delete all the buffers that depend on the keyframe we've
   // just deleted.
   int end_index = keyframe_map_.size() > 0 ?
       keyframe_map_.begin()->second - keyframe_map_index_base_ :
       buffers_.size();
 
   // Delete buffers from the beginning of the buffered range up until (but not
   // including) the next keyframe.
   for (int i = 0; i < end_index; i++) {
-    int bytes_deleted = buffers_.front()->data_size();
+    DCHECK_GE(buffers_.front()->data_size(), 0);
+    size_t bytes_deleted = buffers_.front()->data_size();
+    DCHECK_GE(size_in_bytes_, bytes_deleted);
     size_in_bytes_ -= bytes_deleted;
     total_bytes_deleted += bytes_deleted;
     deleted_buffers->push_back(buffers_.front());
     buffers_.pop_front();
     ++buffers_deleted;
   }
 
   // Update |keyframe_map_index_base_| to account for the deleted buffers.
   keyframe_map_index_base_ += buffers_deleted;
 
   if (next_buffer_index_ > -1) {
     next_buffer_index_ -= buffers_deleted;
     DCHECK_GE(next_buffer_index_, 0);
   }
 
   // Invalidate media segment start time if we've deleted the first buffer of
   // the range.
   if (buffers_deleted > 0)
     media_segment_start_time_ = kNoDecodeTimestamp();
 
   return total_bytes_deleted;
 }
 
-int SourceBufferRange::DeleteGOPFromBack(BufferQueue* deleted_buffers) {
+size_t SourceBufferRange::DeleteGOPFromBack(BufferQueue* deleted_buffers) {
   DCHECK(!LastGOPContainsNextBufferPosition());
   DCHECK(deleted_buffers);
 
   // Remove the last GOP's keyframe from the |keyframe_map_|.
   KeyframeMap::iterator back = keyframe_map_.end();
   DCHECK_GT(keyframe_map_.size(), 0u);
   --back;
 
   // The index of the first buffer in the last GOP is equal to the new size of
   // |buffers_| after that GOP is deleted.
   size_t goal_size = back->second - keyframe_map_index_base_;
   keyframe_map_.erase(back);
 
-  int total_bytes_deleted = 0;
+  size_t total_bytes_deleted = 0;
   while (buffers_.size() != goal_size) {
-    int bytes_deleted = buffers_.back()->data_size();
+    DCHECK_GE(buffers_.back()->data_size(), 0);
+    size_t bytes_deleted = buffers_.back()->data_size();
+    DCHECK_GE(size_in_bytes_, bytes_deleted);
     size_in_bytes_ -= bytes_deleted;
     total_bytes_deleted += bytes_deleted;
     // We're removing buffers from the back, so push each removed buffer to the
     // front of |deleted_buffers| so that |deleted_buffers| are in nondecreasing
     // order.
     deleted_buffers->push_front(buffers_.back());
     buffers_.pop_back();
   }
 
   return total_bytes_deleted;
 }
 
-int SourceBufferRange::GetRemovalGOP(
+size_t SourceBufferRange::GetRemovalGOP(
     DecodeTimestamp start_timestamp, DecodeTimestamp end_timestamp,
-    int total_bytes_to_free, DecodeTimestamp* removal_end_timestamp) {
-  int bytes_to_free = total_bytes_to_free;
-  int bytes_removed = 0;
+    size_t total_bytes_to_free, DecodeTimestamp* removal_end_timestamp) {
+  size_t bytes_removed = 0;
 
   KeyframeMap::iterator gop_itr = GetFirstKeyframeAt(start_timestamp, false);
   if (gop_itr == keyframe_map_.end())
     return 0;
   int keyframe_index = gop_itr->second - keyframe_map_index_base_;
   BufferQueue::iterator buffer_itr = buffers_.begin() + keyframe_index;
   KeyframeMap::iterator gop_end = keyframe_map_.end();
   if (end_timestamp < GetBufferedEndTimestamp())
     gop_end = GetFirstKeyframeAtOrBefore(end_timestamp);
 
   // Check if the removal range is within a GOP and skip the loop if so.
   // [keyframe]...[start_timestamp]...[end_timestamp]...[keyframe]
   KeyframeMap::iterator gop_itr_prev = gop_itr;
   if (gop_itr_prev != keyframe_map_.begin() && --gop_itr_prev == gop_end)
     gop_end = gop_itr;
 
-  while (gop_itr != gop_end && bytes_to_free > 0) {
+  while (gop_itr != gop_end && bytes_removed < total_bytes_to_free) {
     ++gop_itr;
 
-    int gop_size = 0;
+    size_t gop_size = 0;
     int next_gop_index = gop_itr == keyframe_map_.end() ?
         buffers_.size() : gop_itr->second - keyframe_map_index_base_;
     BufferQueue::iterator next_gop_start = buffers_.begin() + next_gop_index;
-    for (; buffer_itr != next_gop_start; ++buffer_itr)
+    for (; buffer_itr != next_gop_start; ++buffer_itr) {
+      DCHECK_GE((*buffer_itr)->data_size(), 0);
       gop_size += (*buffer_itr)->data_size();
+    }
 
     bytes_removed += gop_size;
-    bytes_to_free -= gop_size;
   }
   if (bytes_removed > 0) {
     *removal_end_timestamp = gop_itr == keyframe_map_.end() ?
         GetBufferedEndTimestamp() : gop_itr->first;
   }
   return bytes_removed;
 }
 
 bool SourceBufferRange::FirstGOPContainsNextBufferPosition() const {
   if (!HasNextBufferPosition())
@@ skipping 19 matching lines @@
   KeyframeMap::const_iterator last_gop = keyframe_map_.end();
   --last_gop;
   return last_gop->second - keyframe_map_index_base_ <= next_buffer_index_;
 }
 
 void SourceBufferRange::FreeBufferRange(
     const BufferQueue::iterator& starting_point,
     const BufferQueue::iterator& ending_point) {
   for (BufferQueue::iterator itr = starting_point;
        itr != ending_point; ++itr) {
-    size_in_bytes_ -= (*itr)->data_size();
-    DCHECK_GE(size_in_bytes_, 0);
+    DCHECK_GE((*itr)->data_size(), 0);

[ddorwin, 2015/07/20 21:23:57]

Should BufferQueue also be fixed in a separate CL?

[servolk, 2015/07/22 19:02:11]

BufferQueue is just a typedef for a collection of StreamParserBuffers. As
wolenetz@ noted above what needs to be fixed is /DecoderBuffer. I've added a
TODO for that.

+    size_t itr_data_size = static_cast<size_t>((*itr)->data_size());

[ddorwin, 2015/07/20 21:23:57]

Does this value always come from internal code or do we need to use checked_cast
for security?

[servolk, 2015/07/22 19:02:11]

It comes from StreamParserBuffer::data_size.
I think it should be safe to do just static_cast here, given there's already
CHECK_GE(size_, 0) in DecoderBuffer::Initialize and DecoderBuffer::size_ is only
set in constructors.

+    DCHECK_GE(size_in_bytes_, itr_data_size);
+    size_in_bytes_ -= itr_data_size;
   }
   buffers_.erase(starting_point, ending_point);
 }
 
 bool SourceBufferRange::TruncateAt(
     const BufferQueue::iterator& starting_point, BufferQueue* removed_buffers) {
   DCHECK(!removed_buffers || removed_buffers->empty());
 
   // Return if we're not deleting anything.
   if (starting_point == buffers_.end())
@@ skipping 220 matching lines @@
     }
 
     if (buffer->timestamp() + buffer->duration() <= start.ToPresentationTime())
       continue;
     buffers->push_back(buffer);
   }
   return previous_size < buffers->size();
 }
 
 }  // namespace media