Add plumbing and support for crossfading StreamParserBuffers.

Add plumbing and support for crossfading StreamParserBuffers.

Per the MSE specification adds a fade-out section to StreamParserBuffer
which will contain buffers for crossfading.

Modifies SourceBufferStream::GetNextBuffer() to properly return these
buffers with a config change in between the fade out and fade in
sections.

BUG=334493
TEST=New unittests.
NOTRY=true

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

[DaleCurtis, 2014-01-06 23:26:43]

Preliminary implementation.  Does not yet implement the functionality for
populating the fade-out section of the buffer.  Please provide a design review
before things get ugly in Append() and Remove() :)

I'm concerned that using |track_buffer_| may be problematic.  It was originally
modified only by Append() and DCHECK'd to be empty in a lot of places which
expected it to only be modified their...

[acolwell GONE FROM CHROMIUM, 2014-01-07 18:09:08]

On 2014/01/06 23:26:43, DaleCurtis wrote:
> Preliminary implementation.  Does not yet implement the functionality for
> populating the fade-out section of the buffer.  Please provide a design review
> before things get ugly in Append() and Remove() :)
> 
> I'm concerned that using |track_buffer_| may be problematic.  It was
originally
> modified only by Append() and DCHECK'd to be empty in a lot of places which
> expected it to only be modified their...

I think your life would be easier if you handled the fade in buffers
"downstream" of the existing logic. Conceptually, you could accomplish this by
simply renaming the existing GetNextBuffer() and implementing a new version that
manages the slot and an index into the fade-in buffers. It would then use the
existing logic to fetch the next buffer once all the fade-in buffers and buffer
in the slot have been returned. Then all you have to worry about is clearing the
slot and index when a seek occurs. You might even consider implementing this in
ChunkDemuxerStream because this little dance isn't really about the
SourceBuffer, but how we are conveying the splice to the audio renderer.

[DaleCurtis, 2014-01-08 00:14:12]

Sounds good, I'm all for keeping the inside of ChunkDemuxer(Stream)  However in
a previous conversation I thought you had mentioned a preference for keeping the
code inside SourceBufferStream.  Ditto on yesterday's conversation around
re-using |track_buffer_|.

[acolwell GONE FROM CHROMIUM, 2014-01-08 02:03:20]

On 2014/01/08 00:14:12, DaleCurtis wrote:
> Sounds good, I'm all for keeping the inside of ChunkDemuxer(Stream)  However
in
> a previous conversation I thought you had mentioned a preference for keeping
the
> code inside SourceBufferStream.
I'm on the fence about this one. I think it just depends on which looks cleaner
to you. SourceBufferStream essentially manages the "track buffer" behavior, in
the MSE spec sense (ie the buffered data for an individual track in the
SourceBuffer). Technically these special StreamParserBuffers with the extra
fields represent the "audio splice frame" and SourceBufferStream is just handing
one of those out. How this is conveyed to the rest of the media system could be
considered up to ChunkDemuxer(Stream). There is already some blurriness about
this separation of responsibilities, but this is how I'm thinking about it right
now. Obviously ChunkDemuxer(Stream) and SourceBufferStream have been evolving
over 2 years as the spec has changes to things aren't quite as clean as if I
were writing this from scratch today.

  Ditto on yesterday's conversation around
> re-using |track_buffer_|.
So when I was talking about track_buffer_ I was more concerned about your
immutability concerns. I wasn't talking about putting the fade-in buffers in
there like you were doing in this patch. I was talking about moving the whole
"buffer+fade-in buffers" in there like the existing code does. Sorry if I
miscommunicated that. The track_buffer_ provides a way to continue decoding some
old SourceBuffer content that for one reason or another, we need to commit to
decoding all the way through before resuming decoding of the current data "in
the SourceBuffer". This is usually to handle the end of GOPs that get
overwritten, but in your case I was suggesting that you use it as a way to keep
around a splice that you were in the middle of rendering. This is sort of like a
GOP because there are dependencies within that group of buffers that you want to
preserve. Moving it into the track_buffer_ and potentially inserting a new
replacement buffer in the SourceBufferRange  with different fade-in buffers
could provide a way to keep the buffers immutable while still being able to
handle the "overlap on top of a splice" case you were talking about.

[DaleCurtis, 2014-01-08 02:31:14]

Thanks for the elaboration.  I think it's much cleaner in ChunkDemuxerStream. 
WDYT?

[acolwell GONE FROM CHROMIUM, 2014-01-08 18:13:25]

Yes. This is the general approach I was envisioning.

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

https://codereview.chromium.org/125543002/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1043: *out_buffer =
I think you need to be careful here because I believe it is possible for buffers
in GetFadeOutPreroll() to have different config_ids so you'll need to properly
detect & signal config changes while you are iterating over these buffers. This
may require this logic to be pushed back into SourceBufferStream because that is
where the id -> config mappings are. I didn't realize this until I saw this
code. Sorry. :/

Also I believe this code will emit an extra config change if the splice point
also happens to be a config change location. SourceBufferStream would return
kConfigChange before this code even gets the buffer with a non-empty
GetFadeOutPreroll(). Since the buffers in FadeOutPreroll would likely have the
config of the previous buffer before the splice, this would trigger signalling
another kConfigChange to restore the old config and decoder state would have
been destroyed during that config toggle.

[DaleCurtis, 2014-01-08 18:45:55]

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

https://codereview.chromium.org/125543002/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1043: *out_buffer =
On 2014/01/08 18:13:26, acolwell wrote:
> I think you need to be careful here because I believe it is possible for
buffers
> in GetFadeOutPreroll() to have different config_ids so you'll need to properly
> detect & signal config changes while you are iterating over these buffers.
This
> may require this logic to be pushed back into SourceBufferStream because that
is
> where the id -> config mappings are. I didn't realize this until I saw this
> code. Sorry. :/
> 
> Also I believe this code will emit an extra config change if the splice point
> also happens to be a config change location. SourceBufferStream would return
> kConfigChange before this code even gets the buffer with a non-empty
> GetFadeOutPreroll(). Since the buffers in FadeOutPreroll would likely have the
> config of the previous buffer before the splice, this would trigger signalling
> another kConfigChange to restore the old config and decoder state would have
> been destroyed during that config toggle.

Ah you're right on both counts. Crap; this will probably need to be in
SourceBufferStream... I'll play around with it some more and see if I can come
up with something clean.

[DaleCurtis, 2014-01-08 22:23:51]

Just comments.

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

https://codereview.chromium.org/125543002/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1043: *out_buffer =
On 2014/01/08 18:45:55, DaleCurtis wrote:
> On 2014/01/08 18:13:26, acolwell wrote:
> > I think you need to be careful here because I believe it is possible for
> buffers
> > in GetFadeOutPreroll() to have different config_ids so you'll need to
properly
> > detect & signal config changes while you are iterating over these buffers.
> This
> > may require this logic to be pushed back into SourceBufferStream because
that
> is
> > where the id -> config mappings are. I didn't realize this until I saw this
> > code. Sorry. :/
> > 
> > Also I believe this code will emit an extra config change if the splice
point
> > also happens to be a config change location. SourceBufferStream would return
> > kConfigChange before this code even gets the buffer with a non-empty
> > GetFadeOutPreroll(). Since the buffers in FadeOutPreroll would likely have
the
> > config of the previous buffer before the splice, this would trigger
signalling
> > another kConfigChange to restore the old config and decoder state would have
> > been destroyed during that config toggle.
> 
> Ah you're right on both counts. Crap; this will probably need to be in
> SourceBufferStream... I'll play around with it some more and see if I can come
> up with something clean.

Actually, we could keep the code here if we extend the config id checks in
SourceBufferStream to account for fade-out buffers. Specifically this means
extending SourceBufferRange::GetNextConfigId() and the track_buffer_ config id
checks in SourceBufferStream::GetNextBuffer().

With those changes, this code just needs an added check for config id changes
within a buffer, which is doable here since we'd know the current_config_id ==
fade_out_preroll_buffer[0].configId.

It's a little messy to have the logic split across classes, but rolling this all
into SourceBufferStream::GetNextBuffer() was starting to look hideous. WDYT?

[DaleCurtis, 2014-01-08 23:00:38]

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

https://codereview.chromium.org/125543002/diff/100001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1043: *out_buffer =
On 2014/01/08 22:23:52, DaleCurtis wrote:
> On 2014/01/08 18:45:55, DaleCurtis wrote:
> > On 2014/01/08 18:13:26, acolwell wrote:
> > > I think you need to be careful here because I believe it is possible for
> > buffers
> > > in GetFadeOutPreroll() to have different config_ids so you'll need to
> properly
> > > detect & signal config changes while you are iterating over these buffers.
> > This
> > > may require this logic to be pushed back into SourceBufferStream because
> that
> > is
> > > where the id -> config mappings are. I didn't realize this until I saw
this
> > > code. Sorry. :/
> > > 
> > > Also I believe this code will emit an extra config change if the splice
> point
> > > also happens to be a config change location. SourceBufferStream would
return
> > > kConfigChange before this code even gets the buffer with a non-empty
> > > GetFadeOutPreroll(). Since the buffers in FadeOutPreroll would likely have
> the
> > > config of the previous buffer before the splice, this would trigger
> signalling
> > > another kConfigChange to restore the old config and decoder state would
have
> > > been destroyed during that config toggle.
> > 
> > Ah you're right on both counts. Crap; this will probably need to be in
> > SourceBufferStream... I'll play around with it some more and see if I can
come
> > up with something clean.
> 
> Actually, we could keep the code here if we extend the config id checks in
> SourceBufferStream to account for fade-out buffers. Specifically this means
> extending SourceBufferRange::GetNextConfigId() and the track_buffer_ config id
> checks in SourceBufferStream::GetNextBuffer().
> 
> With those changes, this code just needs an added check for config id changes
> within a buffer, which is doable here since we'd know the current_config_id ==
> fade_out_preroll_buffer[0].configId.
> 
> It's a little messy to have the logic split across classes, but rolling this
all
> into SourceBufferStream::GetNextBuffer() was starting to look hideous. WDYT?

Ahgh, an issue with this idea is when the next buffer after a splice frame is
processed... |current_config_index_| will become stale if config changes occur
within the fade-out or between the fade-out and the overlapping buffer.  As such
we'd get an extra config change after each splice frame :(

I'll keep trying to find a clean SourceBufferStream approach.

[DaleCurtis, 2014-01-08 23:54:08]

New upload merges the code back into SourceBufferStream by moving the existing
GetNextBuffer() to GetNextBufferInternal(). I believe it handles all possible
config changes now. WDYT?

[acolwell GONE FROM CHROMIUM, 2014-01-10 22:53:31]

looks good. Please add tests.

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1143: // been issued, we don't need to
account for
nit: Finish the sentence. :)

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1163:
next_buffer->GetFadeOutPreroll().at(0)->GetConfigId();
nit: Any reason you can't use the [0] instead of .at(0)? Similar comment in
other at() instances.

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
media/filters/source_buffer_stream.h:296: // See GetNextBuffer().
nit: It would be good to at least outline the division of responsibility between
GetNextBuffer() and GetNextBufferInternal().

[DaleCurtis, 2014-01-10 23:03:49]

Just comments.  Since this doesn't include the code to actually generate splice
frames, it's a little hard to test.  I'd like to leave that for a second CL
since this is already a bit larger than expected.  How about adding a new
meta-buffer code point to your SourceBufferStream::AppendBuffer(<string>) to
generate fake splices?

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1143: // been issued, we don't need to
account for
On 2014/01/10 22:53:31, acolwell wrote:
> nit: Finish the sentence. :)

Haha, awesome, I accidentally a sentence.

https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1163:
next_buffer->GetFadeOutPreroll().at(0)->GetConfigId();
On 2014/01/10 22:53:31, acolwell wrote:
> nit: Any reason you can't use the [0] instead of .at(0)? Similar comment in
> other at() instances.

I switched to .at() since that seemed to be the style in SourceBufferRange.  I
see now that both styles are used.  Which would you prefer?

[acolwell GONE FROM CHROMIUM, 2014-01-10 23:21:00]

On 2014/01/10 23:03:49, DaleCurtis wrote:
> Just comments.  Since this doesn't include the code to actually generate
splice
> frames, it's a little hard to test.  I'd like to leave that for a second CL
> since this is already a bit larger than expected.  How about adding a new
> meta-buffer code point to your SourceBufferStream::AppendBuffer(<string>) to
> generate fake splices?

Eventhough the SourceBufferStream doesn't generate splices itself, I still would
like to have tests that verify the new logic in isolation. I'd suggest factoring
out the string -> BufferQueue functionality from AppendBuffer(string) and use
that to manually create the splices. It should be relatively simple to do.

> 
>
https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
> File media/filters/source_buffer_stream.cc (right):
> 
>
https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
> media/filters/source_buffer_stream.cc:1143: // been issued, we don't need to
> account for
> On 2014/01/10 22:53:31, acolwell wrote:
> > nit: Finish the sentence. :)
> 
> Haha, awesome, I accidentally a sentence.
> 
>
https://codereview.chromium.org/125543002/diff/210001/media/filters/source_bu...
> media/filters/source_buffer_stream.cc:1163:
> next_buffer->GetFadeOutPreroll().at(0)->GetConfigId();
> On 2014/01/10 22:53:31, acolwell wrote:
> > nit: Any reason you can't use the [0] instead of .at(0)? Similar comment in
> > other at() instances.
> 
> I switched to .at() since that seemed to be the style in SourceBufferRange.  I
> see now that both styles are used.  Which would you prefer?

Yeah I saw that too. I prefer [] over at() because I think it is easier to read
and... less characters. :)

[DaleCurtis, 2014-01-13 21:23:31]

Not ready for full review yet, but take a look at what I did in the unittests
and see if you like it.  I added support for a new coded buffer "S(XX YY ZZ)"
which also supports config changes within the buffer by using a coded "C".

If you like it I'll add some more tests around track buffer and seeking.

[DaleCurtis, 2014-01-13 23:34:18]

Sounds reasonable.  What about an "mse" or "formats" top-level dir?

[DaleCurtis, 2014-01-13 23:34:47]

Whoops, that comment was for the other CL...

[acolwell GONE FROM CHROMIUM, 2014-01-14 02:01:26]

looks pretty good. Just a few comments.

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1163: next_buffer->GetConfigId() :
It feels like a static helper function with a signature like

int GetConfigId(StreamParserBuffer* buffer, size_t index)

should be used for this and the similar logic elsewhere.

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:345: if (EndsWith(timestamps[i],
"K", true)) {
I think you probably need to move this down below the new splice frame code so
that "S(1 2 3)K" isn't accepted. I'd expect "S(1K 2 3)" to be the way to signal
the keyframe for the fade out buffer. Either that or move to something like "S(2
3)1K" or "S(2 3)->1K" so reading left to right reflects the order the buffers
will be output.

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:3348: NewSegmentAppend("0K S(10 3
6 9) 15 20 S(35 25 30) 40");
This is why I think changing the format would be a little nicer. I think
something like 20 S(25 30)->35 40 might be a little better because the numbers
are always increasing from left to right.

This also reminds me that we should have logic that only allows fade-out buffers
to be added to buffers that are keyframes. Otherwise we'll cause problems
downstream.

[DaleCurtis, 2014-01-14 22:44:34]

Just comments.

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:345: if (EndsWith(timestamps[i],
"K", true)) {
On 2014/01/14 02:01:26, acolwell wrote:
> I think you probably need to move this down below the new splice frame code so
> that "S(1 2 3)K" isn't accepted. I'd expect "S(1K 2 3)" to be the way to
signal
> the keyframe for the fade out buffer. Either that or move to something like
"S(2
> 3)1K" or "S(2 3)->1K" so reading left to right reflects the order the buffers
> will be output.

I'll switch it to S(1K 2 3) since that seems most legible to me.

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:3348: NewSegmentAppend("0K S(10 3
6 9) 15 20 S(35 25 30) 40");
On 2014/01/14 02:01:26, acolwell wrote:
> This is why I think changing the format would be a little nicer. I think
> something like 20 S(25 30)->35 40 might be a little better because the numbers
> are always increasing from left to right.
> 
> This also reminds me that we should have logic that only allows fade-out
buffers
> to be added to buffers that are keyframes. Otherwise we'll cause problems
> downstream.

Are all audio frames keyframes?  The mp3 parser tags all frames as key frames. 
It's harder to tell with the aac parser.  Should splice frames be created even
in the video case?

[acolwell GONE FROM CHROMIUM, 2014-01-14 22:56:43]

On 2014/01/14 22:44:34, DaleCurtis wrote:
> Just comments.
> 
>
https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
> File media/filters/source_buffer_stream_unittest.cc (right):
> 
>
https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
> media/filters/source_buffer_stream_unittest.cc:345: if
(EndsWith(timestamps[i],
> "K", true)) {
> On 2014/01/14 02:01:26, acolwell wrote:
> > I think you probably need to move this down below the new splice frame code
so
> > that "S(1 2 3)K" isn't accepted. I'd expect "S(1K 2 3)" to be the way to
> signal
> > the keyframe for the fade out buffer. Either that or move to something like
> "S(2
> > 3)1K" or "S(2 3)->1K" so reading left to right reflects the order the
buffers
> > will be output.
> 
> I'll switch it to S(1K 2 3) since that seems most legible to me.
Ok. I'm assuming that 3 is the timestamp on the fade-in buffer and 1 & 2 are the
timestamps on the fade-out buffers. The only reason I suggested the "->" was to
provide a visual hint that there was a difference between these buffers.

> 
>
https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
> media/filters/source_buffer_stream_unittest.cc:3348: NewSegmentAppend("0K S(10
3
> 6 9) 15 20 S(35 25 30) 40");
> On 2014/01/14 02:01:26, acolwell wrote:
> > This is why I think changing the format would be a little nicer. I think
> > something like 20 S(25 30)->35 40 might be a little better because the
numbers
> > are always increasing from left to right.
> > 
> > This also reminds me that we should have logic that only allows fade-out
> buffers
> > to be added to buffers that are keyframes. Otherwise we'll cause problems
> > downstream.
> 
> Are all audio frames keyframes?  
For all the codecs we deal with right now yes. Technically the keyframe flag in
the MSE code actually means "random access point" but I used keyframe because
it's a more familiar concept to people.

> The mp3 parser tags all frames as key frames. 
> It's harder to tell with the aac parser.  Should splice frames be created even
> in the video case?
Splice frames could eventually be created for video to allow frame accurate
edits. Basically the "fade out buffers" would contain all the frames in the GOP
that the "fade out buffer" depends on to decode properly. That would allow you
to splice on P-frames instead of only allowing splices on I-frames like we do
right now.

[DaleCurtis, 2014-01-15 01:42:05]

Okay, this should be ready for review now.  Tests added for all the cases I can
think of.

[DaleCurtis, 2014-01-15 02:06:08]

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/125543002/diff/300001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1163: next_buffer->GetConfigId() :
On 2014/01/14 02:01:26, acolwell wrote:
> It feels like a static helper function with a signature like
> 
> int GetConfigId(StreamParserBuffer* buffer, size_t index)
> 
> should be used for this and the similar logic elsewhere.
> 

Done.

[acolwell GONE FROM CHROMIUM, 2014-01-15 19:28:49]

looks pretty good. Just a few minor comments.

https://codereview.chromium.org/125543002/diff/450001/media/base/stream_parse...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/125543002/diff/450001/media/base/stream_parse...
media/base/stream_parser_buffer.cc:73: fade_out_preroll_ = fade_out_preroll;
nit: It might be worth adding a DCHECK(HasNoFadeOutPreroll(fade_out_preroll));
to make sure that  none of the preroll buffers have preroll buffers since we
have no logic to handle recursive nesting of these guys.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1122: }
nit: I think you should set *out_buffer = NULL here so that you can't
accidentally hand out the wrong pointer and so that the value is NULL when
kConfigChange is returned.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1420: current_config_index_ =
track_buffer_.front()->GetConfigId();
I think you need to use the helper function here too just in case a splice frame
makes its way into track_buffer_.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1943: return
next_buffer->GetFadeOutPreroll().empty() ?
nit: Use the helper function here too.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:346: splice_frame = true;
nit: Add DCHECK(!splice_frame) to avoid allowing S(S(1 2 3) 5 6)?

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:1419:
SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
I'm assuming this is just to make the comment and code match. No behavior change
occurred right?

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:1577:
SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
ditto?

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:3350: CheckExpectedBuffers("0K 3K
6 9");
Consider adding support for "C" to CheckExpectedBuffers() so that this test
could become something like.

NewSegmentAppend("0K S(3K 6 9 10) 15 20 S(25K 30 35) 40");
CheckExpectedBuffers("0K 3K 6 9 C 10 15 20 25K 30 C 35 40");
CheckNoNextBuffer();

I think it should be pretty straight forward to do so.

[DaleCurtis, 2014-01-15 22:40:55]

https://codereview.chromium.org/125543002/diff/450001/media/base/stream_parse...
File media/base/stream_parser_buffer.cc (right):

https://codereview.chromium.org/125543002/diff/450001/media/base/stream_parse...
media/base/stream_parser_buffer.cc:73: fade_out_preroll_ = fade_out_preroll;
On 2014/01/15 19:28:50, acolwell wrote:
> nit: It might be worth adding a DCHECK(HasNoFadeOutPreroll(fade_out_preroll));
> to make sure that  none of the preroll buffers have preroll buffers since we
> have no logic to handle recursive nesting of these guys.

Done.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1122: }
On 2014/01/15 19:28:50, acolwell wrote:
> nit: I think you should set *out_buffer = NULL here so that you can't
> accidentally hand out the wrong pointer and so that the value is NULL when
> kConfigChange is returned.

Done.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1420: current_config_index_ =
track_buffer_.front()->GetConfigId();
On 2014/01/15 19:28:50, acolwell wrote:
> I think you need to use the helper function here too just in case a splice
frame
> makes its way into track_buffer_.

Done.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:1943: return
next_buffer->GetFadeOutPreroll().empty() ?
On 2014/01/15 19:28:50, acolwell wrote:
> nit: Use the helper function here too.

Done.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:346: splice_frame = true;
On 2014/01/15 19:28:50, acolwell wrote:
> nit: Add DCHECK(!splice_frame) to avoid allowing S(S(1 2 3) 5 6)?

Done.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:1419:
SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
On 2014/01/15 19:28:50, acolwell wrote:
> I'm assuming this is just to make the comment and code match. No behavior
change
> occurred right?

Correct, test passes as before.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:1577:
SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
On 2014/01/15 19:28:50, acolwell wrote:
> ditto?

Ditto.

https://codereview.chromium.org/125543002/diff/450001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:3350: CheckExpectedBuffers("0K 3K
6 9");
On 2014/01/15 19:28:50, acolwell wrote:
> Consider adding support for "C" to CheckExpectedBuffers() so that this test
> could become something like.
> 
> NewSegmentAppend("0K S(3K 6 9 10) 15 20 S(25K 30 35) 40");
> CheckExpectedBuffers("0K 3K 6 9 C 10 15 20 25K 30 C 35 40");
> CheckNoNextBuffer();
> 
> I think it should be pretty straight forward to do so.

Great idea. It loses the ability to verify the config, but there are other tests
for that. Done.

[acolwell GONE FROM CHROMIUM, 2014-01-15 23:30:58]

lgtm

https://codereview.chromium.org/125543002/diff/510001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/125543002/diff/510001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:3471: CheckExpectedBuffers("0K 3K
C 6 9 C 10 15");
Oh. I actually liked the config change verification you had. I figured you would
have kept it by doing the following.

CheckExpectedBuffers("0K 3K C");
CheckConfig(new_config);

CheckExpectedBuffers("6 9 C");
CheckConfig(video_config_);

CheckExpectedBuffers("10 15");

[DaleCurtis, 2014-01-16 01:16:11]

Thanks for review!

https://codereview.chromium.org/125543002/diff/510001/media/filters/source_bu...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/125543002/diff/510001/media/filters/source_bu...
media/filters/source_buffer_stream_unittest.cc:3471: CheckExpectedBuffers("0K 3K
C 6 9 C 10 15");
On 2014/01/15 23:30:59, acolwell wrote:
> Oh. I actually liked the config change verification you had. I figured you
would
> have kept it by doing the following.
> 
> CheckExpectedBuffers("0K 3K C");
> CheckConfig(new_config);
> 
> CheckExpectedBuffers("6 9 C");
> CheckConfig(video_config_);
> 
> CheckExpectedBuffers("10 15");

Seems reasonable, I've kept the config checks on the tests which actually change
configs.

[commit-bot: I haz the power, 2014-01-16 01:26:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/125543002/630001

[commit-bot: I haz the power, 2014-01-16 01:26:32]

Failed to apply patch for media/filters/source_buffer_stream_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/filters/source_buffer_stream_unittest.cc
  Hunk #3 succeeded at 241 (offset 5 lines).
  Hunk #4 succeeded at 271 (offset 5 lines).
  Hunk #5 succeeded at 363 with fuzz 2 (offset 36 lines).
  Hunk #6 FAILED at 350.
  Hunk #7 FAILED at 368.
  Hunk #8 succeeded at 409 (offset 22 lines).
  Hunk #9 succeeded at 1431 (offset 43 lines).
  Hunk #10 succeeded at 1589 (offset 43 lines).
  Hunk #11 succeeded at 3106 (offset 64 lines).
  Hunk #12 succeeded at 3115 (offset 64 lines).
  Hunk #13 succeeded at 3356 (offset 64 lines).
  Hunk #14 succeeded at 3446 (offset 64 lines).
  2 out of 14 hunks FAILED -- saving rejects to file
media/filters/source_buffer_stream_unittest.cc.rej

Patch:       media/filters/source_buffer_stream_unittest.cc
Index: media/filters/source_buffer_stream_unittest.cc
diff --git a/media/filters/source_buffer_stream_unittest.cc
b/media/filters/source_buffer_stream_unittest.cc
index
c9a193ad232de69881d587315a53413fadf9f828..90c8d33fa23cd0b582ba01f60f02e2cf18e3511e
100644
--- a/media/filters/source_buffer_stream_unittest.cc
+++ b/media/filters/source_buffer_stream_unittest.cc
@@ -6,6 +6,8 @@
 
 #include <string>
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
@@ -29,8 +31,8 @@ class SourceBufferStreamTest : public testing::Test {
  protected:
   SourceBufferStreamTest() {
     video_config_ = TestVideoConfig::Normal();
-    stream_.reset(new SourceBufferStream(video_config_, LogCB()));
     SetStreamInfo(kDefaultFramesPerSecond, kDefaultKeyframesPerSecond);
+    stream_.reset(new SourceBufferStream(video_config_, log_cb()));
   }
 
   void SetMemoryLimit(int buffers_of_data) {
@@ -234,6 +236,13 @@ class SourceBufferStreamTest : public testing::Test {
       if (i > 0)
         ss << " ";
 
+      if (timestamps[i] == "C") {
+        EXPECT_EQ(SourceBufferStream::kConfigChange, status);
+        stream_->GetCurrentVideoDecoderConfig();
+        ss << timestamps[i];
+        continue;
+      }
+
       EXPECT_EQ(SourceBufferStream::kSuccess, status);
       if (status != SourceBufferStream::kSuccess)
         break;
@@ -257,6 +266,11 @@ class SourceBufferStreamTest : public testing::Test {
         << "\nActual: " << actual.AsHumanReadableString();
   }
 
+  const LogCB log_cb() {
+    return base::Bind(&SourceBufferStreamTest::DebugMediaLog,
+                      base::Unretained(this));
+  }
+
   base::TimeDelta frame_duration() const { return frame_duration_; }
 
   scoped_ptr<SourceBufferStream> stream_;
@@ -313,6 +327,21 @@ class SourceBufferStreamTest : public testing::Test {
       EXPECT_EQ(expect_success, stream_->Append(queue));
   }
 
+  // AppendBuffers() allows for the generation of StreamParserBuffers from
coded
+  // strings of timestamps separated by spaces.  Supported syntax:
+  //
+  // ##:
+  // Generates a StreamParserBuffer with decode timestamp ##.  E.g., "0 1 2 3".
+  //
+  // ##K:
+  // Indicates the buffer with timestamp ## reflects a keyframe.  E.g., "0K 1".
+  //
+  // S(a# ... y# z#)
+  // Indicates a splice frame buffer should be created with timestamp z#.  The
+  // preceding timestamps a# ... y# will be treated as the fade out preroll for
+  // the splice frame.  If a timestamp within the preroll ends with C the
config
+  // id to use for that and subsequent preroll appends is incremented by one.
+  // The config id for non-splice frame appends will not be affected.
   void AppendBuffers(const std::string& buffers_to_append,
                      bool start_new_segment, bool one_by_one,
                      bool expect_success) {
@@ -321,14 +350,40 @@ class SourceBufferStreamTest : public testing::Test {
 
     CHECK_GT(timestamps.size(), 0u);
 
+    bool splice_frame = false;
+    size_t splice_config_id = stream_->append_config_index_;
+    std::vector<scoped_refptr<StreamParserBuffer> > fade_out_preroll;
     SourceBufferStream::BufferQueue buffers;
     for (size_t i = 0; i < timestamps.size(); i++) {
       bool is_keyframe = false;
+      bool last_splice_frame = false;
+      // Handle splice frame starts.
+      if (StartsWithASCII(timestamps[i], "S(", true)) {
+        CHECK(!splice_frame);
+        splice_frame = true;
+        // Remove the "S(" off of the token.
+        timestamps[i] = timestamps[i].substr(2, timestamps[i].length());
+      }
+      if (splice_frame && EndsWith(timestamps[i], ")", true)) {
+        splice_frame = false;
+        last_splice_frame = true;
+        // Remove the ")" off of the token.
+        timestamps[i] = timestamps[i].substr(0, timestamps[i].length() - 1);
+      }
+      // Handle config changes within the splice frame.
+      if (splice_frame && EndsWith(timestamps[i], "C", true)) {
+        splice_config_id++;
+        CHECK(splice_config_id < stream_->audio_configs_.size() ||
+              splice_config_id < stream_->video_configs_.size());
+        // Remove the "C" off of the token.
+        timestamps[i] = timestamps[i].substr(0, timestamps[i].length() - 1);
+      }
       if (EndsWith(timestamps[i], "K", true)) {
         is_keyframe = true;
         // Remove the "K" off of the token.
         timestamps[i] = timestamps[i].substr(0, timestamps[i].length() - 1);
       }
+
       int time_in_ms;
       CHECK(base::StringToInt(timestamps[i], &time_in_ms));
 
@@ -339,6 +394,25 @@ class SourceBufferStreamTest : public testing::Test {
           base::TimeDelta::FromMilliseconds(time_in_ms);
       buffer->SetDecodeTimestamp(timestamp);
 
+      if (splice_frame) {
+        if (!fade_out_preroll.empty()) {
+          // Enforce strictly monotonically increasing timestamps.
+          CHECK_GT(
+              timestamp.InMicroseconds(),
+              fade_out_preroll.back()->GetDecodeTimestamp().InMicroseconds());
+        }
+        buffer->SetConfigId(splice_config_id);
+        fade_out_preroll.push_back(buffer);
+        continue;
+      }
+
+      if (last_splice_frame) {
+        // Forbid splice frames without preroll.
+        CHECK(!fade_out_preroll.empty());
+        buffer->SetFadeOutPreroll(fade_out_preroll);
+        fade_out_preroll.clear();
+      }
+
       if (i == 0u && start_new_segment)
         stream_->OnNewMediaSegment(timestamp);
 
@@ -358,6 +432,10 @@ class SourceBufferStreamTest : public testing::Test {
     }
   }
 
+  void DebugMediaLog(const std::string& log) {
+    DVLOG(1) << log;
+  }
+
   int frames_per_second_;
   int keyframes_per_second_;
   base::TimeDelta frame_duration_;
@@ -1355,7 +1433,7 @@ TEST_F(SourceBufferStreamTest,
Overlap_OneByOne_TrackBuffer) {
   CheckExpectedRangesByTimestamp("{ [10,160) }");
 
   // Seek to 70ms.
-  SeekToTimestamp(base::TimeDelta::FromMilliseconds(10));
+  SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms.
@@ -1513,7 +1591,7 @@ TEST_F(SourceBufferStreamTest,
Overlap_OneByOne_TrackBuffer6) {
   CheckExpectedRangesByTimestamp("{ [10,160) [200,260) }");
 
   // Seek to 70ms.
-  SeekToTimestamp(base::TimeDelta::FromMilliseconds(10));
+  SeekToTimestamp(base::TimeDelta::FromMilliseconds(70));
   CheckExpectedBuffers("10K 40");
 
   // Overlap with a new segment from 0 to 120ms.
@@ -3009,7 +3087,7 @@ TEST_F(SourceBufferStreamTest,
SameTimestamp_Video_Overlap_3) {
 TEST_F(SourceBufferStreamTest, SameTimestamp_Audio) {
   AudioDecoderConfig config(kCodecMP3, kSampleFormatF32, CHANNEL_LAYOUT_STEREO,
                             44100, NULL, 0, false);
-  stream_.reset(new SourceBufferStream(config, LogCB()));
+  stream_.reset(new SourceBufferStream(config, log_cb()));
   Seek(0);
   NewSegmentAppend("0K 0K 30K 30 60 60");
   CheckExpectedBuffers("0K 0K 30K 30 60 60");
@@ -3018,7 +3096,7 @@ TEST_F(SourceBufferStreamTest, SameTimestamp_Audio) {
 TEST_F(SourceBufferStreamTest, SameTimestamp_Audio_Invalid_1) {
   AudioDecoderConfig config(kCodecMP3, kSampleFormatF32, CHANNEL_LAYOUT_STEREO,
                             44100, NULL, 0, false);
-  stream_.reset(new SourceBufferStream(config, LogCB()));
+  stream_.reset(new SourceBufferStream(config, log_cb()));
   Seek(0);
   NewSegmentAppend_ExpectFailure("0K 30 30K 60");
 }
@@ -3259,7 +3337,6 @@ TEST_F(SourceBufferStreamTest, Remove_GOPBeingAppended) {
   CheckExpectedBuffers("240K 270 300");
 }
 
-
 TEST_F(SourceBufferStreamTest,
        Remove_PreviousAppendDestroyedAndOverwriteExistingRange) {
   SeekToTimestamp(base::TimeDelta::FromMilliseconds(90));
@@ -3350,6 +3427,92 @@ TEST_F(SourceBufferStreamTest, Text_OverlapBefore) {
   CheckExpectedBuffers("0K 501K 1001K 1501K 2001K 2500K 3000K 3500K");
 }
 
+TEST_F(SourceBufferStreamTest, SpliceFrame_Basic) {
+  Seek(0);
+  NewSegmentAppend("0K S(3K 6 9 10) 15 20 S(25K 30 35) 40");
+  CheckExpectedBuffers("0K 3K 6 9 C 10 15 20 25K 30 C 35 40");
+  CheckNoNextBuffer();
+}
+
+TEST_F(SourceBufferStreamTest, SpliceFrame_SeekClearsSplice) {
+  Seek(0);
+  NewSegmentAppend("0K S(3K 6 9 10) 15K 20");
+  CheckExpectedBuffers("0K 3K 6");
+
+  SeekToTimestamp(base::TimeDelta::FromMilliseconds(15));
+  CheckExpectedBuffers("15K 20");
+  CheckNoNextBuffer();
+}
+
+TEST_F(SourceBufferStreamTest, SpliceFrame_SeekClearsSpliceFromTrackBuffer) {
+  Seek(0);
+  NewSegmentAppend("0K 2K S(3K 6 9 10) 15K 20");
+  CheckExpectedBuffers("0K 2K");
+
+  // Overlap the existing segment.
+  NewSegmentAppend("5K 15K 20");
+  CheckExpectedBuffers("3K 6");
+
+  SeekToTimestamp(base::TimeDelta::FromMilliseconds(15));
+  CheckExpectedBuffers("15K 20");
+  CheckNoNextBuffer();
+}
+
+TEST_F(SourceBufferStreamTest, SpliceFrame_ConfigChangeWithinSplice) {
+  VideoDecoderConfig new_config = TestVideoConfig::Large();
+  ASSERT_FALSE(new_config.Matches(video_config_));
+
+  // Add a new video config, th…
(message too large)

[commit-bot: I haz the power, 2014-01-16 02:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/125543002/690001

[commit-bot: I haz the power, 2014-01-16 03:11:32]

Retried try job too often on win_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-16 18:38:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/125543002/690001

[commit-bot: I haz the power, 2014-01-17 01:47:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/125543002/690001

[commit-bot: I haz the power, 2014-01-17 01:47:31]

Change committed as 245413