Allow StreamParsers to request automatic timestampOffset updates.

Allow StreamParsers to request automatic timestampOffset updates.

Extends StreamParser::InitCB with a new parameter which indicates
that SourceState::OnNewBuffers() should update the timestampOffset
based on the earliest end time of returned buffers (accounting for
any current appendWindow).

To prevent doubling of the timestamps in conjuction with the update
to timestampOffset, the MPEGStreamParserBase now resets the timestamp
helper after each SendBuffers() call.

Also adds missing pipeline integration tests for ADTS.

BUG=353307
TEST=existing tests pass.  Post mp3 append, timestampOffset is updated.

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

[acolwell GONE FROM CHROMIUM, 2014-03-17 22:52:17]

looks good. Please add a test to verify the timestamp offset update.
PipelineIntegrationTest or a LayoutTest would probably be the easiest.

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:664: base::TimeDelta new_timestamp_offset =
nit: Drop base:: since we've got a using directive above.

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:667: : audio_buffers.back()->timestamp() +
You need an empty guard here if we only happened to get text track data. How
about something like this?

TimeDelta audio_end = audio_buffers.empty() ? kNoTimestamp() :
audio_buffers.back()->timestamp() + audio_buffers.back()->duration();
TimeDelta video_end = video_buffers.empty() ? kNoTimestamp() :
video_buffers.back()->timestamp() + video_buffers.back()->duration();

TimeDelta new_timestamp_offset = audio_end;
if (new_timestamp_offset == kNoTimestamp()) {
  new_timestamp_offset = video_end;
} else if (video_end != kNoTimestamp() && video_end < audio_end)
  new_timestamp_offset = video_end;
}

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:675: *timestamp_offset_during_append_ =
new_timestamp_offset;
You might need to snapshot *timestamp_offset_during_append_ before the
ProcessFrames() call and only update it if the *timestamp_offset_during_append_
matches the snapshot. You might want to check w/ Matt about how this logic
should interact with his new coded frame processing algorithm that can also
update the offset.

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:1462: bool auto_update_timestamp_offset) {
nit: No need to expose this detail to ChunkDemuxer. Just leave this signature as
is and have SourceState provide a typedef for its own InitCB instead of using
the StreamParser::InitCB.

https://codereview.chromium.org/195973006/diff/1/media/formats/mp4/mp4_stream...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/195973006/diff/1/media/formats/mp4/mp4_stream...
media/formats/mp4/mp4_stream_parser_unittest.cc:64: DVLOG(1) << "OnInit: ok=" <<
init_ok
nit: s/OnInit/InitF since that is the actual name of the function.

https://codereview.chromium.org/195973006/diff/1/media/formats/webm/webm_stre...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/195973006/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:218: init_cb_.Run(true, duration,
false);
Please change this code to use base::ResetAndReturn and remove the line below.

[DaleCurtis, 2014-03-18 00:14:02]

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:664: base::TimeDelta new_timestamp_offset =
On 2014/03/17 22:52:17, acolwell wrote:
> nit: Drop base:: since we've got a using directive above.

Done.

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:667: : audio_buffers.back()->timestamp() +
On 2014/03/17 22:52:17, acolwell wrote:
> You need an empty guard here if we only happened to get text track data. How
> about something like this?
> 
> TimeDelta audio_end = audio_buffers.empty() ? kNoTimestamp() :
> audio_buffers.back()->timestamp() + audio_buffers.back()->duration();
> TimeDelta video_end = video_buffers.empty() ? kNoTimestamp() :
> video_buffers.back()->timestamp() + video_buffers.back()->duration();
> 
> TimeDelta new_timestamp_offset = audio_end;
> if (new_timestamp_offset == kNoTimestamp()) {
>   new_timestamp_offset = video_end;
> } else if (video_end != kNoTimestamp() && video_end < audio_end)
>   new_timestamp_offset = video_end;
> }

Reflowed to a multi-if statement + helper function which I think reads a lot
more clearly.

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:675: *timestamp_offset_during_append_ =
new_timestamp_offset;
On 2014/03/17 22:52:17, acolwell wrote:
> You might need to snapshot *timestamp_offset_during_append_ before the
> ProcessFrames() call and only update it if the
*timestamp_offset_during_append_
> matches the snapshot. You might want to check w/ Matt about how this logic
> should interact with his new coded frame processing algorithm that can also
> update the offset.

+wolenetz, what do you think? Probably we both shouldn't be updating the
timestamp offset.  For now I'll guard by checking prev==curr.

https://codereview.chromium.org/195973006/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:1462: bool auto_update_timestamp_offset) {
On 2014/03/17 22:52:17, acolwell wrote:
> nit: No need to expose this detail to ChunkDemuxer. Just leave this signature
as
> is and have SourceState provide a typedef for its own InitCB instead of using
> the StreamParser::InitCB.

Done.

https://codereview.chromium.org/195973006/diff/1/media/formats/mp4/mp4_stream...
File media/formats/mp4/mp4_stream_parser_unittest.cc (right):

https://codereview.chromium.org/195973006/diff/1/media/formats/mp4/mp4_stream...
media/formats/mp4/mp4_stream_parser_unittest.cc:64: DVLOG(1) << "OnInit: ok=" <<
init_ok
On 2014/03/17 22:52:17, acolwell wrote:
> nit: s/OnInit/InitF since that is the actual name of the function.

Done.

https://codereview.chromium.org/195973006/diff/1/media/formats/webm/webm_stre...
File media/formats/webm/webm_stream_parser.cc (right):

https://codereview.chromium.org/195973006/diff/1/media/formats/webm/webm_stre...
media/formats/webm/webm_stream_parser.cc:218: init_cb_.Run(true, duration,
false);
On 2014/03/17 22:52:17, acolwell wrote:
> Please change this code to use base::ResetAndReturn and remove the line below.

Done.

[acolwell GONE FROM CHROMIUM, 2014-03-18 00:21:54]

lgtm % nits

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

https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:101: typedef base::Callback<void(bool,
base::TimeDelta)> InitCB;
nit: drop base::

https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:697: init_cb_.Run(success, duration);
nit: Use base::ResetAndReturn(&init_cb_) here.

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

https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.h:278: base::TimeDelta duration);
nit: Revert this change since it fits on one line.

[wolenetz, 2014-03-18 00:47:55]

On 2014/03/18 00:21:54, acolwell wrote:
> lgtm % nits
> 
>
https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
> media/filters/chunk_demuxer.cc:101: typedef base::Callback<void(bool,
> base::TimeDelta)> InitCB;
> nit: drop base::
> 
>
https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
> media/filters/chunk_demuxer.cc:697: init_cb_.Run(success, duration);
> nit: Use base::ResetAndReturn(&init_cb_) here.
> 
>
https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
> File media/filters/chunk_demuxer.h (right):
> 
>
https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
> media/filters/chunk_demuxer.h:278: base::TimeDelta duration);
> nit: Revert this change since it fits on one line.

chatted offline - the code assumes that all media segments begin with time 0
when passed to OnNewBuffers() when this auto timestamp offset update mode is
engaged. This probably needs verification/adjustment of the parser. Also, append
window begin/end filter what is actually sent by ProcessFrames() to the track
buffers, so the auto adjustment of the timestamp offset will need to similarly
observe the append window. Sequence mode coded frame processing algorithm, once
it lands, will also need indication that the timestamp offset has been
auto-adjusted each time it changes, so that "sequence" mode processing behaves.

[DaleCurtis, 2014-03-18 01:26:56]

New patch set:
- Uses a minimized version of LegacyFrameProcessor's filtering logic to compute
the correct timestamp offset when append windows are present.
- Resets the AudioTimestampHelper for each SendBuffers() call within the
MPEGAudioStreamHelper.

With these changes, things seem to work properly both in unittests and with my
demo page.

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

https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:101: typedef base::Callback<void(bool,
base::TimeDelta)> InitCB;
On 2014/03/18 00:21:54, acolwell wrote:
> nit: drop base::

Done.

https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:697: init_cb_.Run(success, duration);
On 2014/03/18 00:21:54, acolwell wrote:
> nit: Use base::ResetAndReturn(&init_cb_) here.

Done.

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

https://codereview.chromium.org/195973006/diff/20001/media/filters/chunk_demu...
media/filters/chunk_demuxer.h:278: base::TimeDelta duration);
On 2014/03/18 00:21:54, acolwell wrote:
> nit: Revert this change since it fits on one line.

Done.

[wolenetz, 2014-03-18 02:09:30]

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

https://codereview.chromium.org/195973006/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:685: const bool success =
nit: If |success| is false, this indicates a parse error. In that case, not all
of the frames may have been processed successfully, so probably we should abort
the auto timestampOffset update process, too.

https://codereview.chromium.org/195973006/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:701: const TimeDelta video_end =
EndTimestamp(audio_buffers,
s/audio_buf/video_buf/

[DaleCurtis, 2014-03-18 02:16:04]

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

https://codereview.chromium.org/195973006/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:685: const bool success =
On 2014/03/18 02:09:30, wolenetz wrote:
> nit: If |success| is false, this indicates a parse error. In that case, not
all
> of the frames may have been processed successfully, so probably we should
abort
> the auto timestampOffset update process, too.

Done.

https://codereview.chromium.org/195973006/diff/40001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:701: const TimeDelta video_end =
EndTimestamp(audio_buffers,
On 2014/03/18 02:09:30, wolenetz wrote:
> s/audio_buf/video_buf/

Done.

[wolenetz, 2014-03-18 17:33:15]

lgtm

[DaleCurtis, 2014-03-18 17:49:55]

acolwell: Still lgty?

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

https://codereview.chromium.org/195973006/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:41: if (buffer->timestamp() >=
append_window_start &&
This will iterate more than necessary if |buffers| is completely outside the
append window, but the code is simpler.  If you prefer to avoid the iteration,
let me know.

[DaleCurtis, 2014-03-18 19:47:21]

Okay, per offline conversation, the appendWindow changes have been reverted. 
PTAL.

[acolwell GONE FROM CHROMIUM, 2014-03-18 19:57:15]

lgtm % nit

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

https://codereview.chromium.org/195973006/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:663: // Update the timestamp offset for
audio/video tracks if it hasn't already
nit: Please fix comment since you are only computing the potential new offset
here and not actually doing an update.

[wolenetz, 2014-03-18 20:23:32]

lgtm % nits:

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

https://codereview.chromium.org/195973006/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:666: const bool have_audio_buffers =
!audio_buffers.empty();
nit: move these bools and their initializations into the if (auto...){} clause,
below, since they are only used there.

https://codereview.chromium.org/195973006/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:690: if (timestamp_offset_before_processing ==
*timestamp_offset_during_append_)
nit: though currently correct, add condition auto_update_timestamp_offset_ &&
[rest of if clause]?

[DaleCurtis, 2014-03-18 20:37:57]

Thanks for the reviews!

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

https://codereview.chromium.org/195973006/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:663: // Update the timestamp offset for
audio/video tracks if it hasn't already
On 2014/03/18 19:57:16, acolwell wrote:
> nit: Please fix comment since you are only computing the potential new offset
> here and not actually doing an update.

Done.

https://codereview.chromium.org/195973006/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:666: const bool have_audio_buffers =
!audio_buffers.empty();
On 2014/03/18 20:23:33, wolenetz wrote:
> nit: move these bools and their initializations into the if (auto...){}
clause,
> below, since they are only used there.

Done.

https://codereview.chromium.org/195973006/diff/110001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:690: if (timestamp_offset_before_processing ==
*timestamp_offset_during_append_)
On 2014/03/18 20:23:33, wolenetz wrote:
> nit: though currently correct, add condition auto_update_timestamp_offset_ &&
> [rest of if clause]?

Done.

[DaleCurtis, 2014-03-18 20:40:03]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:42:53]

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

[commit-bot: I haz the power, 2014-03-18 21:27:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:27:47]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[DaleCurtis, 2014-03-18 21:33:51]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-18 21:45:05]

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

[commit-bot: I haz the power, 2014-03-18 22:30:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 22:30:53]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[DaleCurtis, 2014-03-18 23:28:43]

acolwell: PTAL at the ADTS and MP3 StreamParser unittests, they now have a lot
of 0K timestamps :)

[acolwell GONE FROM CHROMIUM, 2014-03-18 23:50:22]

On 2014/03/18 23:28:43, DaleCurtis wrote:
> acolwell: PTAL at the ADTS and MP3 StreamParser unittests, they now have a lot
> of 0K timestamps :)

Could we also modify those tests to validate the buffered ranges. With all those
zeros it would be good to make sure the buffered range is actually what we
expect.

[DaleCurtis, 2014-03-19 00:01:02]

Can you elaborate?  These tests aren't wired into a real ChunkDemuxer, so they
don't actually have a buffered range.

[DaleCurtis, 2014-03-19 00:19:46]

The latest patch set fakes ranges calculation in the StreamParserTestBase, is
this what you were thinking?

[acolwell GONE FROM CHROMIUM, 2014-03-19 00:35:06]

On 2014/03/19 00:19:46, DaleCurtis wrote:
> The latest patch set fakes ranges calculation in the StreamParserTestBase, is
> this what you were thinking?

Oh. Sorry. I forgot that these are simply parser tests and don't involve
ChunkDemuxer. Checking the buffered range start and end in the pipeline
integration test should be sufficient. Sorry for the churn.

[DaleCurtis, 2014-03-19 00:45:27]

Done. ADTS tests were missing, so I had to add them.

[acolwell GONE FROM CHROMIUM, 2014-03-19 00:56:27]

lgtm

[DaleCurtis, 2014-03-19 00:58:30]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-19 01:00:21]

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

[commit-bot: I haz the power, 2014-03-19 01:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 01:41:58]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[DaleCurtis, 2014-03-19 01:44:30]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-03-19 01:44:54]

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

[commit-bot: I haz the power, 2014-03-19 11:39:05]

Change committed as 257932