Extract coded frame processing from SourceState into LegacyFrameProcessor

Extract coded frame processing from SourceState into LegacyFrameProcessor

In preparation for compliant coded frame processing implementation,
this change extracts the previous coded frame processing implementation
out of SourceState into FrameProcessorBase and LegacyFrameProcessor.
ChunkDemuxerStream is also lifted into chunk_demuxer.h so that the
frame processing algorithm may continue to directly use those streams.

R=acolwell@chromium.org
BUG=249422
TEST=All media unittests and http/tests/media layout tests pass locally on Linux

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

[wolenetz, 2014-03-07 21:39:51]

Please take a look. One of the next changes will be to introduce a new subclass
of FrameProcessorBase that implements a more compliant version of coded frame
processing, while keeping the LegacyFrameProcessor around to not break folks
while the new implementation stabilizes.

[acolwell GONE FROM CHROMIUM, 2014-03-08 21:15:51]

looks pretty good. Just a few comments on my initial pass.

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

https://codereview.chromium.org/191513002/diff/10001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:107: // error occurred. Append processing in the
|frame_processor_| uses a cached
nit: Why don't we just have the frame processor use this value instead of the
cached one? 

Perhaps I misinterpreted an earlier discussion we had this week. I think you
should at least make Chromium-side changes in WebSourceBufferImpl to only call
an IsParsingMediaSegment() method on ChunkDemuxer in response to
SetTimestampOffset() calls. Then this code could be updated to always use the
value passed into Append(). I think that is a reasonable tradeoff and would
avoid the frame processor from having to deal with the cached value.

https://codereview.chromium.org/191513002/diff/10001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:647: timestamp_offset_during_append_);
nit: I think you should just pass the append window start and end in this call
as well. We know all implementations will need to use these values. Might as
well just allow this code to handle the caching. In the future, I think
WebSourceBuffer::append() should probably carry these values as well so that we
don't have to cache them even at this layer.

I also think you should s/MergeQueuesAnd// since the legacy implementation
doesn't actually merge the queues.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:23: class TrackBufferAppendState {
nit: How about we just call this MseTrackBuffer?

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:76: // Get/set the current append mode.
Before setting, caller must do steps 1-6
This comment seems overly verbose. I think simply indicating that false maps to
"segments" and true maps to "sequence" is sufficient. The reader can look at the
spec to understand what these modes imply.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:109: virtual void SetTimestampOffset(const
base::TimeDelta& timestamp_offset) = 0;
I think this should be removed and the value passed to ProcessFrames() should be
used instead.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:146: typedef
std::map<StreamParser::TrackId, TrackBufferAppendState*> TrackMap;
nit: s/TrackMap/TrackBufferMap/?

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:173: TrackMap tracks_;
nit: s/tracks_/track_buffers_/ just so the code will roughly mirror what the
spec says.

[wolenetz, 2014-03-10 23:03:47]

Thanks for great comments! Please take a look at patch set 3:

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

https://codereview.chromium.org/191513002/diff/10001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:107: // error occurred. Append processing in the
|frame_processor_| uses a cached
On 2014/03/08 21:15:51, acolwell wrote:
> nit: Why don't we just have the frame processor use this value instead of the
> cached one? 
> 
> Perhaps I misinterpreted an earlier discussion we had this week. I think you
> should at least make Chromium-side changes in WebSourceBufferImpl to only call
> an IsParsingMediaSegment() method on ChunkDemuxer in response to
> SetTimestampOffset() calls. Then this code could be updated to always use the
> value passed into Append(). I think that is a reasonable tradeoff and would
> avoid the frame processor from having to deal with the cached value.

Good point. Done: I've updated to cache the offset in WSBI.
I also updated WSBI::setMode() to similarly use CD::IsParsingMediaSegment() and
CD..LegacyFrameProcessor::SetSequenceMode() to return void, not bool (this helps
keep ChunkDemuxerTest changes similar, too).

A couple notes that you're probably already aware of:

1. The prefixed MSE append implementation does not allow sequence mode or
recognize any timestamp offset updates originated on chromium side, so the
eventual new FrameProcessor must NOT update timestamp offset during
ProcessFrames() (or any other operations) when there has never been any
transition to 'sequence' mode.

2. The new FrameProcessor (not the legacy one) will need a signal when
WSBI::setTimestampOffset() occurs (or has occurred since last append) for step 6
of http://www.w3.org/TR/media-source/#widl-SourceBuffer-timestampOffset. That,
of course, can come in the later CL that adds that new FrameProcessor, since the
LegacyFrameProcessor needs no such logic: it doesn't concern itself with the
spec's "group start timestamp".

https://codereview.chromium.org/191513002/diff/10001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:647: timestamp_offset_during_append_);
On 2014/03/08 21:15:51, acolwell wrote:
> nit: I think you should just pass the append window start and end in this call
> as well. We know all implementations will need to use these values. Might as
> well just allow this code to handle the caching. In the future, I think
> WebSourceBuffer::append() should probably carry these values as well so that
we
> don't have to cache them even at this layer.
> 
> I also think you should s/MergeQueuesAnd// since the legacy implementation
> doesn't actually merge the queues.

Done and done.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:23: class TrackBufferAppendState {
On 2014/03/08 21:15:51, acolwell wrote:
> nit: How about we just call this MseTrackBuffer?

Done.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:76: // Get/set the current append mode.
Before setting, caller must do steps 1-6
On 2014/03/08 21:15:51, acolwell wrote:
> This comment seems overly verbose. I think simply indicating that false maps
to
> "segments" and true maps to "sequence" is sufficient. The reader can look at
the
> spec to understand what these modes imply.

Done.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:109: virtual void SetTimestampOffset(const
base::TimeDelta& timestamp_offset) = 0;
On 2014/03/08 21:15:51, acolwell wrote:
> I think this should be removed and the value passed to ProcessFrames() should
be
> used instead.

Done.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:146: typedef
std::map<StreamParser::TrackId, TrackBufferAppendState*> TrackMap;
On 2014/03/08 21:15:51, acolwell wrote:
> nit: s/TrackMap/TrackBufferMap/?

Done.

https://codereview.chromium.org/191513002/diff/10001/media/filters/frame_proc...
media/filters/frame_processor_base.h:173: TrackMap tracks_;
On 2014/03/08 21:15:51, acolwell wrote:
> nit: s/tracks_/track_buffers_/ just so the code will roughly mirror what the
> spec says.

Done.

[acolwell GONE FROM CHROMIUM, 2014-03-11 20:00:37]

looks pretty good. Just a few comments.

https://codereview.chromium.org/191513002/diff/20001/content/renderer/media/w...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/191513002/diff/20001/content/renderer/media/w...
content/renderer/media/websourcebuffer_impl.cc:50: break;
nit: remove break here and below.

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

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:112: const base::TimeDelta& append_window_start,
ditto

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:124: bool IsParsingMediaSegment() const { return
parsing_media_segment_; }
nit: unix_hacker_style since this is a simple accessor.

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:283: bool err = stream_parser_->Parse(data,
length);
nit: Add a TODO here to change the StreamParser::Parse() signature to take a
NewBuffersCB so that you can simply bind the window and timestamp_offset vars
instead of keeping them as member variables. You can assign this to me if you'd
like.

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.h:204: const base::TimeDelta& append_window_start,
nit: s/const base::TimeDelta&/base::TimeDelta/

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (left):

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:2587:
ASSERT_TRUE(demuxer_->SetTimestampOffset(
nit: Create a SetTimestampOffset() method in the test class to reduce the amount
of duplicate code. I think that should greatly reduce the changes needed in this
file.

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
media/filters/frame_processor_base.h:27: // Get/set
|needs_random_access_point_|.
nit: Add empty line between previous method and comment so they don't blend into
the comments.

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
media/filters/frame_processor_base.h:47: // Pointer to the stream associated
with this track. The stream is not owned
ditto.

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
media/filters/frame_processor_base.h:79: // TODO(wolenetz): Actually implement
"sequence" mode compliant coded frame
nit: The base class seems like a weird place for this TODO comment. How about
just dropping the TODO since we know you are going to implement an alternate
processor shortly.

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.cc (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.cc:88: *new_media_segment = false;
hmm. It is unfortunate that we need to pass this state down especially since I
don't believe the coded frame processing algorithm in the spec even cares about
media segment boundaries. We should probably have a TODO to investigate whether
we can remove this. I'm pretty sure the SourceBufferStream only needs this info
to deal with weird edge cases. We should probably see if we can find a different
way to address them once the new algorithm is in place.

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.h (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:28: const base::TimeDelta&
append_window_start,
ditto

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:45: void FilterWithAppendWindow(const
base::TimeDelta& append_window_start,
ditto

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:69: const base::TimeDelta&
append_window_start,
ditto

[wolenetz, 2014-03-12 00:46:14]

Please take a look.
This CL now requires landing https://codereview.chromium.org/196173002/ first
(currently that CL is in CQ).

Thank you!

https://codereview.chromium.org/191513002/diff/20001/content/renderer/media/w...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/191513002/diff/20001/content/renderer/media/w...
content/renderer/media/websourcebuffer_impl.cc:50: break;
On 2014/03/11 20:00:37, acolwell wrote:
> nit: remove break here and below.

Done in the prereq CL I split out of this one.

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

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:112: const base::TimeDelta& append_window_start,
On 2014/03/11 20:00:37, acolwell wrote:
> ditto

Done in the prereq CL I split out of this.

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:124: bool IsParsingMediaSegment() const { return
parsing_media_segment_; }
On 2014/03/11 20:00:37, acolwell wrote:
> nit: unix_hacker_style since this is a simple accessor.

Done in the prereq CL I split out of this.

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:283: bool err = stream_parser_->Parse(data,
length);
On 2014/03/11 20:00:37, acolwell wrote:
> nit: Add a TODO here to change the StreamParser::Parse() signature to take a
> NewBuffersCB so that you can simply bind the window and timestamp_offset vars
> instead of keeping them as member variables. You can assign this to me if
you'd
> like.

Done in prereq CL I split out of this. I assigned it to me
(http://crbug.com/351454).

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.h:204: const base::TimeDelta& append_window_start,
On 2014/03/11 20:00:37, acolwell wrote:
> nit: s/const base::TimeDelta&/base::TimeDelta/

Done in the prereq CL I split out of this.

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
File media/filters/chunk_demuxer_unittest.cc (left):

https://codereview.chromium.org/191513002/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer_unittest.cc:2587:
ASSERT_TRUE(demuxer_->SetTimestampOffset(
On 2014/03/11 20:00:37, acolwell wrote:
> nit: Create a SetTimestampOffset() method in the test class to reduce the
amount
> of duplicate code. I think that should greatly reduce the changes needed in
this
> file.

Done in prereq CL I split off from this one.

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
File media/filters/frame_processor_base.h (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
media/filters/frame_processor_base.h:27: // Get/set
|needs_random_access_point_|.
On 2014/03/11 20:00:37, acolwell_OOO_til_3-15 wrote:
> nit: Add empty line between previous method and comment so they don't blend
into
> the comments.

Done.

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
media/filters/frame_processor_base.h:47: // Pointer to the stream associated
with this track. The stream is not owned
On 2014/03/11 20:00:37, acolwell_OOO_til_3-15 wrote:
> ditto.

Done.

https://codereview.chromium.org/191513002/diff/20001/media/filters/frame_proc...
media/filters/frame_processor_base.h:79: // TODO(wolenetz): Actually implement
"sequence" mode compliant coded frame
On 2014/03/11 20:00:37, acolwell_OOO_til_3-15 wrote:
> nit: The base class seems like a weird place for this TODO comment. How about
> just dropping the TODO since we know you are going to implement an alternate
> processor shortly.

Done.

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.cc (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.cc:88: *new_media_segment = false;
On 2014/03/11 20:00:37, acolwell wrote:
> hmm. It is unfortunate that we need to pass this state down especially since I
> don't believe the coded frame processing algorithm in the spec even cares
about
> media segment boundaries. We should probably have a TODO to investigate
whether
> we can remove this. I'm pretty sure the SourceBufferStream only needs this
info
> to deal with weird edge cases. We should probably see if we can find a
different
> way to address them once the new algorithm is in place.

I added a TODO @SourceState::new_media_segment_ in the prereq CL I split off
from this one. (I filed http://crbug.com/351489).

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.h (right):

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:28: const base::TimeDelta&
append_window_start,
On 2014/03/11 20:00:37, acolwell_OOO_til_3-15 wrote:
> ditto

Done.

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:45: void FilterWithAppendWindow(const
base::TimeDelta& append_window_start,
On 2014/03/11 20:00:37, acolwell_OOO_til_3-15 wrote:
> ditto

Done.

https://codereview.chromium.org/191513002/diff/20001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:69: const base::TimeDelta&
append_window_start,
On 2014/03/11 20:00:37, acolwell_OOO_til_3-15 wrote:
> ditto

Done.

[acolwell GONE FROM CHROMIUM, 2014-03-12 01:12:10]

lgtm % nits

https://codereview.chromium.org/191513002/diff/60001/media/filters/frame_proc...
File media/filters/frame_processor_base.cc (right):

https://codereview.chromium.org/191513002/diff/60001/media/filters/frame_proc...
media/filters/frame_processor_base.cc:64: if (itr == track_buffers_.end())
DCHECK instead? If this happens I think it's a bug. Right?

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.cc (right):

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.cc:115: if (!text_buffers.empty()) {
nit: reverse condition and continue.

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.h (right):

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:43: // items in |buffers|.
|*new_media_segment| is set true if any of |buffers|
The new_media_segment comment, while true, is definitely a sign that we need to
revisit this logic or at least clarify the terminology.

[wolenetz, 2014-03-12 02:08:29]

Thank you. I'll send PS5 to CQ once prerequisite CL
https://codereview.chromium.org/196173002/ has landed.

https://codereview.chromium.org/191513002/diff/60001/media/filters/frame_proc...
File media/filters/frame_processor_base.cc (right):

https://codereview.chromium.org/191513002/diff/60001/media/filters/frame_proc...
media/filters/frame_processor_base.cc:64: if (itr == track_buffers_.end())
On 2014/03/12 01:12:11, acolwell_OOO_til_3-15 wrote:
> DCHECK instead? If this happens I think it's a bug. Right?

Callers of FindTrack() condition their processing on result, including NULL.

Hmm. I suppose instead of AddTrack() returning false (== parse error) on
duplicate track id addition attempt, I could switch that logic in AddTrack() to
be a DCHECK since that *would* be a bug.

DCHECKing within FindTrack() would break regular lookup of audio or video track
in LegacyFrameProcessor::ProcessFrames, for a stream that is missing one or more
of [audio,video]. NULL FindTrack() retval for missing track is expected behavior
in that case.

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.cc (right):

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.cc:115: if (!text_buffers.empty()) {
On 2014/03/12 01:12:11, acolwell_OOO_til_3-15 wrote:
> nit: reverse condition and continue.

Done.

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
File media/filters/legacy_frame_processor.h (right):

https://codereview.chromium.org/191513002/diff/60001/media/filters/legacy_fra...
media/filters/legacy_frame_processor.h:43: // items in |buffers|.
|*new_media_segment| is set true if any of |buffers|
On 2014/03/12 01:12:11, acolwell_OOO_til_3-15 wrote:
> The new_media_segment comment, while true, is definitely a sign that we need
to
> revisit this logic or at least clarify the terminology.

I agree. I added a TODO() for this in the prereq CL referencing
http://crbug.com/351489 from SourceState::new_media_segment_. I've also updated
the bug due to this comment.

[wolenetz, 2014-03-12 13:41:59]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-03-12 13:42:01]

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

[commit-bot: I haz the power, 2014-03-12 16:55:32]

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

[commit-bot: I haz the power, 2014-03-12 17:44:02]

Change committed as 256569