Add Chromium-side support for SourceBuffer.appendWindowStart and SourceBuffer.appendWindowEnd.

Add Chromium-side support for SourceBuffer.appendWindowStart and SourceBuffer.appendWindowEnd.

BUG=229408
TEST=ChunkDemuxerTest.AppendWindow

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

[acolwell GONE FROM CHROMIUM, 2013-07-24 21:01:25]

Note: The StreamParser changes are needed to handle discontinuities created by
the append window. You need to be able to see both the all the audio & video
buffers in a single append to properly compute the actual start of the
discontinuity. I'm intentionally ignoring text tracks in this CL because more
invasive changes will be needed to fix them.

[scherkus (not reviewing), 2013-07-24 22:47:41]

looking good, mostly nits 'n q's

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:65: void SetAppendWindowStart(TimeDelta start) {
append_window_start_ = start; }
these should be unix_hacker style as they're inlined simple setters

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:249: append_window_start_(TimeDelta()),
do you have this here to make it more explicit?

it's technically not needed as the default ctor is used anyway

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:306: can_update_offset_ = true;
sanity check: does new_media_segment_ need to be updated here?

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:387: void
SourceState::OnNewMediaSegment(TimeDelta timestamp) {
|timestamp| isn't used anymore other than for DCHECK/DVLOG

time to remove?

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:417: // Find the earliest timestamp in the
filtered buffers and use
nit: you can fit a few more words on this line

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:430: new_media_segment_ = false;
sanity check: does can_update_offset_ need to be updated here?

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:461: StreamParser::BufferQueue
SourceState::FilterWithAppendWindow(
do we know how large this queue can get?

any reason not to use an out-param to avoid copying the std::deque and
ref+deref?


(I was intrigued by this question and did a quick benchmark... on release build
copying a std::deque of scoped_refptr<T*> vs T* is ~3x slower, but I was using
very large std::deques... but still!)

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:493: new_media_segment_ = true;
sanity check: does can_update_offset_ need to be updated here?

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:1383: DVLOG(2) <<
"ChunkDemuxer::OnNewMediaSegment(" << source_id << ", "
FYI the DVLOGs in this class are inconsistent re: class name (e.g., Shutdown())

was this an intentional change? should we remove all the CD:: or add CD::?

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:319: std::string timestampStr =
timestamps[i];
unix_hacker for timestampStr

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:335: scoped_ptr<Cluster>
cluster(cb.Finish());
allocate on stack?

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:760: void
CheckExpectedBuffers(DemuxerStream* stream, const std::string expected) {
missing & From std::string

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:2721: // Set the append window to
[20,280)
period

https://codereview.chromium.org/20123002/diff/9002/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/20123002/diff/9002/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:535: if (audio_buffers->empty() &&
video_buffers->empty())
OOC is there any harm in running the cb w/ empty queues?

[acolwell GONE FROM CHROMIUM, 2013-07-25 20:39:33]

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:65: void SetAppendWindowStart(TimeDelta start) {
append_window_start_ = start; }
On 2013/07/24 22:47:41, scherkus wrote:
> these should be unix_hacker style as they're inlined simple setters

Done.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:249: append_window_start_(TimeDelta()),
On 2013/07/24 22:47:41, scherkus wrote:
> do you have this here to make it more explicit?
> 
> it's technically not needed as the default ctor is used anyway 
Yeah that was the original idea, but it's a pretty lame reason. Removed.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:306: can_update_offset_ = true;
On 2013/07/24 22:47:41, scherkus wrote:
> sanity check: does new_media_segment_ need to be updated here?
No. We will always get an OnNewMediaSegment() call before any OnNewBuffers()
call that occurs after this function.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:387: void
SourceState::OnNewMediaSegment(TimeDelta timestamp) {
On 2013/07/24 22:47:41, scherkus wrote:
> |timestamp| isn't used anymore other than for DCHECK/DVLOG
> 
> time to remove?

Done.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:417: // Find the earliest timestamp in the
filtered buffers and use
On 2013/07/24 22:47:41, scherkus wrote:
> nit: you can fit a few more words on this line

Done.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:430: new_media_segment_ = false;
On 2013/07/24 22:47:41, scherkus wrote:
> sanity check: does can_update_offset_ need to be updated here?
No. can_update_offset_ tracks the actual media segments being appended. The spec
says to reject timestamp offset changes when the application is in the middle of
appending a media segment.

new_media_segment_ essentially keeps track of the beginning of bytestream media
segments as well as what the spec calls a "coded frame group". I'm just using
"media segment" here because historically there wasn't a difference between
"media segments" and "coded frame groups." Now because of the append window,
multiple "coded frame groups" can be created within a single "media segment". 

If it is ok, I'd prefer to untangle the meanings of "media segment" and "coded
frame group" here and in SourceBufferStream in a followup CL. SourceState should
care about media segments, but SourceBufferStream should only care about "coded
frame groups". I've mixed the concepts here so that SourceBufferStream is
getting all the calls it was getting before, plus a few additional ones caused
by the append window.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:461: StreamParser::BufferQueue
SourceState::FilterWithAppendWindow(
On 2013/07/24 22:47:41, scherkus wrote:
> do we know how large this queue can get?
> 
> any reason not to use an out-param to avoid copying the std::deque and
> ref+deref?
Nope. Done.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:493: new_media_segment_ = true;
On 2013/07/24 22:47:41, scherkus wrote:
> sanity check: does can_update_offset_ need to be updated here?
No. This is a "virtual" media segment (i.e., a new coded frame group) within the
current media segment. can_update_offset_ should be and remain false here
because we are still within the bytestream media segment.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer.cc:1383: DVLOG(2) <<
"ChunkDemuxer::OnNewMediaSegment(" << source_id << ", "
On 2013/07/24 22:47:41, scherkus wrote:
> FYI the DVLOGs in this class are inconsistent re: class name (e.g.,
Shutdown())
> 
> was this an intentional change? should we remove all the CD:: or add CD::?
Actually this method doesn't need to exist anymore. I've merged the body into
SourceState::OnNewBuffers() since that is the only place this method was being
called from via the new_segment_cb_. Removed all new_segment_cb_ code as well.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:319: std::string timestampStr =
timestamps[i];
On 2013/07/24 22:47:41, scherkus wrote:
> unix_hacker for timestampStr

Done.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:335: scoped_ptr<Cluster>
cluster(cb.Finish());
On 2013/07/24 22:47:41, scherkus wrote:
> allocate on stack?
That would actually require changing the ClusterBuilder::Finish() return value
which would effect a bunch of other tests.

Cluster should just be replaced with std::vector<uint8> everywhere. I don't
really get why I didn't do that to begin with. I'll do that in a follow-up CL.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:760: void
CheckExpectedBuffers(DemuxerStream* stream, const std::string expected) {
On 2013/07/24 22:47:41, scherkus wrote:
> missing & From std::string

Done.

https://codereview.chromium.org/20123002/diff/9002/media/filters/chunk_demuxe...
media/filters/chunk_demuxer_unittest.cc:2721: // Set the append window to
[20,280)
On 2013/07/24 22:47:41, scherkus wrote:
> period

Done.

https://codereview.chromium.org/20123002/diff/9002/media/mp4/mp4_stream_parse...
File media/mp4/mp4_stream_parser.cc (right):

https://codereview.chromium.org/20123002/diff/9002/media/mp4/mp4_stream_parse...
media/mp4/mp4_stream_parser.cc:535: if (audio_buffers->empty() &&
video_buffers->empty())
On 2013/07/24 22:47:41, scherkus wrote:
> OOC is there any harm in running the cb w/ empty queues?
No. It just violates the contract for the NewBuffersCB. It is expected to be
called with at least 1 buffer. I can lift this restriction, but it means we
might not detect badly written parsers that just run the callback w/o any data
all the time.

[scherkus (not reviewing), 2013-07-25 20:56:45]

lgtm

follow-up CLs = sgtm

[commit-bot: I haz the power, 2013-07-26 02:21:14]

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

[commit-bot: I haz the power, 2013-07-26 02:22:47]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-07-26 03:59:24]

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

[commit-bot: I haz the power, 2013-07-26 06:39:51]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-26 13:53:41]

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

[commit-bot: I haz the power, 2013-07-26 20:42:59]

Change committed as 213982