Fix incorrect opus seek preroll and flaky pre-skip removal.

Fix incorrect opus seek preroll and flaky pre-skip removal.

Opus seek preroll was being treated as decoder delay when it really
just means that for seeking to a time T, we need to decode packets
starting from time "T - seek_preroll" and throw them away. Instead
we were discarding time [T, T+seek_preroll] and shifting the
timestamps backwards to accommodate the deletion.

For pre-skip, we would assume the first packet seen by the decoder
was the timestamp for which pre-skip should be applied.  However,
this is incorrect and pre-skip should always be dropped because
timestamp zero represents the beginning of the pre-skip, which
causes all future timestamps to be displaced by pre-skip duration.

This patch fixes pre-skip and seek preroll for src= and pre-skip only
for MSE. ChunkDemuxer needs to be modified to support preroll. src=
is fixed by expanding the hacks in place for negative ogg timestamps
to handle negative timestamps from ffmpeg in a more general manner.

This patch also adds test cases for the seeking cases (one of which MSE
can pass because the test clip has a codec delay > seek preroll).
Since these tests would otherwise take some time to run, this also
adds support for clockless hashed pipeline integration tests.

BUG=509894
TEST=OpusAudioDecoder and OPUS PipelineIntegration tests pass
w/o expectations modification.  New trimming pipeline tests.
New FFmpegDemuxerTest to verify audio/video timestamp sync.

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

[DaleCurtis, 2015-07-28 23:28:40]

dalecurtis@chromium.org changed reviewers:
+ chcunningham@chromium.org, wolenetz@chromium.org

[DaleCurtis, 2015-07-28 23:31:21]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2015-07-28 23:45:52]

Actually, after posting this I realized there was a four line hack which would
keep MSE working correctly in the cases it previously did while fixing the
incorrect ones. I think it's worth doing correctly given the percentage of MSE
views on opus.

[wolenetz, 2015-07-29 00:17:19]

The discussion in the bug (comments #18,20-21) is confusing me w.r.t. this CL:
is FFmpeg subracting Opus pre-skip from all Opus packet pts, or just the first
packet's pts? And should we add it back for just the first, or all?

[DaleCurtis, 2015-07-29 01:27:15]

Hmm, after investigation all that code can be deleted, it was not correct. 
FFmpeg is doing the right thing; is MSE adjusting the timestamps based on codec
delay somewhere? It seems to be doing the right thing too.

[wolenetz, 2015-07-29 21:57:27]

I like the new tests :)
How did the local-only attempt to hack seek_preroll into
SourceBufferStream/ChunkDemuxer work out (did the MSE seek hash  become
correct?)

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
File media/audio/clockless_audio_sink.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
media/audio/clockless_audio_sink.cc:43: return audio_hash_->ToString();
nit: Is this for test only (I think so)? If not, then this might hit NPE on null
audio_hash_.get() if called without |hashing| true in ctor. Include "ForTesting"
in the name?

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
media/audio/clockless_audio_sink.cc:53: audio_hash_->Update(audio_bus_.get(),
frames_received);
hmm. Could frames_received indeed be negative (see next line which implies
this)? If so, AudioHash::Update() casts int to uint32 and negative would be a
problem.

https://codereview.chromium.org/1260193005/diff/70001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:257: // Because FFmpeg rolls codec delay and
skip samples into one we can only
Is there any AV_PKT_DATA_SKIP_SAMPLES (front skip) ever populated for
AV_CODEC_ID_OPUS? Perhaps add a DCHECK to help catch regression?

https://codereview.chromium.org/1260193005/diff/70001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:287: base::TimeDelta()));
Could there be a single packet that is both the start and end of an Opus stream?
If so, then if there is any end sample skipping, this code doesn't work properly
IIUC.

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:475: // is the first packet and the
timestamp is zero, strip codec delay.
Hmm. In MSE, the fix could be simpler: on config change, a new Opus decoder is
initialized. The Opus decoder output following that config change will need the
first config->codec_delay samples discarded no matter what IIUC.

For example, append init segment containing Opus codec delay of 312
frames@48Khz=6.5ms, and then time [10s,20s) of an Opus stream that begins at
time 0. Seek to time 10s. Ignoring the lack of seek preroll support for Opus in
MSE, the first 6.5ms of decode output from the encoded media @ 10s would need to
be discarded.  This is because the PTS of 10s in webm is for a block containing
codec_delay frames (from previous packet spilling over transitively), followed
by the frames that are really meant to be presented beginning at 10s.

So, instead of messing with start_timestamp_, couldn't we just apply Opus codec
delay discard padding to the first buffer passed to a new opus decoder instance?
Would this also work for ffmpeg demuxed buffers?

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
File media/test/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
media/test/pipeline_integration_test_base.cc:271: // Don't allow the audio
renderer to resample buffers if hashing is enabled.
Is AudioHash too sensitive to resampling, despite the comments in its header
file?

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
media/test/pipeline_integration_test_base.cc:290:
audio_sink_->StartAudioHashForTesting();
hmm. Previously we only hashed if |hashing_enabled_|, now we always do it? Seems
wrong.

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
File media/test/pipeline_integration_test_base.h (right):

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
media/test/pipeline_integration_test_base.h:75: enum TestType { kHashed = 1,
kClockless = 2};
nit: include "Flags" in the name to indicate that these are not exclusive of
each other, but instead part of a mask?

[DaleCurtis, 2015-07-30 01:28:14]

I've confirmed that at least for a seek within the pre-skip section adding the
80ms of preroll does fix MSE. The seek to the midpoint does not seem correct
currently, but that could be because my hack is bad or because there's a real
issue with timestamp handling in MSE as it related to pre-skip -- I'll
investigate more tomorrow.

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
File media/audio/clockless_audio_sink.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
media/audio/clockless_audio_sink.cc:43: return audio_hash_->ToString();
On 2015/07/29 21:57:27, wolenetz wrote:
> nit: Is this for test only (I think so)? If not, then this might hit NPE on
null
> audio_hash_.get() if called without |hashing| true in ctor. Include
"ForTesting"
> in the name?

It is only for testing, but I can't add ForTesting() because presubmit isn't
smart enough to figure out that one ForTesting method has called another
ForTesting() method. I added a DCHECK().

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
media/audio/clockless_audio_sink.cc:53: audio_hash_->Update(audio_bus_.get(),
frames_received);
On 2015/07/29 21:57:27, wolenetz wrote:
> hmm. Could frames_received indeed be negative (see next line which implies
> this)? If so, AudioHash::Update() casts int to uint32 and negative would be a
> problem.

No, it's always >= 0. I've added a DCHECK and fixed the misleading conditional.

https://codereview.chromium.org/1260193005/diff/70001/media/filters/ffmpeg_de...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:257: // Because FFmpeg rolls codec delay and
skip samples into one we can only
On 2015/07/29 21:57:27, wolenetz wrote:
> Is there any AV_PKT_DATA_SKIP_SAMPLES (front skip) ever populated for
> AV_CODEC_ID_OPUS? Perhaps add a DCHECK to help catch regression?

Added a DCHECK and reflowed the conditional for support of end-trimming, though
I'm not sure that's actually possible.

https://codereview.chromium.org/1260193005/diff/70001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:287: base::TimeDelta()));
On 2015/07/29 21:57:27, wolenetz wrote:
> Could there be a single packet that is both the start and end of an Opus
stream?
> If so, then if there is any end sample skipping, this code doesn't work
properly
> IIUC.

Done.

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:475: // is the first packet and the
timestamp is zero, strip codec delay.
On 2015/07/29 21:57:27, wolenetz wrote:
> Hmm. In MSE, the fix could be simpler: on config change, a new Opus decoder is
> initialized. The Opus decoder output following that config change will need
the
> first config->codec_delay samples discarded no matter what IIUC.

That understanding is not correct, only the first buffer has data which needs
trimming. Only timestamps should be shifted by the pre-skip value, the actual
decoded audio data should not be trimmed unless we're dealing with the
beginning-of-stream packet(s).

The timestamp shifting is done by ffmpeg for opus and webm  (see the code I
removed in FFmpegDemuxer), I'm not sure where or if it needs to happen for webm
in mse, though the file plays correctly from start, and a seek within the
pre-skip appears to work correctly -- however the midpoint seek does not match
the results returned by ffmpeg for ogg/webm.

> 
> For example, append init segment containing Opus codec delay of 312
> mailto:frames@48Khz=6.5ms, and then time [10s,20s) of an Opus stream that
begins at
> time 0. Seek to time 10s. Ignoring the lack of seek preroll support for Opus
in
> MSE, the first 6.5ms of decode output from the encoded media @ 10s would need
to
> be discarded.  This is because the PTS of 10s in webm is for a block
containing
> codec_delay frames (from previous packet spilling over transitively), followed
> by the frames that are really meant to be presented beginning at 10s.
> 
> So, instead of messing with start_timestamp_, couldn't we just apply Opus
codec
> delay discard padding to the first buffer passed to a new opus decoder
instance?
> Would this also work for ffmpeg demuxed buffers?

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
File media/test/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
media/test/pipeline_integration_test_base.cc:271: // Don't allow the audio
renderer to resample buffers if hashing is enabled.
On 2015/07/29 21:57:27, wolenetz wrote:
> Is AudioHash too sensitive to resampling, despite the comments in its header
> file?

Possibly, the tests files are also mono, so they were being upmixed to stereo
which combined with resampling was definitely too much change.

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
media/test/pipeline_integration_test_base.cc:290:
audio_sink_->StartAudioHashForTesting();
On 2015/07/29 21:57:27, wolenetz wrote:
> hmm. Previously we only hashed if |hashing_enabled_|, now we always do it?
Seems
> wrong.

Done.

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
File media/test/pipeline_integration_test_base.h (right):

https://codereview.chromium.org/1260193005/diff/70001/media/test/pipeline_int...
media/test/pipeline_integration_test_base.h:75: enum TestType { kHashed = 1,
kClockless = 2};
On 2015/07/29 21:57:27, wolenetz wrote:
> nit: include "Flags" in the name to indicate that these are not exclusive of
> each other, but instead part of a mask?

Done.

[DaleCurtis, 2015-07-30 19:04:58]

Okay disregard my last comment about further issues with MSE, the only issue is
the seek preroll -- the problem is that I was using duration / 2 as the seek
point and ffmpeg and mse differ on the duration of the files.

To verify that seek preroll is the only issue, I just called
ChunkDemuxer::StartWaitingForSeek(time - 80ms) and then commented out the
SeekAllSources in ChunkDemuxer::Seek().

This should be all good to go now.

[DaleCurtis, 2015-07-30 23:24:44]

Turns out new code wasn't necessary in ffmpeg demxuer, we already had code in
place for dealing with negative timestamps for ffmpeg, it was just restricted to
ogg files, removing the ogg restriction and improving sample accuracy when delay
is present is enough to resolve trimming issues reliably.

I've added a new test and video file to verify that a/v sync is correct with
src= playbacks now.

[DaleCurtis, 2015-07-31 02:32:03]

Patchset #9 (id:170001) has been deleted

[DaleCurtis, 2015-07-31 02:41:00]

Need to find another seek test clip for mse/src= since the one I added has a
codec delay > seek preroll so thus both MSE and src= pass the test. 
Unfortunately bear-opus.webm seeks differently in mse vs ffmpeg (ffmpeg seeks to
0 always (which seems like a bug in and of itself) while mse is correctly
seeking to ~1s).  I'll investigate more tomorrow -- but otherwise this should
all be correct now.  Sorry for all the back and forth!

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:475: // is the first packet and the
timestamp is zero, strip codec delay.
On 2015/07/30 01:28:14, DaleCurtis wrote:
> On 2015/07/29 21:57:27, wolenetz wrote:
> > Hmm. In MSE, the fix could be simpler: on config change, a new Opus decoder
is
> > initialized. The Opus decoder output following that config change will need
> the
> > first config->codec_delay samples discarded no matter what IIUC.
> 
> That understanding is not correct, only the first buffer has data which needs
> trimming. Only timestamps should be shifted by the pre-skip value, the actual
> decoded audio data should not be trimmed unless we're dealing with the
> beginning-of-stream packet(s).
> 
> The timestamp shifting is done by ffmpeg for opus and webm  (see the code I
> removed in FFmpegDemuxer), I'm not sure where or if it needs to happen for
webm
> in mse, though the file plays correctly from start, and a seek within the
> pre-skip appears to work correctly -- however the midpoint seek does not match
> the results returned by ffmpeg for ogg/webm.
> 
> > 
> > For example, append init segment containing Opus codec delay of 312
> > mailto:frames@48Khz=6.5ms, and then time [10s,20s) of an Opus stream that
> begins at
> > time 0. Seek to time 10s. Ignoring the lack of seek preroll support for Opus
> in
> > MSE, the first 6.5ms of decode output from the encoded media @ 10s would
need
> to
> > be discarded.  This is because the PTS of 10s in webm is for a block
> containing
> > codec_delay frames (from previous packet spilling over transitively),
followed
> > by the frames that are really meant to be presented beginning at 10s.
> > 
> > So, instead of messing with start_timestamp_, couldn't we just apply Opus
> codec
> > delay discard padding to the first buffer passed to a new opus decoder
> instance?
> > Would this also work for ffmpeg demuxed buffers?
> 

Turns out my understanding was totally wrong. It is correct that we always want
to drop pre-skip worth of samples since timestamp zero represents the beginning
of the pre-skip. The hash tests I ran previously were in fact forcing ffmpeg to
do the wrong thing. Sorry for all the confusion on my part.

[chcunningham, 2015-07-31 19:48:45]

Mostly I'm just asking questions for my own understanding :)

https://codereview.chromium.org/1260193005/diff/190001/media/base/audio_disca...
File media/base/audio_discard_helper.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/base/audio_disca...
media/base/audio_discard_helper.cc:108: // consumes the entire buffer.
What about the case where current_discard_padding is swapped into
delayed_discard_padding_? Is it right to still use the padding in a delayed
fashion when we would otherwise drop the discard in the not-delayed case?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_a...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_a...
media/filters/ffmpeg_audio_decoder.cc:372: discard_helper_.reset(new
AudioDiscardHelper(config_.samples_per_second(),
Move this into ResetTimestampState like you did for opus?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (left):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:323: // Only discard part of the frame if it
overlaps zero.
Did you mean to kill the comment?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:314: // negative timestamps for post-decode
discard.
just to check my understanding....

1. we see negative timestamps here because ffmpeg is moving the timeline back
for preskip, so the skip porition is negative

2. we don't mark it for post-decode discard because we will handle that via
opus_decoder's discard helper, and marking it for discard_padding here is
redundant and error prone?

If I have the above right, are we confident in making this change for non-opus
codecs? Might there be other decoders which were relying on us to set discard
padding here for their codec delay?

You might add more to this comment about why the code path is different for
codec->delay.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:328: -stream_timestamp,
buffer->discard_padding().second));
I can see why this is more correct, but I'm curious: did you hit this at some
point (what were the circumstances), or is it just drive-by fixing?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:636: : time < start_time_ ? start_time_ : time;
I don't follow why we only add start_time_ in the case where start_time < 0?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:644: if (config.codec() == kCodecOpus)
How does this work for other codecs that need pre-roll?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:997:
streams_[i]->enable_negative_timestamp_fixups();
Why do we do this for all streams and not just the audio stream?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/opus_aud...
File media/filters/opus_audio_decoder.cc (left):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/opus_aud...
media/filters/opus_audio_decoder.cc:323: if (start_input_timestamp_ ==
kNoTimestamp())
Am I right that what you removed here is equivalent behavior to what we have
now? That is, we used to set start_timestamp with the TS from the first buffer
and configure the discard_helper with preskip. Now we just we configure the
discard helper as soon as the configuration arrives. The effect in both cases is
that we discard the first so many buffers we see (and adjust timestamps) until
preskip is satisfied. If I'm right, the new way is definitely cleaner, but it
makes me wonder if I've missed something. The change description suggests to me
that the original behavior was different/wrong.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:114: // The above hash, plus an
additional playthrough starting from T=6.36s.
Which "above" do you mean here? Could make this more clear if you ref the
variable name of the "above"

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:502: chunk_demuxer_->Abort(
Noob questions: I expected to see

chunk_demuxer->StartWaitingForSeek(...)
chunk_demuxer->Seek(...)

I don't follow why we abort, nor why we don't actually call seek?

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:885: // TODO(dalecurtis): Add a test
file which ffmpeg and chunkdemuxer will both
Don't forget to file a bug if you don't handle this here.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1021: TEST_F(PipelineIntegrationTest,
DISABLED_MediaSource_Opus_Seeking_WebM) {
Still flaky? Perhaps the hashes are different now?

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test_base.cc:272: if (!hashing_enabled_) {
Noob question: Can you tell me why we resample in non-hashing cases?

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test_base.h (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test_base.h:74: // hashes).  May be combined
using the bitwise or operator.
Might mention that it must be powers of 2.

[DaleCurtis, 2015-08-03 22:49:53]

https://codereview.chromium.org/1260193005/diff/190001/media/base/audio_disca...
File media/base/audio_discard_helper.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/base/audio_disca...
media/base/audio_discard_helper.cc:108: // consumes the entire buffer.
On 2015/07/31 19:48:44, chcunningham wrote:
> What about the case where current_discard_padding is swapped into
> delayed_discard_padding_? Is it right to still use the padding in a delayed
> fashion when we would otherwise drop the discard in the not-delayed case?

That would still be correct, as the initial discard only applies to what's
already been decoded; whereas with delayed discard we haven't decoded the next
buffer just yet.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_a...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_a...
media/filters/ffmpeg_audio_decoder.cc:372: discard_helper_.reset(new
AudioDiscardHelper(config_.samples_per_second(),
On 2015/07/31 19:48:44, chcunningham wrote:
> Move this into ResetTimestampState like you did for opus?

Done.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (left):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:323: // Only discard part of the frame if it
overlaps zero.
On 2015/07/31 19:48:44, chcunningham wrote:
> Did you mean to kill the comment?

Nope, reverted.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:314: // negative timestamps for post-decode
discard.
On 2015/07/31 19:48:44, chcunningham wrote:
> just to check my understanding....
> 
> 1. we see negative timestamps here because ffmpeg is moving the timeline back
> for preskip, so the skip porition is negative
> 
> 2. we don't mark it for post-decode discard because we will handle that via
> opus_decoder's discard helper, and marking it for discard_padding here is
> redundant and error prone?
> 
> If I have the above right, are we confident in making this change for non-opus
> codecs? Might there be other decoders which were relying on us to set discard
> padding here for their codec delay?
> 
> You might add more to this comment about why the code path is different for
> codec->delay.

Yes that is correct. We're confident because this code doesn't change anything
for codecs which don't specify codec delay, for those that do I've added a
DCHECK to verify we trim enough.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:328: -stream_timestamp,
buffer->discard_padding().second));
On 2015/07/31 19:48:44, chcunningham wrote:
> I can see why this is more correct, but I'm curious: did you hit this at some
> point (what were the circumstances), or is it just drive-by fixing?

Just fixing this as wolenetz@ complained about it earlier. I haven't ever hit
this and am not sure it's possible, but the discard helper will allow it.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:636: : time < start_time_ ? start_time_ : time;
On 2015/07/31 19:48:44, chcunningham wrote:
> I don't follow why we only add start_time_ in the case where start_time < 0?

See l.305, the gist is that we don't rebase positive start times.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:644: if (config.codec() == kCodecOpus)
On 2015/07/31 19:48:44, chcunningham wrote:
> How does this work for other codecs that need pre-roll?

It doesn't :) No other codecs specify this information in terms of an actual
seek_preroll(). For cases like mp3 and aac which would benefit from an explicit
seek_preroll() we aren't doing anything special -- though in many cases ffmpeg
seeks early enough that the decoder is effectively satisfied.

Thus far this hasn't been a noticeable problem, it may not be for opus either...
I expect at worst you might here a pop or click after the seek. I don't know if
skipping the opus preroll may have harsher effects or is more of a quality of
implementation thing.

We could probably start setting a seek_preroll() of 529 frames for MP3 and 1152
for AAC if we want.  I can take a look at that in a followup patch.  It
shouldn't be too hard for src=, not sure how you'd want to handle it for MSE,
but it'd be similar to whatever you want to do for OPUS.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:997:
streams_[i]->enable_negative_timestamp_fixups();
On 2015/07/31 19:48:44, chcunningham wrote:
> Why do we do this for all streams and not just the audio stream? 

Because we need to tell the video stream that it shouldn't rebase the timestamps
against the negative start time.  I've updated the function comments for
clarity.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/opus_aud...
File media/filters/opus_audio_decoder.cc (left):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/opus_aud...
media/filters/opus_audio_decoder.cc:323: if (start_input_timestamp_ ==
kNoTimestamp())
On 2015/07/31 19:48:44, chcunningham wrote:
> Am I right that what you removed here is equivalent behavior to what we have
> now? That is, we used to set start_timestamp with the TS from the first buffer
> and configure the discard_helper with preskip. Now we just we configure the
> discard helper as soon as the configuration arrives. The effect in both cases
is
> that we discard the first so many buffers we see (and adjust timestamps) until
> preskip is satisfied. If I'm right, the new way is definitely cleaner, but it
> makes me wonder if I've missed something. The change description suggests to
me
> that the original behavior was different/wrong.

Kind of, in the first playback case the behavior is similar. As noted in the
description, however subsequent playbacks or seeks would do the wrong thing and
be initialized with the seek preroll instead of the codec delay though.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:114: // The above hash, plus an
additional playthrough starting from T=6.36s.
On 2015/07/31 19:48:45, chcunningham wrote:
> Which "above" do you mean here? Could make this more clear if you ref the
> variable name of the "above"

The one immediately above :) I dislike putting variable names in comments when
possible because they may change and it feels a bit redundant. Unless you
strongly prefer, I'm going to leave it as is.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:502: chunk_demuxer_->Abort(
On 2015/07/31 19:48:45, chcunningham wrote:
> Noob questions: I expected to see
> 
> chunk_demuxer->StartWaitingForSeek(...)
> chunk_demuxer->Seek(...)
> 
> I don't follow why we abort, nor why we don't actually call seek?

I don't know either, I just copied the Seek(a,b,c) method on l.486. If you have
suggestions which would work better let me know.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:885: // TODO(dalecurtis): Add a test
file which ffmpeg and chunkdemuxer will both
On 2015/07/31 19:48:45, chcunningham wrote:
> Don't forget to file a bug if you don't handle this here.

Attached the bug to this.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:1021: TEST_F(PipelineIntegrationTest,
DISABLED_MediaSource_Opus_Seeking_WebM) {
On 2015/07/31 19:48:44, chcunningham wrote:
> Still flaky? Perhaps the hashes are different now?

I haven't checked, will need to build a valgrind build and see. That's something
we can solve outside of this change though.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test_base.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test_base.cc:272: if (!hashing_enabled_) {
On 2015/07/31 19:48:45, chcunningham wrote:
> Noob question: Can you tell me why we resample in non-hashing cases? 

Test coverage for the AudioBufferConverter.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test_base.h (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test_base.h:74: // hashes).  May be combined
using the bitwise or operator.
On 2015/07/31 19:48:45, chcunningham wrote:
> Might mention that it must be powers of 2.

Done.

[chcunningham, 2015-08-04 20:38:09]

Did you learn any more about the state of MSE pre-roll? Is it still just
appearing to do the right thing (-80ms seeks) but we don't know where?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:636: : time < start_time_ ? start_time_ : time;
On 2015/08/03 22:49:53, DaleCurtis wrote:
> On 2015/07/31 19:48:44, chcunningham wrote:
> > I don't follow why we only add start_time_ in the case where start_time < 0?
> 
> See l.305, the gist is that we don't rebase positive start times.

I see. This is a fragile contract between two methods several hundred lines
apart. Might be cool to have a module like FFmpegTimestampGlue that takes in a
configuration and has APIs like 

TimeDelta FfmpegSeekTimeFromPipelineTime(const DecoderBuffer*) - 
void FixPipelineTimestamp(DecoderBuffer*) - use in EnqueuePacket

eh... I don't know, just looking for ways to tame the spaghetti monster.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:644: if (config.codec() == kCodecOpus)
On 2015/08/03 22:49:53, DaleCurtis wrote:
> On 2015/07/31 19:48:44, chcunningham wrote:
> > How does this work for other codecs that need pre-roll?
> 
> It doesn't :) No other codecs specify this information in terms of an actual
> seek_preroll(). For cases like mp3 and aac which would benefit from an
explicit
> seek_preroll() we aren't doing anything special -- though in many cases ffmpeg
> seeks early enough that the decoder is effectively satisfied.
> 
> Thus far this hasn't been a noticeable problem, it may not be for opus
either...
> I expect at worst you might here a pop or click after the seek. I don't know
if
> skipping the opus preroll may have harsher effects or is more of a quality of
> implementation thing.
> 
> We could probably start setting a seek_preroll() of 529 frames for MP3 and
1152
> for AAC if we want.  I can take a look at that in a followup patch.  It
> shouldn't be too hard for src=, not sure how you'd want to handle it for MSE,
> but it'd be similar to whatever you want to do for OPUS.

Is this something we can extract into a utility used by both src= and mse? 

TimeDelta media::GetCodecSeekTime(TimeDelta, Codec) {
swtich(codec)
... various preroll adjustments
}

maintain it all in one place?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:997:
streams_[i]->enable_negative_timestamp_fixups();
On 2015/08/03 22:49:53, DaleCurtis wrote:
> On 2015/07/31 19:48:44, chcunningham wrote:
> > Why do we do this for all streams and not just the audio stream? 
> 
> Because we need to tell the video stream that it shouldn't rebase the
timestamps
> against the negative start time.  I've updated the function comments for
> clarity.

I see. I feel like this should really be called 
enable_negative_*audio*_timestamp_fixups()

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:502: chunk_demuxer_->Abort(
On 2015/08/03 22:49:53, DaleCurtis wrote:
> On 2015/07/31 19:48:45, chcunningham wrote:
> > Noob questions: I expected to see
> > 
> > chunk_demuxer->StartWaitingForSeek(...)
> > chunk_demuxer->Seek(...)
> > 
> > I don't follow why we abort, nor why we don't actually call seek?
> 
> I don't know either, I just copied the Seek(a,b,c) method on l.486. If you
have
> suggestions which would work better let me know.

wolenetz@, do you know more on this?

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:238: // If a packet is returned by FFmpeg's
av_parser_parse2() the packet will
random: do they mean parse2 here? or should this comment say
av_packet_get_side_data?

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:297: // If this is an OGG file with negative
timestamps don't rebase any other
This comment is still specific to OGG while the condition is made more general

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:334: // mark the packet for post decode discard.
 Since timestamps may be in
One thing thats still missing for me in this comment is *why* we don't set
discard padding for codec delay. You cover that in the .h file though... maybe
ref it here?

Also, do we have a check the the decoders do actually handle this discard?
Perhaps we already have a check in a renderer somewhere that we don't have
negative timestamps?

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.h:83: // rebased to time zero, all other stream
types will use the muxed timestamp.
Let me check my understanding

if we had a webm file with vp9 and aac, any negative timestamps in vp9 and/or
aac would be rebased and not discard.

but if that same file is vp9 and ogg, only the ogg is rebased and negative
portions are discarded, while the vp9 is not rebased nor discarded ... and I
guess negative timestamps make it all the way to the renderer in such cases? 

I follow our special handling of ogg, but if the above is correct, I'm not sure
I completely understand why video rebasing is not done in the ogg case.... is it
just because there's too much noise to do it with any confidence? If I have this
right, we should def rename this method/var to indicate its special to audio,
and maybe add some comment (even more lol) as to why we don't do video in these
cases.

[wolenetz, 2015-08-04 21:32:38]

My apologies for slow review on this. I'll get to my next iteration on this
soon.

[DaleCurtis, 2015-08-04 21:38:34]

MSE preroll is not doing the right thing, but it luckily works if pre-skip >
preroll since enough data is fed in anyways. The tests I added pass because
pre-skip > preroll for the new test files.

I'll need to create a special test case that has pre-skip < preroll and for
which ffmpeg and mse seek the same in. I may not get to that in this CL.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:644: if (config.codec() == kCodecOpus)
On 2015/08/04 20:38:09, chcunningham wrote:
> On 2015/08/03 22:49:53, DaleCurtis wrote:
> > On 2015/07/31 19:48:44, chcunningham wrote:
> > > How does this work for other codecs that need pre-roll?
> > 
> > It doesn't :) No other codecs specify this information in terms of an actual
> > seek_preroll(). For cases like mp3 and aac which would benefit from an
> explicit
> > seek_preroll() we aren't doing anything special -- though in many cases
ffmpeg
> > seeks early enough that the decoder is effectively satisfied.
> > 
> > Thus far this hasn't been a noticeable problem, it may not be for opus
> either...
> > I expect at worst you might here a pop or click after the seek. I don't know
> if
> > skipping the opus preroll may have harsher effects or is more of a quality
of
> > implementation thing.
> > 
> > We could probably start setting a seek_preroll() of 529 frames for MP3 and
> 1152
> > for AAC if we want.  I can take a look at that in a followup patch.  It
> > shouldn't be too hard for src=, not sure how you'd want to handle it for
MSE,
> > but it'd be similar to whatever you want to do for OPUS.
> 
> Is this something we can extract into a utility used by both src= and mse? 
> 
> TimeDelta media::GetCodecSeekTime(TimeDelta, Codec) {
> swtich(codec)
> ... various preroll adjustments
> }
> 
> maintain it all in one place?

This is already encoded in the seek_preroll() and since it's just a subtraction,
I don't think it's worth abstracting further.

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:997:
streams_[i]->enable_negative_timestamp_fixups();
On 2015/08/04 20:38:09, chcunningham wrote:
> On 2015/08/03 22:49:53, DaleCurtis wrote:
> > On 2015/07/31 19:48:44, chcunningham wrote:
> > > Why do we do this for all streams and not just the audio stream? 
> > 
> > Because we need to tell the video stream that it shouldn't rebase the
> timestamps
> > against the negative start time.  I've updated the function comments for
> > clarity.
> 
> I see. I feel like this should really be called 
> enable_negative_*audio*_timestamp_fixups()

Well, it has different effects on audio and video, so that's misleading.

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:238: // If a packet is returned by FFmpeg's
av_parser_parse2() the packet will
On 2015/08/04 20:38:09, chcunningham wrote:
> random: do they mean parse2 here? or should this comment say
> av_packet_get_side_data?

I'm not sure if this is up to date, it used to refer to how av_read_packet()
worked internally.  In any case it's out of scope for this review.

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:297: // If this is an OGG file with negative
timestamps don't rebase any other
On 2015/08/04 20:38:09, chcunningham wrote:
> This comment is still specific to OGG while the condition is made more general

Done.

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:334: // mark the packet for post decode discard.
 Since timestamps may be in
On 2015/08/04 20:38:09, chcunningham wrote:
> One thing thats still missing for me in this comment is *why* we don't set
> discard padding for codec delay. You cover that in the .h file though... maybe
> ref it here?
> 
> Also, do we have a check the the decoders do actually handle this discard?
> Perhaps we already have a check in a renderer somewhere that we don't have
> negative timestamps?

Added reference. We don't have a check currently, I wouldn't want to add one in
this CL, maybe in a follow up one which could be reverted more easily.

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/1260193005/diff/210001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.h:83: // rebased to time zero, all other stream
types will use the muxed timestamp.
On 2015/08/04 20:38:09, chcunningham wrote:
> Let me check my understanding
> 
> if we had a webm file with vp9 and aac, any negative timestamps in vp9 and/or
> aac would be rebased and not discard.
> 
> but if that same file is vp9 and ogg, only the ogg is rebased and negative
> portions are discarded, while the vp9 is not rebased nor discarded ... and I
> guess negative timestamps make it all the way to the renderer in such cases? 
> 
> I follow our special handling of ogg, but if the above is correct, I'm not
sure
> I completely understand why video rebasing is not done in the ogg case.... is
it
> just because there's too much noise to do it with any confidence? If I have
this
> right, we should def rename this method/var to indicate its special to audio,
> and maybe add some comment (even more lol) as to why we don't do video in
these
> cases.

I assume you mean vp9 and vorbis since you can't put vp9 in ogg. It's important
to understand that negative timestamps are a part of ffmpeg, not part of the
encoded packet.  FFmpeg shifts these timestamps to be negative without touching
other streams like video.

We don't know of any cases where negative timestamps are returned for video,
only audio.  In the non-opus, non-ogg case we will simply rebase the negative
timestamps to be positive.

So your first statement is correct, vp9+aac (in separate containers) would be
rebased w/o discard. vp9+vorbis in webm doesn't have any negative timestamps
that we know of either, and hence this fix is not enabled. (theora|vp8)+vorbis
in ogg or (vp8|vp9)+opus in webm do have negative timestamps, but the negative
timestamps are manufactured by ffmpeg (and not baked into the video timestamps),
as such we only want to rebase and drop the audio timestamps while leaving video
alone.

Leaving video alone is not the default behavior, the default behavior is
everything gets rebased to >= 0, so this method is not specific to audio or
video and is not named as such.

[chcunningham, 2015-08-04 23:11:41]

LGTM. Thanks for all the info :D.

[wolenetz, 2015-08-06 22:21:27]

lgtm % 2 nits in pipeline_integration_test.cc:

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
File media/audio/clockless_audio_sink.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
media/audio/clockless_audio_sink.cc:43: return audio_hash_->ToString();
On 2015/07/30 01:28:14, DaleCurtis wrote:
> On 2015/07/29 21:57:27, wolenetz wrote:
> > nit: Is this for test only (I think so)? If not, then this might hit NPE on
> null
> > audio_hash_.get() if called without |hashing| true in ctor. Include
> "ForTesting"
> > in the name?
> 
> It is only for testing, but I can't add ForTesting() because presubmit isn't
> smart enough to figure out that one ForTesting method has called another
> ForTesting() method. I added a DCHECK().

Acknowledged.

https://codereview.chromium.org/1260193005/diff/70001/media/audio/clockless_a...
media/audio/clockless_audio_sink.cc:53: audio_hash_->Update(audio_bus_.get(),
frames_received);
On 2015/07/30 01:28:14, DaleCurtis wrote:
> On 2015/07/29 21:57:27, wolenetz wrote:
> > hmm. Could frames_received indeed be negative (see next line which implies
> > this)? If so, AudioHash::Update() casts int to uint32 and negative would be
a
> > problem.
> 
> No, it's always >= 0. I've added a DCHECK and fixed the misleading
conditional.

Acknowledged.

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
File media/filters/opus_audio_decoder.cc (right):

https://codereview.chromium.org/1260193005/diff/70001/media/filters/opus_audi...
media/filters/opus_audio_decoder.cc:475: // is the first packet and the
timestamp is zero, strip codec delay.
On 2015/07/31 02:41:00, DaleCurtis wrote:
> On 2015/07/30 01:28:14, DaleCurtis wrote:
> > On 2015/07/29 21:57:27, wolenetz wrote:
> > > Hmm. In MSE, the fix could be simpler: on config change, a new Opus
decoder
> is
> > > initialized. The Opus decoder output following that config change will
need
> > the
> > > first config->codec_delay samples discarded no matter what IIUC.
> > 
> > That understanding is not correct, only the first buffer has data which
needs
> > trimming. Only timestamps should be shifted by the pre-skip value, the
actual
> > decoded audio data should not be trimmed unless we're dealing with the
> > beginning-of-stream packet(s).
> > 
> > The timestamp shifting is done by ffmpeg for opus and webm  (see the code I
> > removed in FFmpegDemuxer), I'm not sure where or if it needs to happen for
> webm
> > in mse, though the file plays correctly from start, and a seek within the
> > pre-skip appears to work correctly -- however the midpoint seek does not
match
> > the results returned by ffmpeg for ogg/webm.
> > 
> > > 
> > > For example, append init segment containing Opus codec delay of 312
> > > mailto:frames@48Khz=6.5ms, and then time [10s,20s) of an Opus stream that
> > begins at
> > > time 0. Seek to time 10s. Ignoring the lack of seek preroll support for
Opus
> > in
> > > MSE, the first 6.5ms of decode output from the encoded media @ 10s would
> need
> > to
> > > be discarded.  This is because the PTS of 10s in webm is for a block
> > containing
> > > codec_delay frames (from previous packet spilling over transitively),
> followed
> > > by the frames that are really meant to be presented beginning at 10s.
> > > 
> > > So, instead of messing with start_timestamp_, couldn't we just apply Opus
> > codec
> > > delay discard padding to the first buffer passed to a new opus decoder
> > instance?
> > > Would this also work for ffmpeg demuxed buffers?
> > 
> 
> Turns out my understanding was totally wrong. It is correct that we always
want
> to drop pre-skip worth of samples since timestamp zero represents the
beginning
> of the pre-skip. The hash tests I ran previously were in fact forcing ffmpeg
to
> do the wrong thing. Sorry for all the confusion on my part.

No worries. Thanks for following-up on this point of confusion!

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:297: // If this is an OGG file with negative
timestamps don't rebase any other
nit: s/an OGG/a/ ?

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer_unittest.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer_unittest.cc:625: EXPECT_EQ(base::TimeDelta(),
demuxer_->GetStartTime());
Dale and I chatted a little offline about some potential problem if first real
audio PTS > 0 (and even the first
adjusted-earlier-in-time-by-preskip-or-codec-delay-ffmpeg-stream-timestamp > 0)
and > first real video PTS, though it seems less likely, probably impacting only
buffer durations (which are already imprecise from container timecode
granularity), and out of scope of this CL.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:502: chunk_demuxer_->Abort(
On 2015/08/04 20:38:09, chcunningham wrote:
> On 2015/08/03 22:49:53, DaleCurtis wrote:
> > On 2015/07/31 19:48:45, chcunningham wrote:
> > > Noob questions: I expected to see
> > > 
> > > chunk_demuxer->StartWaitingForSeek(...)
> > > chunk_demuxer->Seek(...)
> > > 
> > > I don't follow why we abort, nor why we don't actually call seek?
> > 
> > I don't know either, I just copied the Seek(a,b,c) method on l.486. If you
> have
> > suggestions which would work better let me know.
> 
> wolenetz@, do you know more on this?

Good catch! tl;dr, drop the Abort() from this new Seek() helper method.

The l.486 Seek helper actually appends some data, too. This explains why the
Abort() is done there. Perhaps that helper should be renamed to something like
SeekDemuxerAndAppend, or otherwise refactored to extract the abort+append out
into some other abstraction.

Actual Demuxer::Seek() is done by pipeline_->Seek() in these tests. The helper
in l.486 just prepares the demuxer for an upcoming seek (and has crufty
side-effect of aborting any previous append/remove algorithm and appending some
more data). This new helper just prepares the demuxer for the upcoming seek and
includes no abort+append side-effects.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:875: // Seek somewhere outside of the
pre-skip / end-trim section, demxuer should
nit: demuxer spellcheck

[DaleCurtis, 2015-08-06 23:45:27]

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:297: // If this is an OGG file with negative
timestamps don't rebase any other
On 2015/08/06 22:21:27, wolenetz wrote:
> nit: s/an OGG/a/ ?

Already fixed in later patch set.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:502: chunk_demuxer_->Abort(
On 2015/08/06 22:21:27, wolenetz wrote:
> On 2015/08/04 20:38:09, chcunningham wrote:
> > On 2015/08/03 22:49:53, DaleCurtis wrote:
> > > On 2015/07/31 19:48:45, chcunningham wrote:
> > > > Noob questions: I expected to see
> > > > 
> > > > chunk_demuxer->StartWaitingForSeek(...)
> > > > chunk_demuxer->Seek(...)
> > > > 
> > > > I don't follow why we abort, nor why we don't actually call seek?
> > > 
> > > I don't know either, I just copied the Seek(a,b,c) method on l.486. If you
> > have
> > > suggestions which would work better let me know.
> > 
> > wolenetz@, do you know more on this?
> 
> Good catch! tl;dr, drop the Abort() from this new Seek() helper method.
> 
> The l.486 Seek helper actually appends some data, too. This explains why the
> Abort() is done there. Perhaps that helper should be renamed to something like
> SeekDemuxerAndAppend, or otherwise refactored to extract the abort+append out
> into some other abstraction.
> 
> Actual Demuxer::Seek() is done by pipeline_->Seek() in these tests. The helper
> in l.486 just prepares the demuxer for an upcoming seek (and has crufty
> side-effect of aborting any previous append/remove algorithm and appending
some
> more data). This new helper just prepares the demuxer for the upcoming seek
and
> includes no abort+append side-effects.

Hmm, didn't work when I tried this originally, but does now! Done.

https://codereview.chromium.org/1260193005/diff/190001/media/test/pipeline_in...
media/test/pipeline_integration_test.cc:875: // Seek somewhere outside of the
pre-skip / end-trim section, demxuer should
On 2015/08/06 22:21:27, wolenetz wrote:
> nit: demuxer spellcheck

Done.

[DaleCurtis, 2015-08-06 23:45:37]

Thanks for review!

[DaleCurtis, 2015-08-06 23:45:42]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-08-06 23:45:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1260193005/#ps250001
(title: "Rebase. Comments.")

[commit-bot: I haz the power, 2015-08-06 23:47:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260193005/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260193005/250001

[DaleCurtis, 2015-08-07 00:17:40]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-08-07 00:17:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1260193005/#ps270001
(title: "Fix mojo renderer.")

[commit-bot: I haz the power, 2015-08-07 00:19:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260193005/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260193005/270001

[commit-bot: I haz the power, 2015-08-07 02:10:12]

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

[commit-bot: I haz the power, 2015-08-07 02:10:13]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[DaleCurtis, 2015-08-07 02:19:00]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-08-07 02:19:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260193005/270001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260193005/270001

[commit-bot: I haz the power, 2015-08-07 02:59:10]

Committed patchset #13 (id:270001)

[commit-bot: I haz the power, 2015-08-07 03:00:08]

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