Initialize media timeline correctly for positive start times.

Initialize media timeline correctly for positive start times.

The previous fix for positive start times http://crrev.com/93c37f1b is
incomplete. It fixed seeking, but did not fix rendering. As such, the
media timeline will start playback from 0 and advance in real time to
any positive start time... even if the actual start is hours away.

When testing I used a test clip with a short positive start time so
did not notice the issue.  In attempting to write a layout test, I
retested with a larger start time which revealed the deficiency in my
first solution.

This change reverts the removal of Demuxer::StartTime() with the added
restriction that it must always be positive.  Pipeline uses this time
like it did before, during initialization and as a minimum bound for
seeking.

BUG=413292
TEST=four new tests! layout test forthcoming.

Committed: https://crrev.com/b9ec078cf3b5355a1a19c55a75ebb6ab101b4483
Cr-Commit-Position: refs/heads/master@{#295159}

[DaleCurtis, 2014-09-15 22:31:28]

dalecurtis@chromium.org changed reviewers:
+ scherkus@chromium.org

[DaleCurtis, 2014-09-15 22:31:29]

scherkus, as discussed. acolwell FYI, I should have gotten to that layout test
sooner rather than later :(

[DaleCurtis, 2014-09-15 22:57:41]

LayoutTest here https://codereview.chromium.org/544173012/

[scherkus (not reviewing), 2014-09-16 00:07:31]

preliminary questions on tests -- still piecing together the change in my head

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:590: std::max(time, demuxer_->GetStartTime());
do we need to update pipeline_unittest.cc? IIRC we have a
StrictMock<MockDemuxer> and it'd be nice to have at least one test that verifies
we're using the right time

https://codereview.chromium.org/575643002/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/575643002/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:1575: ASSERT_EQ(base::TimeDelta(),
demuxer_->GetStartTime());
do we have (need?) tests that verify non-zero start time?

[DaleCurtis, 2014-09-16 00:10:07]

https://codereview.chromium.org/575643002/diff/1/media/filters/pipeline_integ...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/575643002/diff/1/media/filters/pipeline_integ...
media/filters/pipeline_integration_test.cc:1575: ASSERT_EQ(base::TimeDelta(),
demuxer_->GetStartTime());
On 2014/09/16 00:07:31, scherkus wrote:
> do we have (need?) tests that verify non-zero start time?

FFmpegDemuxer has a nonzero-start-time.webm we can try using as a
PipelineIntegrationTest.  I'll see about using it here.

[scherkus (not reviewing), 2014-09-16 00:16:51]

overall I think this is fine but I could use some extra Q&A'ing

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:351: DCHECK(start_timestamp_ >= base::TimeDelta());
is it actually possible to craft such a file? if so, what happens in release
build? is this something we care about?

(IOW, DCHECK -> CHECK?)

https://codereview.chromium.org/575643002/diff/1/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/575643002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:185: // timeline.
can you refresh my memory as to what's the impact of having negative timestamps
vs. just letting them flow through?

[DaleCurtis, 2014-09-16 01:17:50]

PTAL. There are now four tests covering all major players.

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:351: DCHECK(start_timestamp_ >= base::TimeDelta());
On 2014/09/16 00:16:51, scherkus wrote:
> is it actually possible to craft such a file? if so, what happens in release
> build? is this something we care about?
> 
> (IOW, DCHECK -> CHECK?)

I don't think it should be a CHECK, just a DCHECK. It's something that the API
contract says shouldn't be possible. It could only ever happen if Blink let
through a negative seek or GetStartTime() fails its contract.

https://codereview.chromium.org/575643002/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:590: std::max(time, demuxer_->GetStartTime());
On 2014/09/16 00:07:31, scherkus wrote:
> do we need to update pipeline_unittest.cc? IIRC we have a
> StrictMock<MockDemuxer> and it'd be nice to have at least one test that
verifies
> we're using the right time

Done.

https://codereview.chromium.org/575643002/diff/1/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/575643002/diff/1/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:185: // timeline.
On 2014/09/16 00:16:51, scherkus wrote:
> can you refresh my memory as to what's the impact of having negative
timestamps
> vs. just letting them flow through?

The HTML5 specification says we must only expose positive timestamps, letting
them through would expose the original issue: http://crbug.com/377295

[scherkus (not reviewing), 2014-09-16 18:55:41]

https://codereview.chromium.org/575643002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/575643002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:183: EXPECT_CALL(*demuxer_,
GetStartTime()).WillRepeatedly(Return(start_time_));
could we move these to Pipeline() along with the other demuxer_ default
expectations, and then override them as needed in tests that need to?

e.g.,

SetStartTimeExpectations(base::TimeDelta start_time) {
  start_time_ = start_time;
  EXPECT_CALL(*demuxer_, GetStartTime()).WillRepeatedly(Return(start_time_));
}

https://codereview.chromium.org/575643002/diff/20001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/575643002/diff/20001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:1600: // Tests that we signal ended
even when audio runs shorter than video track.
comment doesn't apply

[DaleCurtis, 2014-09-16 20:03:03]

https://codereview.chromium.org/575643002/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/575643002/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:183: EXPECT_CALL(*demuxer_,
GetStartTime()).WillRepeatedly(Return(start_time_));
On 2014/09/16 18:55:41, scherkus wrote:
> could we move these to Pipeline() along with the other demuxer_ default
> expectations, and then override them as needed in tests that need to?
> 
> e.g.,
> 
> SetStartTimeExpectations(base::TimeDelta start_time) {
>   start_time_ = start_time;
>   EXPECT_CALL(*demuxer_, GetStartTime()).WillRepeatedly(Return(start_time_));
> }

Nice, I didn't know you could override like this.  I guess they stack and
WillRepeatedly never exhausts.

https://codereview.chromium.org/575643002/diff/20001/media/filters/pipeline_i...
File media/filters/pipeline_integration_test.cc (right):

https://codereview.chromium.org/575643002/diff/20001/media/filters/pipeline_i...
media/filters/pipeline_integration_test.cc:1600: // Tests that we signal ended
even when audio runs shorter than video track.
On 2014/09/16 18:55:41, scherkus wrote:
> comment doesn't apply

Done.

[scherkus (not reviewing), 2014-09-16 20:09:20]

lgtm

thanks for writing tests!

[DaleCurtis, 2014-09-16 20:12:32]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-09-16 20:12:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/575643002/40001

[commit-bot: I haz the power, 2014-09-16 22:23:29]

Committed patchset #3 (id:40001) as f603e05bed57babbda3254d31b57c7ca56fe24d3

[commit-bot: I haz the power, 2014-09-16 22:24:18]

Patchset 3 (id:??) landed as
https://crrev.com/b9ec078cf3b5355a1a19c55a75ebb6ab101b4483
Cr-Commit-Position: refs/heads/master@{#295159}