Fix seeking when the start time is non-zero.

Fix seeking when the start time is non-zero.

The seek timestamp should always be adjusted by the start time,
though this manipulation should not be visible to external (web)
clients.

Since this is an FFmpeg only problem, I've removed the concept
of start time from the Demuxer interface in favor of local method
only for FFmpegDemuxer.  FFmpegDemuxerStream's will now use this
value to adjust timestamps such that external clients always see
a zero based timeline.

Doing so required moving some of our ogg vorbis discard code into
the FFmpegDemuxerStream, which actually makes it more accurate and
more narrowly scoped to ogg w/ vorbis instead of all vorbis.

These changes subtly change how we handle seeking.  Previously we
would let FFmpeg choose the stream to perform seeking within.  Now
we always use the stream with the lowest order timestamp.  This
means we will sometime use the audio stream where previously we
used the video stream.

I've extended the tests around non-zero start times to verify the
new behavior.

An FFmpeg DEPS roll is required for the new tests to pass:
9c12290 Revert "avformat/mp3dec: fix start time in light of initial skip samples"
1e661a6 avcodec/vorbisdec: Reset first_frame
7d88be4 avformat/oggparsevorbis: Dont attempt to calculate timestamps from gp=0

BUG=377295
TEST=new unittests. demo page from bug works.

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

[DaleCurtis, 2014-06-06 23:18:44]

http://i.imgur.com/ChzUb.jpg

[acolwell GONE FROM CHROMIUM, 2014-06-09 23:14:46]

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

https://codereview.chromium.org/325503003/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:760: clock_->SetTime(time, time);
This makes me really nervous and I'm pretty sure it is wrong. I suspect this
will break A/V sync if the start time != 0 unless there is something that is
subtracting off the start time in the demuxer. You should definitely do tests
with muxed A/V content that starts at something significantly greater than 0
before landing this just to make sure you aren't breaking things.

[DaleCurtis, 2014-06-10 20:26:54]

As discussed offline, I've localized start time manipulations to FFmpegDemuxer
and FFmpegDemuxerStream.  I've added tests for all the new behavior.  PTAL.

[acolwell GONE FROM CHROMIUM, 2014-06-10 23:20:39]

here are my initial round of comments. You might want to loop in scherkus@ on
this since this is a pretty fundamental change and he may have some insight into
why the code is the way it currently is.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:264: if (!buffer->end_of_stream() &&
!discard_helper_->initialized() &&
with your changes to the demuxer, can we DCHECK(buffer->timestamp() >=
base::TimeDelta() now just to make sure negative values can't sneak in anymore.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:251: buffer->set_timestamp(stream_timestamp -
demuxer_->start_time());
Does this hold up with chained Ogg? Does FFmpeg guarantee that timestamps will
always increase? I'm just wondering if it protects us from having to worry about
timestamps later in the stream that end up being < start_time()

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:442: start_time_(kInfiniteDuration()),
This seems a little out of place since we typically use kNoTimestamp() to signal
"not set". It seems like your use of kInfiniteDurat() should be constrained to
OnFindStreamInfoDone() so it is closer to the code that actually uses it for
special purposes.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:477: const int flags = AVSEEK_FLAG_BACKWARD;
Why not simply inline this below?

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:720:
streams_[i]->enable_negative_timestamp_discard();
nit: Just pass this via the constructor 2 lines up since this value isn't
expected to change during the life of the stream.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:747: start_time_ = base::TimeDelta();
If we don't know at this point, shouldn't we just try to defer this decision
until we are able to get the first packet from all of the streams? If we don't
it seems like we could accidentally trigger bad behavior if the first packets
happen to be negative or really large.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer_unittest.cc:492: // FFmpeg does not set timestamps
when demuxing wave files.  Ensure that the
nit: Fix the comment since it talks about wave, but this test uses ogv.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer_unittest.cc:506: // similar to the positive and
zero start time tests.
Shouldn't we resolve this before landing this change? What will happen in the
downstream code now if negative timestamp can come out of the demuxer?

[DaleCurtis, 2014-06-11 16:59:07]

Just comments.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:264: if (!buffer->end_of_stream() &&
!discard_helper_->initialized() &&
On 2014/06/10 23:20:38, acolwell wrote:
> with your changes to the demuxer, can we DCHECK(buffer->timestamp() >=
> base::TimeDelta() now just to make sure negative values can't sneak in
anymore.

Should be okay. I'll have to test though.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:251: buffer->set_timestamp(stream_timestamp -
demuxer_->start_time());
On 2014/06/10 23:20:38, acolwell wrote:
> Does this hold up with chained Ogg? Does FFmpeg guarantee that timestamps will
> always increase? I'm just wondering if it protects us from having to worry
about
> timestamps later in the stream that end up being < start_time()

Depends on how short the first link in the chain is I guess. I've never seen the
chained ogg go backwards by more than few seconds.  I suspect it's possible
today and my patch doesn't do anything to fix it.  It's possible if we add the
DCHECK(timestamp > 0) to the decoder we'll now fail hard instead of soft there
though.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:747: start_time_ = base::TimeDelta();
On 2014/06/10 23:20:38, acolwell wrote:
> If we don't know at this point, shouldn't we just try to defer this decision
> until we are able to get the first packet from all of the streams? If we don't
> it seems like we could accidentally trigger bad behavior if the first packets
> happen to be negative or really large.

Maybe.  Couldn't a seek happen before the first frame is read?  I don't think
it's worth adding this functionality.  What I've done here will fall back to
what we do today.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer_unittest.cc:506: // similar to the positive and
zero start time tests.
On 2014/06/10 23:20:38, acolwell wrote:
> Shouldn't we resolve this before landing this change? What will happen in the
> downstream code now if negative timestamp can come out of the demuxer?

http://git.videolan.org/?p=ffmpeg.git;a=blobdiff;f=libavformat/oggparsevorbis...

I'll try it out after Google Serve and see how it goes.

[acolwell GONE FROM CHROMIUM, 2014-06-11 17:40:24]

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:251: buffer->set_timestamp(stream_timestamp -
demuxer_->start_time());
On 2014/06/11 16:59:07, DaleCurtis wrote:
> On 2014/06/10 23:20:38, acolwell wrote:
> > Does this hold up with chained Ogg? Does FFmpeg guarantee that timestamps
will
> > always increase? I'm just wondering if it protects us from having to worry
> about
> > timestamps later in the stream that end up being < start_time()
> 
> Depends on how short the first link in the chain is I guess. I've never seen
the
> chained ogg go backwards by more than few seconds.  I suspect it's possible
> today and my patch doesn't do anything to fix it.  It's possible if we add the
> DCHECK(timestamp > 0) to the decoder we'll now fail hard instead of soft there
> though.

I guess my concern is that if FFmpeg allows timestamps to go backwards, how can
we reliably seek? If you use something like oggz-chop to split an ogg file into
2 pieces(A & B) and then concatenate them so they are in B,A order, what does
FFmpeg do? 

I realize that this may be broken in the existing code as well. I'm just trying
to understand what will happen.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:747: start_time_ = base::TimeDelta();
On 2014/06/11 16:59:07, DaleCurtis wrote:
> On 2014/06/10 23:20:38, acolwell wrote:
> > If we don't know at this point, shouldn't we just try to defer this decision
> > until we are able to get the first packet from all of the streams? If we
don't
> > it seems like we could accidentally trigger bad behavior if the first
packets
> > happen to be negative or really large.
> 
> Maybe.  Couldn't a seek happen before the first frame is read?  I don't think
> it's worth adding this functionality.  What I've done here will fall back to
> what we do today.
A seek can't happen until we transition to HAVE_METADATA and that can't happen
until the demuxer calls its init callback and all the renderers are initialized.
It should be pretty straightforward to delay calling the init callback until we
have received the first buffer for each stream. Either that or we should block
anything with a timestamp < start_time_ from being sent downstream so that other
components don't have to deal with it.

[DaleCurtis, 2014-06-12 00:21:56]

I'll roll the DEPS for FFmpeg fixes within this CL once I've confirmed the mp3
decoder fix.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_aud...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_aud...
media/filters/ffmpeg_audio_decoder.cc:264: if (!buffer->end_of_stream() &&
!discard_helper_->initialized() &&
On 2014/06/11 16:59:07, DaleCurtis wrote:
> On 2014/06/10 23:20:38, acolwell wrote:
> > with your changes to the demuxer, can we DCHECK(buffer->timestamp() >=
> > base::TimeDelta() now just to make sure negative values can't sneak in
> anymore.
> 
> Should be okay. I'll have to test though.

I've added the DCHECK to the demuxer.  However, it exposed some issues with how
FFmpeg is setting start_time during MP3 demuxing.  It seems wrong, so I've
pinged upstream.  With a local patch things seem to pass nicely.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:251: buffer->set_timestamp(stream_timestamp -
demuxer_->start_time());
On 2014/06/11 17:40:24, acolwell wrote:
> On 2014/06/11 16:59:07, DaleCurtis wrote:
> > On 2014/06/10 23:20:38, acolwell wrote:
> > > Does this hold up with chained Ogg? Does FFmpeg guarantee that timestamps
> will
> > > always increase? I'm just wondering if it protects us from having to worry
> > about
> > > timestamps later in the stream that end up being < start_time()
> > 
> > Depends on how short the first link in the chain is I guess. I've never seen
> the
> > chained ogg go backwards by more than few seconds.  I suspect it's possible
> > today and my patch doesn't do anything to fix it.  It's possible if we add
the
> > DCHECK(timestamp > 0) to the decoder we'll now fail hard instead of soft
there
> > though.
> 
> I guess my concern is that if FFmpeg allows timestamps to go backwards, how
can
> we reliably seek? If you use something like oggz-chop to split an ogg file
into
> 2 pieces(A & B) and then concatenate them so they are in B,A order, what does
> FFmpeg do? 
> 
> I realize that this may be broken in the existing code as well. I'm just
trying
> to understand what will happen.

I chopped the file as you described it seems timestamps are not rewritten, you
get the zero timestamp for the first part at the end of the second part which
generates a non-monotonic timestamp warning with a huge number.  FFmpeg then
returns a packet without a timestamp which errors out playback.  The behavior is
unchanged by my patch.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:442: start_time_(kInfiniteDuration()),
On 2014/06/10 23:20:38, acolwell wrote:
> This seems a little out of place since we typically use kNoTimestamp() to
signal
> "not set". It seems like your use of kInfiniteDurat() should be constrained to
> OnFindStreamInfoDone() so it is closer to the code that actually uses it for
> special purposes.

Done.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:477: const int flags = AVSEEK_FLAG_BACKWARD;
On 2014/06/10 23:20:38, acolwell wrote:
> Why not simply inline this below?

Done.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:720:
streams_[i]->enable_negative_timestamp_discard();
On 2014/06/10 23:20:38, acolwell wrote:
> nit: Just pass this via the constructor 2 lines up since this value isn't
> expected to change during the life of the stream.

Done.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:747: start_time_ = base::TimeDelta();
On 2014/06/11 17:40:24, acolwell wrote:
> On 2014/06/11 16:59:07, DaleCurtis wrote:
> > On 2014/06/10 23:20:38, acolwell wrote:
> > > If we don't know at this point, shouldn't we just try to defer this
decision
> > > until we are able to get the first packet from all of the streams? If we
> don't
> > > it seems like we could accidentally trigger bad behavior if the first
> packets
> > > happen to be negative or really large.
> > 
> > Maybe.  Couldn't a seek happen before the first frame is read?  I don't
think
> > it's worth adding this functionality.  What I've done here will fall back to
> > what we do today.
> A seek can't happen until we transition to HAVE_METADATA and that can't happen
> until the demuxer calls its init callback and all the renderers are
initialized.
> It should be pretty straightforward to delay calling the init callback until
we
> have received the first buffer for each stream. Either that or we should block
> anything with a timestamp < start_time_ from being sent downstream so that
other
> components don't have to deal with it.
> 

This is not easy to do since the preroll decision is handled by pipeline the 
demuxer and renderers have completed initialization.  We could trigger Preroll
until we've received all stream start times, but that may delay the rest of
pipeline initialization since those would be network loads.

I've instead just added a DCHECK() for this to the demuxer.

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/325503003/diff/20001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer_unittest.cc:492: // FFmpeg does not set timestamps
when demuxing wave files.  Ensure that the
On 2014/06/10 23:20:38, acolwell wrote:
> nit: Fix the comment since it talks about wave, but this test uses ogv.

Done.

[acolwell GONE FROM CHROMIUM, 2014-06-13 18:12:13]

lgtm % comments

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:284: last_packet_timestamp_ =
buffer->timestamp();
Can't this cause gaps in the buffered ranges reported if a timestamp is set to
kNoTimestamp()? File a bug and add a TODO?

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:734: 
Can't we move the start_time check/update here and avoid duplicate code above? I
think this would also fix the computation if a text track cue happens to have
the earliest timestamp.

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer_unittest.cc:111:
av_dict_set(&demuxer_->glue_->format_context()->metadata,
This seems brittle. Why not just add an xxForTesting() method and set the value
directly? The demuxer could just avoid updating timeline_offset_ if it is
already set.

[DaleCurtis, 2014-06-13 20:40:02]

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:284: last_packet_timestamp_ =
buffer->timestamp();
On 2014/06/13 18:12:13, acolwell wrote:
> Can't this cause gaps in the buffered ranges reported if a timestamp is set to
> kNoTimestamp()? File a bug and add a TODO?

We should probably just stop allowing kNoTimestamp() here since the audio
decoder is going to issue a decode error when it happens.  I'll mark it with a
TODO.

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:734: 
On 2014/06/13 18:12:12, acolwell wrote:
> Can't we move the start_time check/update here and avoid duplicate code above?
I
> think this would also fix the computation if a text track cue happens to have
> the earliest timestamp.

Done.

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/325503003/diff/40001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer_unittest.cc:111:
av_dict_set(&demuxer_->glue_->format_context()->metadata,
On 2014/06/13 18:12:13, acolwell wrote:
> This seems brittle. Why not just add an xxForTesting() method and set the
value
> directly? The demuxer could just avoid updating timeline_offset_ if it is
> already set.

The timeline_offset_ value isn't set until OnFindStreamInfoDone is run, which is
PostTask'd during Initialize().  The code which adjusts the timeline_offset_ by
start_time_ is done there, so we need to set this value before that call
happens.  A SetTimelineOffsetForTesting() function wouldn't accomplish that.

At best I could move this code into FFmpegDemuxer, but I don't think that's any
less fragile.  Using av_dict_set() instead of overriding timeline_offset_ also
gives us better testing of the complete timeline_offset_ pipeline.

[DaleCurtis, 2014-06-13 20:40:40]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-06-13 20:43:57]

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

[commit-bot: I haz the power, 2014-06-13 23:59:50]

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

[commit-bot: I haz the power, 2014-06-13 23:59:51]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/16838)

[DaleCurtis, 2014-06-15 23:41:15]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-06-15 23:41:45]

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

[commit-bot: I haz the power, 2014-06-16 02:58:49]

Change committed as 277348

[tkent, 2014-06-16 06:06:06]

A revert of this CL has been created in
https://codereview.chromium.org/334163002/ by tkent@chromium.org.

The reason for reverting is: Broke the following layout tests:
- media/track/track-cue-rendering-horizontal.html
- media/track/track-cue-rendering-vertical.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
.

[DaleCurtis, 2014-06-17 00:34:36]

acolwell: PTAL.  The following changes were necessary:
- timeline_offset_ is now set directly for testing.  Otherwise race conditions
exist since find_stream_info is executing on a background thread.
- seeking is now done on the first video stream which contains the seek point,
or if none exists, the stream with the lowest starting timestamp containing the
seek point.  Setting -1 as the stream index did not work :(
- start timestamps are now extracted from the ffmpeg packet buffers for better
accuracy.
- ffmpeg deps roll now includes some GN changes from ajwong@.

The new code passes all layout tests and unit tests.

[DaleCurtis, 2014-06-17 00:47:01]

Per acolwell's request, I've moved the changes to a new issue:
https://codereview.chromium.org/335273002/