Fix corrupted audio and video at playback start in ogg containers.

Fix corrupted audio and video at playback start in ogg containers.

When demuxing ogg containers, FFmpeg will seek to the first packet in the
chosen stream that matches the seek criteria.  However, it does not advance
or rewind other streams in the container to match this timeline.

It's an open question whether FFmpeg is doing the wrong thing here:
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2014-June/159212.html

To keep things working in the meantime negative audio start times will
be clipped to zero when being considered for seeking.

Additional fixes:
- Fixes non-audio timestamps being incorrectly offset when negative
timestamps are discarded from the audio stream.

BUG=387996
TEST=new unittest, extended existing tests.

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

[DaleCurtis, 2014-06-24 00:16:22]

Seeking is hard, lets go shopping? :o

[DaleCurtis, 2014-06-24 01:43:34]

PS#2 fixes a seeking issue.  See the bug for details.

[acolwell GONE FROM CHROMIUM, 2014-06-25 16:32:44]

I have concerns about this change. It really seems like this sort of logic
should be in the FFmpeg code. I feel like the Ogg seeking logic must be broken
in some way since seeking to 0 should always result in the negative timestamp
packets being returned since samples at time 0 depend on them.

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

https://codereview.chromium.org/353563002/diff/70001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:268: if (discard_negative_timestamps_ && type()
== VIDEO &&
This seems very Ogg specific without actually mentioning Ogg anywhere. It isn't
clear to me that this logic should apply for all video. It seems like you should
control this with a separate "subtract off start_time" bool since this is really
controlling whether timestamps are modified or not. That seems like a cleaner
way to represent what you are trying to do here.

[DaleCurtis, 2014-06-25 20:37:56]

I'll try and craft a minimized test case (or better yet, try and use the command
line tools to show the issue) and see what upstream says.

[DaleCurtis, 2014-06-25 22:38:41]

Message sent to upstream:
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2014-June/159212.html

[DaleCurtis, 2014-06-26 23:52:08]

The reply upstream is that ffmpeg doesn't guarantee packet order on timestamps
on file position.  Format with seek tables (mp4, matroska) are supposedly immune
to this problem.  There's avformat_seek_file() which will eventually make a
similar guarantee, but is still in beta and not supported for ogg yet.

I think the solution I have here is minimally invasive and accomplishes what we
want: Use the preferred stream when possible, otherwise choose the lowest file
order position.

[acolwell GONE FROM CHROMIUM, 2014-06-27 16:35:00]

On 2014/06/26 23:52:08, DaleCurtis wrote:
> The reply upstream is that ffmpeg doesn't guarantee packet order on timestamps
> on file position.  Format with seek tables (mp4, matroska) are supposedly
immune
> to this problem.  There's avformat_seek_file() which will eventually make a
> similar guarantee, but is still in beta and not supported for ogg yet.
> 
> I think the solution I have here is minimally invasive and accomplishes what
we
> want: Use the preferred stream when possible, otherwise choose the lowest file
> order position.

This still doesn't seem like the right solution to me. This is being cast as a
generic solution even though we are dealing with an Ogg specific problem. It
seems like FFmpeg's decision to represent sample trimming with negative
timestamps is not compatible with their seeking logic. We should fix the Ogg
code to handle this case properly and not force Chrome code to deal with it.
ISTM that the Chrome code is making a bad assumption that just because the audio
stream has a lower timestamp that it should be the preferred stream. The lower
timestamp is a lie created by FFmpeg. Both streams technically start at 0 and it
seems to me that we should be calling FFmpeg with 0 instead of a negative
timestamp. Even if we still need to call seek with a negative timestamp, the Ogg
demuxing code is in the best position to resolve where to start playback since
it can see if the previous page contains data that is after the seek point. We
shouldn't be baking these kinds of assumptions into Chrome.

[DaleCurtis, 2014-06-27 20:52:32]

PTAL.  I've reworked the patch to be clarify that all these fixups are
specifically for ogg.  I've also limited the file position code to
OnFindStreamInfoDone().  If you're still not happy, we can disable the
Read_AudioNegativeStartTimeAndOggDiscard_Bear test and live with missed audio
(which is what happened prior to all my changes).

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

https://codereview.chromium.org/353563002/diff/70001/media/filters/ffmpeg_dem...
media/filters/ffmpeg_demuxer.cc:268: if (discard_negative_timestamps_ && type()
== VIDEO &&
On 2014/06/25 16:32:44, acolwell wrote:
> This seems very Ogg specific without actually mentioning Ogg anywhere. It
isn't
> clear to me that this logic should apply for all video. It seems like you
should
> control this with a separate "subtract off start_time" bool since this is
really
> controlling whether timestamps are modified or not. That seems like a cleaner
> way to represent what you are trying to do here.

I tried this a couple different ways and they're all pretty messy.  Instead I've
renamed discard_negative_timestamps_ to fixup_negative_ogg_timestamps_ to
reflect that this is specifically for ogg fixups only.

[DaleCurtis, 2014-06-27 21:10:33]

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

https://codereview.chromium.org/353563002/diff/110001/media/filters/ffmpeg_de...
media/filters/ffmpeg_demuxer.cc:693: // FFmpeg doesn't guarantee packets will be
in timestamp order across streams
If you decide we should keep this, I can refactor this whole section into a
static method so it's not polluting OnFindStreamInfoDone().

[DaleCurtis, 2014-06-27 22:23:21]

I've pulled out the seek ( https://codereview.chromium.org/353163004/ ) fix so
there's no rush on this change.

[DaleCurtis, 2014-06-30 21:19:20]

As discussed offline, I've removed the file pos workaround and disabled the test
for now.

[acolwell GONE FROM CHROMIUM, 2014-06-30 22:33:02]

lgtm

[DaleCurtis, 2014-06-30 22:37:41]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-06-30 22:38:58]

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

[commit-bot: I haz the power, 2014-07-01 00:58:33]

Change committed as 280720