Implement simple garbage collection in SourceBufferStream

Implement simple garbage collection in SourceBufferStream

Start freeing buffers after Append() calls if total size of the stream goes
above a hard cap. This caps per-stream, not per-video tag.

BUG=125070
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150201

[vrk (LEFT CHROMIUM), 2012-08-04 01:32:31]

Here's the first stab!!

If there's no |selected_range_|, the GC will just start deleting from the
beginning of the range. This probably isn't what we want to do, but it's the
simplest thing to do.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:263: static int kDefaultMemoryLimit = 20 *
1024 * 1024;
Not sure what I should actually set this to.

Also, this sets a cap on the stream, but not the video tag, so if there are 3
streams then they could potentially take up 3 * kDefaultMemoryLimit.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
File media/filters/source_buffer_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1759: NewSegmentAppend(15, 1,
&kDataA);
D'oh, this actually fails right now because of a bug with
|waiting_for_keyframe_|. Will investigate Monday...

[acolwell GONE FROM CHROMIUM, 2012-08-06 16:44:51]

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:263: static int kDefaultMemoryLimit = 20 *
1024 * 1024;
On 2012/08/04 01:32:31, Victoria Kirst wrote:
> Not sure what I should actually set this to.
> 
> Also, this sets a cap on the stream, but not the video tag, so if there are 3
> streams then they could potentially take up 3 * kDefaultMemoryLimit.
I think we should have audio & video specific limits. I believe 12MB should be
enough for ~5 minutes of audio @ 320Kbps. 150MB appears to be enough for ~5
minutes of 4Mbps content which appears to be what YouTube's 1080p content is
encoded at. I'm fine w/ halving these values if you think they are too big.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1130: int end_index = start_index +
buffers_.size();
Is this right? I'm assuming this is trying to specify deleting all the buffers.
Shouldn't it be end_index = buffers_.size() - 1?

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1135: // GOP is deleted. However, the
distances between keyframes are the same, so
nit: Consider rewording "the distances between keyframes are the same" since it
implied to me a constant keyframe interval which we definitely don't want to
assume. I believe you are trying to indicate that the relative distance between
keyframes doesn't change so you can apply a constant offset to all of the
entries to reflect the deleted buffers. Is that correct?

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1138: for (int i = start_index; i <
end_index; i++) {
I think it might be clearer if you changed this to a 0-buffers_to_delete loop
and just use the indices for computing buffers_to_delete above. I think that can
help ease worry about the fact that the data in keyframe_map_ becomes
temporarily invalid and prevents people from accidentally relying on the index
values. Putting the comment above next to a buffers_to_delete = front->second -
start_index; makes it much clearer you only care about the relative relationship
instead of the absolute values.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
File media/filters/source_buffer_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1612: CheckNoNextBuffer();
nit: Please extend this test to also include a limit larger than one (3?) just
to make sure we handle the 1 buffer & > 1 case.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1667: Seek(5);
nit: Add a CheckNoNextBuffer() before the seek?

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1740: CheckExpectedBuffers(10,
14, &kDataA);
Put a CheckExpectedRanges() here just to make it easier to see how the ranges
evolve.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1754: // buffers closest to the
seek position.
[10,10) is kept because the code assumes that position 15 is in that range?

[vrk (LEFT CHROMIUM), 2012-08-06 20:27:14]

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:263: static int kDefaultMemoryLimit = 20 *
1024 * 1024;
On 2012/08/06 16:44:52, acolwell wrote:
> On 2012/08/04 01:32:31, Victoria Kirst wrote:
> > Not sure what I should actually set this to.
> > 
> > Also, this sets a cap on the stream, but not the video tag, so if there are
3
> > streams then they could potentially take up 3 * kDefaultMemoryLimit.
> I think we should have audio & video specific limits. I believe 12MB should be
> enough for ~5 minutes of audio @ 320Kbps. 150MB appears to be enough for ~5
> minutes of 4Mbps content which appears to be what YouTube's 1080p content is
> encoded at. I'm fine w/ halving these values if you think they are too big.
> 

Done.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1130: int end_index = start_index +
buffers_.size();
On 2012/08/06 16:44:52, acolwell wrote:
> Is this right? I'm assuming this is trying to specify deleting all the
buffers.

Yes and yes. 

> Shouldn't it be end_index = buffers_.size() - 1?
No. It's not obvious and that's actually what I had at first, but unit-testing
caught the bug :)

Here's an example. Say there are the following buffers:

0 - 0 KEYFRAME
1 - 33
2 - 66
3 - 100 KEYFRAME
4 - 133
5 - 166

Imagine we've deleted 0 through 2. Then what we have is:
3 - 100 KEYFRAME
4 - 133
5 - 166

Note that |keyframe_map_| has not been updated, so index 3 in |keyframe_map_|
refers to t=100 still. So start index is 3, and buffers.size() is 3, so you need
offset the buffers.size() by the start_index to get 6. The for loop goes from i
= 3 to i < 6, so start_index + buffers.size() is the proper expression.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1135: // GOP is deleted. However, the
distances between keyframes are the same, so
On 2012/08/06 16:44:52, acolwell wrote:
> nit: Consider rewording "the distances between keyframes are the same" since
it
> implied to me a constant keyframe interval which we definitely don't want to
> assume.
Good point! Upon re-reading this morning, that's exactly what it sounds like.

> I believe you are trying to indicate that the relative distance between
> keyframes doesn't change 
Yes.

> so you can apply a constant offset to all of the
> entries to reflect the deleted buffers. Is that correct?

So that's true too, but the reason why I had the comment here was to attempt to
explain the for-loop bounds, i.e. to explain that we go from 3 to 6 instead of 0
to 3 (to use the example I gave above).

Looks like it was still confusing though! I changed the logic a bit and changed
the comment, so hopefully should be clearer what's going on.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream.cc:1138: for (int i = start_index; i <
end_index; i++) {
On 2012/08/06 16:44:52, acolwell wrote:
> I think it might be clearer if you changed this to a 0-buffers_to_delete loop
> and just use the indices for computing buffers_to_delete above. I think that
can
> help ease worry about the fact that the data in keyframe_map_ becomes
> temporarily invalid and prevents people from accidentally relying on the index
> values. Putting the comment above next to a buffers_to_delete = front->second
-
> start_index; makes it much clearer you only care about the relative
relationship
> instead of the absolute values.

Made the loop 0-based but kept end_index; let me know if you think it's still
unclear!

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
File media/filters/source_buffer_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1612: CheckNoNextBuffer();
On 2012/08/06 16:44:52, acolwell wrote:
> nit: Please extend this test to also include a limit larger than one (3?) just
> to make sure we handle the 1 buffer & > 1 case.

Instead of extending this test, I added a second DeleteFrontAndBack test, and I
added a GarbageCollection_TrackBuffer test to cover what happened when I tried
to extend the DeleteFrontAndBack test :)

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1667: Seek(5);
On 2012/08/06 16:44:52, acolwell wrote:
> nit: Add a CheckNoNextBuffer() before the seek?

Done.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1740: CheckExpectedBuffers(10,
14, &kDataA);
On 2012/08/06 16:44:52, acolwell wrote:
> Put a CheckExpectedRanges() here just to make it easier to see how the ranges
> evolve.

Done.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1754: // buffers closest to the
seek position.
On 2012/08/06 16:44:52, acolwell wrote:
> [10,10) is kept because the code assumes that position 15 is in that range?

Yes, though this logic will probably change when I address the bug below.

https://chromiumcodereview.appspot.com/10853013/diff/5001/media/filters/sourc...
media/filters/source_buffer_stream_unittest.cc:1759: NewSegmentAppend(15, 1,
&kDataA);
On 2012/08/04 01:32:31, Victoria Kirst wrote:
> D'oh, this actually fails right now because of a bug with
> |waiting_for_keyframe_|. Will investigate Monday...

Added DISABLED_ prefix + bug for now.

[acolwell GONE FROM CHROMIUM, 2012-08-06 20:48:01]

LGTM

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
media/filters/source_buffer_stream.cc:1139: // is deleted, so adjust the
|end_index| by the |buffers_deleted| to get
nit: s/end_index/front->second/ since that is what actually comes out of
keyframe_map_?

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
File media/filters/source_buffer_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
media/filters/source_buffer_stream_unittest.cc:1766: // Add 5 bufers from
position 20 to 24.
s/bufers/buffers

[vrk (LEFT CHROMIUM), 2012-08-06 20:52:46]

Thanks Aaron!

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
File media/filters/source_buffer_stream.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
media/filters/source_buffer_stream.cc:1139: // is deleted, so adjust the
|end_index| by the |buffers_deleted| to get
On 2012/08/06 20:48:02, acolwell wrote:
> nit: s/end_index/front->second/ since that is what actually comes out of
> keyframe_map_?

Done.

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
File media/filters/source_buffer_stream_unittest.cc (right):

https://chromiumcodereview.appspot.com/10853013/diff/10001/media/filters/sour...
media/filters/source_buffer_stream_unittest.cc:1766: // Add 5 bufers from
position 20 to 24.
On 2012/08/06 20:48:02, acolwell wrote:
> s/bufers/buffers

Done.