Fix various operations in ChunkDemuxer that were not being applied to text tracks.

Fix various operations in ChunkDemuxer that were not being applied to text tracks.

- Fixed range removal.
- Fixed shutdown on error.
- Fixed memory limit setting for testing.
- Cleaned up waiting for seek logic and documented behavior.

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

[acolwell GONE FROM CHROMIUM, 2013-12-27 19:31:00]

Please take a look. I'm still working on the tests to verify these changes, but
I wanted to make sure you were ok with these all being in a single patch. The
changes are pretty trivial, orthogonal, and the resolution of each is
practically identical so I felt like it made sense to just solve them all at
once.

[xhwang, 2013-12-28 01:06:03]

looking good % nits, I'll wait for the test to sign off it.

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:120: base::TimeDelta duration);
Here and in this file, pass base::TimeDelta by const-ref? I see both patterns in
media/ code. base::TimeDelta is small so this is not a big deal, but we
shouldn't make assumptions on the implementation detail of base::TimeDelta.

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:149: void SetMemoryLimitsForTesting(int
memory_limit);
Is |memory_limit| in KB or MB?

[acolwell GONE FROM CHROMIUM, 2013-12-28 02:17:10]

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:120: base::TimeDelta duration);
On 2013/12/28 01:06:03, xhwang wrote:
> Here and in this file, pass base::TimeDelta by const-ref? I see both patterns
in
> media/ code. base::TimeDelta is small so this is not a big deal, but we
> shouldn't make assumptions on the implementation detail of base::TimeDelta.

I've talked with scherkus@ and fischman@ about this before and it basically
comes down to the fact that we wouldn't use const& if TimeDelta was 'typedef
int64 TimeDelta' instead of a class. It is only a class because of the
conversion functions and it is unlikely TimeDelta is going to change to a more
complex implementation. I've also tended to use MessageLoop::PostDelayedTask()
and friends as an example of non-const& usage in critical code. We should
probably have a team discussion about this along the lines of the EXPECT_EQ
discussion from before.

For now, I'm just trying to match the style of the rest of this file. I'll
probably s/base::TimeDelta/TimeDelta/ to further blend in though.

[xhwang, 2013-12-30 17:53:38]

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:120: base::TimeDelta duration);
On 2013/12/28 02:17:10, acolwell wrote:
> On 2013/12/28 01:06:03, xhwang wrote:
> > Here and in this file, pass base::TimeDelta by const-ref? I see both
patterns
> in
> > media/ code. base::TimeDelta is small so this is not a big deal, but we
> > shouldn't make assumptions on the implementation detail of base::TimeDelta.
> 
> I've talked with scherkus@ and fischman@ about this before and it basically
> comes down to the fact that we wouldn't use const& if TimeDelta was 'typedef
> int64 TimeDelta' instead of a class. It is only a class because of the
> conversion functions and it is unlikely TimeDelta is going to change to a more
> complex implementation. I've also tended to use MessageLoop::PostDelayedTask()
> and friends as an example of non-const& usage in critical code. We should
> probably have a team discussion about this along the lines of the EXPECT_EQ
> discussion from before.
> 
> For now, I'm just trying to match the style of the rest of this file. I'll
> probably s/base::TimeDelta/TimeDelta/ to further blend in though.

sg. thanks for the explanation.

[acolwell GONE FROM CHROMIUM, 2014-01-08 02:16:49]

PTAL. I've added tests for the Remove() & Shutdown() changes and added a test to
explicitly verify the SeekWaitingForData  behavior. I didn't add anything for
the memory limit change since it is pretty straight forward and I don't think it
is worth adding a test for it.

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/110693007/diff/50001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:149: void SetMemoryLimitsForTesting(int
memory_limit);
On 2013/12/28 01:06:03, xhwang wrote:
> Is |memory_limit| in KB or MB?
It's bytes. I've added a comment clarifying this here and elsewhere.

[xhwang, 2014-01-08 19:03:26]

Tests look good. Just a few comments.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:468: 
nit: drop an extra empty line?

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1063: TEST_F(ChunkDemuxerTest,
Shutdown_BeforeAllInitSegmentsAppended) {
In this test and the next one, it's not clear how Shutdown() happens. Add a
comment at least?

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1072:
AppendInitSegmentWithSourceId("audio", true, false, false);
Can we also use the new HAS_VIDEO | HAS_XXX pattern for
AppendInitSegmentWithSourceId?

The same applies to CreateInitSegment I guess.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1087:
AppendInitSegmentWithSourceId("video", false, true, true);
s/video/video_and_text?

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1090: TEST_F(ChunkDemuxerTest,
Shutdown_AllPendingReadsEOS) {
This name is ambiguous. It reads like "shutdown during pending EOS reads". But
really it's testing that when we shutdown during pending reads, the reads will
be aborted with EOS (correct me if I misunderstand). I don't have a better name,
but at least it should match EndOfStreamWhileWaitingForGapToBeFilled somehow.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:2995: 
CheckExpectedBuffers here to make sure the buffers are removed (not overwritten
by the newly appended buffers below?

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:3066: }
I like this test!

[acolwell GONE FROM CHROMIUM, 2014-01-09 00:21:13]

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:468: 
On 2014/01/08 19:03:26, xhwang wrote:
> nit: drop an extra empty line?

Done.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1063: TEST_F(ChunkDemuxerTest,
Shutdown_BeforeAllInitSegmentsAppended) {
On 2014/01/08 19:03:26, xhwang wrote:
> In this test and the next one, it's not clear how Shutdown() happens. Add a
> comment at least?
I've added explicit ShutdownDemuxer() calls to make this clear.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1072:
AppendInitSegmentWithSourceId("audio", true, false, false);
On 2014/01/08 19:03:26, xhwang wrote:
> Can we also use the new HAS_VIDEO | HAS_XXX pattern for
> AppendInitSegmentWithSourceId?
> 
> The same applies to CreateInitSegment I guess.
Done. This resulted in a fair amount of changes, but they are pretty trivial.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1087:
AppendInitSegmentWithSourceId("video", false, true, true);
On 2014/01/08 19:03:26, xhwang wrote:
> s/video/video_and_text?
These are just ID names so they don't really matter. I've updated it here and
above.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:1090: TEST_F(ChunkDemuxerTest,
Shutdown_AllPendingReadsEOS) {
On 2014/01/08 19:03:26, xhwang wrote:
> This name is ambiguous. It reads like "shutdown during pending EOS reads". But
> really it's testing that when we shutdown during pending reads, the reads will
> be aborted with EOS (correct me if I misunderstand). I don't have a better
name,
> but at least it should match EndOfStreamWhileWaitingForGapToBeFilled somehow.
Changed the name and added a comment.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:2995: 
On 2014/01/08 19:03:26, xhwang wrote:
> CheckExpectedBuffers here to make sure the buffers are removed (not
overwritten
> by the newly appended buffers below?
I can't use CheckExpectedBuffers here because there aren't any buffers and the
reads will just wait for more data. I've added a CheckExpectedRanges("{ }") to
verify that the source buffer is actually empty though.

https://codereview.chromium.org/110693007/diff/160001/media/filters/chunk_dem...
media/filters/chunk_demuxer_unittest.cc:3066: }
On 2014/01/08 19:03:26, xhwang wrote:
> I like this test!
thanks. :)

[xhwang, 2014-01-09 00:24:26]

lgtm, thanks!

[commit-bot: I haz the power, 2014-01-09 00:33:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/110693007/280001

[commit-bot: I haz the power, 2014-01-09 04:13:08]

Change committed as 243756