Implement core of compliant MediaSource coded frame processing

Implement core of compliant MediaSource coded frame processing

Introduces FrameProcessor and associated unit tests that implement the
core of the Coded Frame Processing algorithm in the current Media Source
Extension spec (April 1, 2014 MSE editor's draft).

R=acolwell@chromium.org
BUG=249422
TEST=All media_unittests pass and no http/tests/media/media-source layout tests regress locally on Linux with proprietary codecs enabled

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

[wolenetz, 2014-02-27 22:19:38]

acolwell: patch set 1 is current WIP. It doesn't build yet, tests are outdated
and lots are missing, but the bulk of the core algorithm is in place.

[wolenetz, 2014-02-27 22:20:18]

On 2014/02/27 22:19:38, wolenetz wrote:
> acolwell: patch set 1 is current WIP. It doesn't build yet, tests are outdated
> and lots are missing, but the bulk of the core algorithm is in place.

oh, and NOT LGTM :)

[wolenetz, 2014-03-11 02:59:05]

Patch set 3 is not ready for review yet. It's just an update with some of the
refactoring in place, now depending on landing the LegacyFrameProcessor
refactoring first (https://codereview.chromium.org/191513002/).

[wolenetz, 2014-03-11 02:59:31]

On 2014/03/11 02:59:05, wolenetz wrote:
> Patch set 3 is not ready for review yet. It's just an update with some of the
> refactoring in place, now depending on landing the LegacyFrameProcessor
> refactoring first (https://codereview.chromium.org/191513002/).

s/Patch set 3/Patch set 2/...

[wolenetz, 2014-03-12 02:47:50]

PS3 is *not ready for review yet*. It's just an updated work-in-progress that's
been rebased on top of the updated prerequisite CLs, and fleshed out a bit more.
Several "BIG TODO" remain for me to finish up on my return from OOO, before it
will be ready for review.

[wolenetz, 2014-03-19 02:28:48]

PS4 is not yet ready for review. In particular, endOfStream/duration change
behavior is different with new FrameProcessor, and likely
ChunkDemuxer::MarkEndOfStream() needs to allow duration increase, not just
decrease.

chunk_demuxer_unittests.cc is updated to generate block groups (with block
durations) to work around http://crbug.com/351166 not yet being fixed, so I can
test the new frame processor locally.

chunk_demuxer_unittests.cc is also (partially) updated to contain adjusted test
steps and expectations acccording to the new behavior of the coded frame
processor.

Before being review-ready, there's more work to be done still on:
ChunkDemuxerTest
PipelineIntegrationTest
Specific FrameProcessor test coverage

[wolenetz, 2014-03-19 17:40:23]

On 2014/03/19 02:28:48, wolenetz wrote:
> PS4 is not yet ready for review. In particular, endOfStream/duration change
> behavior is different with new FrameProcessor, and likely
> ChunkDemuxer::MarkEndOfStream() needs to allow duration increase, not just
> decrease.
> 
> chunk_demuxer_unittests.cc is updated to generate block groups (with block
> durations) to work around http://crbug.com/351166 not yet being fixed, so I
can
> test the new frame processor locally.
> 
> chunk_demuxer_unittests.cc is also (partially) updated to contain adjusted
test
> steps and expectations acccording to the new behavior of the coded frame
> processor.
> 
> Before being review-ready, there's more work to be done still on:
> ChunkDemuxerTest
> PipelineIntegrationTest
> Specific FrameProcessor test coverage

Per thread with acolwell@, spec likely has bug in that the (step 5) duration
update at end of coded frame processing should be to max(current duration,
highest presentation end timestamp), and NOT max(current duration, highest
HTMLMediaElement.buffered range end timestamp). Thus, the legacy processor's
"IncreaseDurationIfNecessary(..)" usage is nearer correct than the new frame
processor code in PS4 here.

My expectation is acolwell@ will get the spec fixed and I'll continue work
assuming max(current duration, highest presentation end timestamp) is correct in
new frame processor duration updating.

[wolenetz, 2014-03-20 17:03:59]

PS5 is not ready for review -- it's just an update of the current new frame
processor work-in-progress CL with new understanding of spec's step 5 of CFP.

It's also a rebase onto https://codereview.chromium.org/205703003/ PS1.

Also, to get ChunkDemuxerTests and PipelineIntegrationTests to pass, many
mediasource webm file-based tests are disabled until http://crbug.com/351166 and
http://crbug.com/354284 are fixed.

Multiple "BIG TODO" remain; next steps are:
1. Land the prereq CL https://codereview.chromium.org/205703003/ (which has
significant risk of delay in landing due to underlying
AppendData->UpdateDuration behavior change to LegacyFrameProcessor)
2. Fill in and get some basic new FrameProcessor unit tests running

Also note that multiple Blink mediasource layout tests currently fail on PS1 of
https://codereview.chromium.org/205703003/ due to change in duration behavior:
it's very likely that layout test html + test media files' init segment changes
will be required, if the rest of that PS1 lands as-is.

[wolenetz, 2014-03-20 17:33:25]

On 2014/03/20 17:03:59, wolenetz wrote:
> PS5 is not ready for review -- it's just an update of the current new frame
> processor work-in-progress CL with new understanding of spec's step 5 of CFP.
> 
> It's also a rebase onto https://codereview.chromium.org/205703003/ PS1.
> 
> Also, to get ChunkDemuxerTests and PipelineIntegrationTests to pass, many
> mediasource webm file-based tests are disabled until http://crbug.com/351166
and
> http://crbug.com/354284 are fixed.
> 
> Multiple "BIG TODO" remain; next steps are:
> 1. Land the prereq CL https://codereview.chromium.org/205703003/ (which has
> significant risk of delay in landing due to underlying
> AppendData->UpdateDuration behavior change to LegacyFrameProcessor)
> 2. Fill in and get some basic new FrameProcessor unit tests running
> 
> Also note that multiple Blink mediasource layout tests currently fail on PS1
of
> https://codereview.chromium.org/205703003/ due to change in duration behavior:
> it's very likely that layout test html + test media files' init segment
changes
> will be required, if the rest of that PS1 lands as-is.

With privately changed PS5 to disable webm and properly reference
IncreaseDurationIfNecessary (not UpdateDurationDuringAppend) in CD creation of
FrameProcessor instance, multiple MP4-based mediasource layout tests fail for
reasons other than prefixed tests expecting webm support. These look similar to
the webm failures where the expected duration is slightly less than the actual
duration calculated by "highest presentation end timestamp":

mediasource-duration:
 FAIL Test seek starts on duration truncation below currentTime assert_equals:
mediaElement fullDuration expected 6.0368 but got 6.0756
 FAIL Test appendBuffer completes previous seek to truncated duration
assert_equals: mediaElement fullDuration expected 6.0368 but got 6.0756
 FAIL Test endOfStream completes previous seek to truncated duration
assert_equals: mediaElement fullDuration expected 6.0368 but got 6.0756
 FAIL Test setting same duration multiple times does not fire duplicate
durationchange assert_equals: mediaElement fullDuration expected 6.0368 but got
6.0756

mediasource-gc-after-decode-error-crash:
 Timeout with no result: this test requires webm support

[wolenetz, 2014-03-20 19:16:35]

PS6 is not ready for review. It's just a minor update to stabilize PS5 before I
switch focus to working on fixing the biggest blocker (http://crbug.com/351166)
of eventually enabling the code in this CL.

[wolenetz, 2014-03-20 19:36:39]

(Not ready for review, just a comment:)

https://codereview.chromium.org/180153003/diff/90001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/90001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3187: // Note that audio frame duration
is 23ms, so post-seek, the first audio
acolwell@: Note, this is the interesting discrepancy I mentioned offline to you
today.

[wolenetz, 2014-03-28 19:36:37]

PS7 is not ready for review, just a rebase update now that some further prereqs
(estimating missing webm frame durations / using webm TrackEntry
DefaultDuration) are landed/close to landing.

Known items remaining before ready for review:
1) Need basic new FrameProcessor unit tests
2) Need to disable illegalization of frame with DTS > PTS (and reference bug
354518 -- see the PipelineIntegrationTest that I've (maybe just temporarily)
disabled in PS7.)
3) Consider setting all PipelineIntegrationTests and ChunkDemuxerTests to run
with Legacy, then separately with new Frame Processor.
4) Determine the scope of any further binary file or layout test html or
expectation changes and put together a Blink CL that at least contains any new
failing expectations in advance of landing this (if this causes any Blink
failure).

https://codereview.chromium.org/180153003/diff/90001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/90001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:3187: // Note that audio frame duration
is 23ms, so post-seek, the first audio
On 2014/03/20 19:36:40, wolenetz wrote:
> acolwell@: Note, this is the interesting discrepancy I mentioned offline to
you
> today.

Once rebased onto my webm frame duration deriver/estimator/defaulter, this
portion of the test change was no longer necessary.

[wolenetz, 2014-05-06 21:42:15]

PS8 is getting closer. It's not ready for review yet.
Items remaining:

    TODO: frame processor unit tests
    (segments & sequence mode)
    A: P(A) -> (a)
    V: P(V) -> (v)
    A: P(A0, A1) -> (a0,a1)
    A: STO(10)+P(A0) -> TSO==10(a0@10)
    A: P(A0, A1) + P(A2, A3) -> (a0,a1,a2,a3)
    
    (segments mode)
    A: P(A2, A3) + P(A0, A1) -> TSO==0,(a0,a1,a2,a3)
    AV: P(A0, A1, A3, A4, A5;V0key, V1notkey, V4notkey, V5) ->
    (a0,a1,a3,a4,a5),(v0,v1,v5)
    
    (sequence mode)
    A: P(A2, A3) -> TSO==2 ; P(A0, A1) -> TSO==4,(a2,a3,a0@4,a1@5)
    A: STO(10)+P(A5) -> TSO==5,(a5@10)
    AV: P(A0, A1, A3, A4, A5;V0key, V1notkey, V4notkey, V5) ->
    (a0,a1,a3,a4,a5@6),(v0,v1,v5@6)
    --> Assumes a4 is processed before v4notkey
    
    (negative)
    A: P(V) -> fail
    A: P(A1,A0) -> fail
    
    TODO: rerun pipeline and chunkdemuxer tests with both old and new frame
processor
    TODO: determine the scope of any binary file or layout test html or
expectation changes
          AND put together a Blink CL that contains failing expectations in
          advance of landing anything that could cause failure without Blink
          adjustment

[wolenetz, 2014-05-07 05:27:41]

PS9 is another checkpoint; not ready for review yet.

[wolenetz, 2014-05-07 20:56:46]

PS10 is ready for review. \o/ !!
acolwell: please review everything
dalecurtis: fyi: I've included your partial start discard on append_window_start
overlap into this new FrameProcessor, too (and the related ChunkDemuxerTest
passes for both frame processors).

I ran the http/test/media/media-source layout tests locally; none failed other
than the (well known) expectation mismatches (FAIL->PASS) for mp4
isTypeSupported-related tests since I've enabled proprietary codecs in my
environment.

LegacyFrameProcessor is kept as the default frame processor for now, with Blink
enabling use of the new FrameProcessor behind the
--enable-experimental-web-platform-features flag.

[acolwell GONE FROM CHROMIUM, 2014-05-08 00:06:43]

Looks pretty good. Just a few comments

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:29: //
https://dvcs.w3.org/hg/html-media/raw-file/d471a4412040/media-source/media-so...
nit: You can use
https://dvcs.w3.org/hg/html-media/raw-file/d471a4412040/media-source/media-so...
to reference the attribute directly.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:173: // Frame PTS is only updated if it
survives processing.
nit:s/Frame/Note: |frame|/  so it is clear that this isn't part of the spec
text?

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:190: track_id = kVideoTrackId;
Can't this and the audio ID be hidden in frame->track_id() or at least where
|frame| is created.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:293: presentation_timestamp =
append_window_start;  // Frame updated, below.
nit: s/Frame/|frame|/ here and below

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:295: frame_duration = frame_end_timestamp -
append_window_start;
nit: Perhaps use presentation_timestamp here since frame_end_timestamp was set
via presentation_timestamp + frame_duration above?

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:347:
track_buffer->stream()->Append(buffer_to_append);
This is OK for now, but I'm a little concerned about a perf hit from doing so
many single buffer appends. In a followup CL we may want to consider collecting
the buffers in |track_buffer| and then committing them in a single Append() on
each track_buffer before ProcessFrames() returns.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor.h (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.h:53: bool group_start_timestamp_set_;
nit: Any reason not to use kNoTimestamp() to track unset state?

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_base.h:126: virtual void
SetGroupStartTimestampIfInSequenceMode(
Does this really need to be virtual? I don't think it hurts anything to simply
have the implementation here even if the legacy impl will never call it.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
nit: s/(c) //

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:68: void AddTestTracks(const bool
has_audio, const bool has_video) {
nit: Use HAS_AUDIO & HAS_VIDEO flags like ChunkDemuxer unit test so the call
sites are a little clearer & self documenting.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:154: } while (++loop_count < 10 &&
read_callback_called_ &&
Do you really need 10 here? Why would you expect more than 1 abort?

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:160: EXPECT_EQ(false,
read_callback_called_);
nit: EXPECT_FALSE ?

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:181: } while (++loop_count < 10 &&
ditto re: 10

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:322: empty_text_buffers_,
nit: Looks like a helper method could clean this up a bit. Many of the
parameters don't seem to vary across call sites.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:394: // TODO(wolenetz): Determine why
we do not need to seek to 50ms, or if this is
This is to deal with content that doesn't always start at exactly 0. See
kSeekToStartFudgeRoom() in source_buffer_stream.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:430: // TODO(wolenetz): Determine why
we do not need to seek to 50ms / 70ms, or if
ditto

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:526: // in SourceBufferStream.
Not sure what is going on here. What happens if you don't seek? Do you get 20 &
30?

[wolenetz, 2014-05-08 02:35:27]

PTAL @ PS11

Thanks!

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:29: //
https://dvcs.w3.org/hg/html-media/raw-file/d471a4412040/media-source/media-so...
On 2014/05/08 00:06:43, acolwell wrote:
> nit: You can use
>
https://dvcs.w3.org/hg/html-media/raw-file/d471a4412040/media-source/media-so...
> to reference the attribute directly.

Done (mode attribute, not timestampOffset)

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:173: // Frame PTS is only updated if it
survives processing.
On 2014/05/08 00:06:43, acolwell wrote:
> nit:s/Frame/Note: |frame|/  so it is clear that this isn't part of the spec
> text?

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:190: track_id = kVideoTrackId;
On 2014/05/08 00:06:43, acolwell wrote:
> Can't this and the audio ID be hidden in frame->track_id() or at least where
> |frame| is created.

For audio/video track buffers, stream parsers currently fill in their own track
ids. There is no indirection yet. SourceState::OnNewConfigs() enforces max 1
track each for audio and video, and assigns them these constant track IDs. It
seems to me that cleaning this up and allowing for possibly multiple tracks and
enabling/disabling them is a big chunk best left for later CLs.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:293: presentation_timestamp =
append_window_start;  // Frame updated, below.
On 2014/05/08 00:06:43, acolwell wrote:
> nit: s/Frame/|frame|/ here and below

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:295: frame_duration = frame_end_timestamp -
append_window_start;
On 2014/05/08 00:06:43, acolwell wrote:
> nit: Perhaps use presentation_timestamp here since frame_end_timestamp was set
> via presentation_timestamp + frame_duration above?

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.cc:347:
track_buffer->stream()->Append(buffer_to_append);
On 2014/05/08 00:06:43, acolwell wrote:
> This is OK for now, but I'm a little concerned about a perf hit from doing so
> many single buffer appends. In a followup CL we may want to consider
collecting
> the buffers in |track_buffer| and then committing them in a single Append() on
> each track_buffer before ProcessFrames() returns.

Good point. I've adjusted the TODO to reference new crbug.com/371197 for getting
this fixed.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor.h (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor.h:53: bool group_start_timestamp_set_;
On 2014/05/08 00:06:43, acolwell wrote:
> nit: Any reason not to use kNoTimestamp() to track unset state?

None now. Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_base.h:126: virtual void
SetGroupStartTimestampIfInSequenceMode(
On 2014/05/08 00:06:43, acolwell wrote:
> Does this really need to be virtual? I don't think it hurts anything to simply
> have the implementation here even if the legacy impl will never call it.

WSBI/ChunkDemuxer/SourceState calls this regardless of which frame processor
(legacy/new) is being used internally by ChunkDemuxer/SourceState.
We could put the legacy implementation as default implementation here, but would
still need to override in FrameProcessor since |group_start_timestamp_| is
private to FrameProcessor.

Am I perhaps misunderstanding your comment?

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:1: // Copyright (c) 2014 The Chromium
Authors. All rights reserved.
On 2014/05/08 00:06:43, acolwell wrote:
> nit: s/(c) //

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:68: void AddTestTracks(const bool
has_audio, const bool has_video) {
On 2014/05/08 00:06:43, acolwell wrote:
> nit: Use HAS_AUDIO & HAS_VIDEO flags like ChunkDemuxer unit test so the call
> sites are a little clearer & self documenting.

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:154: } while (++loop_count < 10 &&
read_callback_called_ &&
On 2014/05/08 00:06:43, acolwell wrote:
> Do you really need 10 here? Why would you expect more than 1 abort?

Done. While debugging and getting strict mocks and seeks, etc working, I wasn't
sure how many might occur. Limiting to max 1 kAborted + 1 non-kAborted here and
below, now.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:160: EXPECT_EQ(false,
read_callback_called_);
On 2014/05/08 00:06:43, acolwell wrote:
> nit: EXPECT_FALSE ?

yep. linux_chromium*_rel try bots also failed to compile due to this.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:181: } while (++loop_count < 10 &&
On 2014/05/08 00:06:43, acolwell wrote:
> ditto re: 10

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:322: empty_text_buffers_,
On 2014/05/08 00:06:43, acolwell wrote:
> nit: Looks like a helper method could clean this up a bit. Many of the
> parameters don't seem to vary across call sites.

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:394: // TODO(wolenetz): Determine why
we do not need to seek to 50ms, or if this is
On 2014/05/08 00:06:43, acolwell wrote:
> This is to deal with content that doesn't always start at exactly 0. See
> kSeekToStartFudgeRoom() in source_buffer_stream.

Makes sense. Thanks!

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:430: // TODO(wolenetz): Determine why
we do not need to seek to 50ms / 70ms, or if
On 2014/05/08 00:06:43, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:526: // in SourceBufferStream.
On 2014/05/08 00:06:43, acolwell wrote:
> Not sure what is going on here. What happens if you don't seek? Do you get 20
&
> 30?

Correct, only 20 then 30 are read, then unexpected stall causes failure.

[acolwell GONE FROM CHROMIUM, 2014-05-08 17:32:22]

Looks good. Just a few more comments

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_base.h:126: virtual void
SetGroupStartTimestampIfInSequenceMode(
On 2014/05/08 02:35:28, wolenetz wrote:
> On 2014/05/08 00:06:43, acolwell wrote:
> > Does this really need to be virtual? I don't think it hurts anything to
simply
> > have the implementation here even if the legacy impl will never call it.
> 
> WSBI/ChunkDemuxer/SourceState calls this regardless of which frame processor
> (legacy/new) is being used internally by ChunkDemuxer/SourceState.
> We could put the legacy implementation as default implementation here, but
would
> still need to override in FrameProcessor since |group_start_timestamp_| is
> private to FrameProcessor.
> 
> Am I perhaps misunderstanding your comment?
You could simply lift group_start_timestamp_ into this class instead. It seems
like there is no harm in that especially since FrameProcessorBase and
FrameProcessor will eventually be merged and the legacy one doesn't seem to care
about this value.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:526: // in SourceBufferStream.
On 2014/05/08 02:35:28, wolenetz wrote:
> On 2014/05/08 00:06:43, acolwell wrote:
> > Not sure what is going on here. What happens if you don't seek? Do you get
20
> &
> > 30?
> 
> Correct, only 20 then 30 are read, then unexpected stall causes failure.
Ok. This is what I'd expect because I believe the SourceBufferStream is
completing its seek when it sees the first append that satisfies the initial
seek condition. We could probably improve upon this to defer this decision until
the next read, but that will likely be tricky and is definitely for a different
CL.

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
media/filters/frame_processor.cc:92: // for a single input frame per April 1,
2014 MSE spec editor's draft:
Please make sure you have a bug filed to track updating the algorithm to contain
the changes made in the April 29th version.

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
media/filters/frame_processor.cc:342: // possible. See http://crbug.com/371197.
Another part of this may entail refactoring SourceBufferStream to closer reflect
what is written in the spec and defer things like GC to run at a different time
like the spec indicates.

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:553:
CheckExpectedRangesByTimestamp(video_.get(), "{ [0,20) [60,70) }");
Why is a gap inserted here? I wouldn't expect a gap to get created in sequence
mode. The whole idea is that this mode is supposed to help avoid gaps caused by
discontinuities.

[wolenetz, 2014-05-08 19:59:01]

PTAL

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_base.h:126: virtual void
SetGroupStartTimestampIfInSequenceMode(
On 2014/05/08 17:32:22, acolwell wrote:
> On 2014/05/08 02:35:28, wolenetz wrote:
> > On 2014/05/08 00:06:43, acolwell wrote:
> > > Does this really need to be virtual? I don't think it hurts anything to
> simply
> > > have the implementation here even if the legacy impl will never call it.
> > 
> > WSBI/ChunkDemuxer/SourceState calls this regardless of which frame processor
> > (legacy/new) is being used internally by ChunkDemuxer/SourceState.
> > We could put the legacy implementation as default implementation here, but
> would
> > still need to override in FrameProcessor since |group_start_timestamp_| is
> > private to FrameProcessor.
> > 
> > Am I perhaps misunderstanding your comment?
> You could simply lift group_start_timestamp_ into this class instead. It seems
> like there is no harm in that especially since FrameProcessorBase and
> FrameProcessor will eventually be merged and the legacy one doesn't seem to
care
> about this value.

Done.

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/190001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:526: // in SourceBufferStream.
On 2014/05/08 17:32:22, acolwell wrote:
> On 2014/05/08 02:35:28, wolenetz wrote:
> > On 2014/05/08 00:06:43, acolwell wrote:
> > > Not sure what is going on here. What happens if you don't seek? Do you get
> 20
> > &
> > > 30?
> > 
> > Correct, only 20 then 30 are read, then unexpected stall causes failure.
> Ok. This is what I'd expect because I believe the SourceBufferStream is
> completing its seek when it sees the first append that satisfies the initial
> seek condition. We could probably improve upon this to defer this decision
until
> the next read, but that will likely be tricky and is definitely for a
different
> CL.

Ok. I filed and referenced crbug 371493 here.

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
media/filters/frame_processor.cc:92: // for a single input frame per April 1,
2014 MSE spec editor's draft:
On 2014/05/08 17:32:22, acolwell wrote:
> Please make sure you have a bug filed to track updating the algorithm to
contain
> the changes made in the April 29th version.

Good point. I filed bug 371499 and referenced it now from the
auto_update_timestamp_offset_ definition in SourceState.

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
media/filters/frame_processor.cc:342: // possible. See http://crbug.com/371197.
On 2014/05/08 17:32:22, acolwell wrote:
> Another part of this may entail refactoring SourceBufferStream to closer
reflect
> what is written in the spec and defer things like GC to run at a different
time
> like the spec indicates.

Good point. I've added your comment to crbug.com/371197 and updated this source
comment. That bug may eventually become split into more pieces (or get some
dependents) to track these potentially multiple concerns.

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
File media/filters/frame_processor_unittest.cc (right):

https://codereview.chromium.org/180153003/diff/210001/media/filters/frame_pro...
media/filters/frame_processor_unittest.cc:553:
CheckExpectedRangesByTimestamp(video_.get(), "{ [0,20) [60,70) }");
On 2014/05/08 17:32:22, acolwell wrote:
> Why is a gap inserted here? I wouldn't expect a gap to get created in sequence
> mode. The whole idea is that this mode is supposed to help avoid gaps caused
by
> discontinuities.

You're correct. As we discovered offline, sequence mode processing was still
telling Chunk/SourceBufferStream to begin a new media segment just prior to the
next CDS::append after discontinuity. Next PS includes our fix.

[acolwell GONE FROM CHROMIUM, 2014-05-08 20:28:49]

lgtm

[wolenetz, 2014-05-08 20:30:46]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-05-08 20:36:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wolenetz@chromium.org/180153003/250001

[wolenetz, 2014-05-08 21:13:29]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-05-08 21:15:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wolenetz@chromium.org/180153003/270001

[commit-bot: I haz the power, 2014-05-09 01:50:36]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[wolenetz, 2014-05-09 20:44:24]

Committed patchset #14 manually as r269397 (presubmit successful).