Please note that this is different from  issue 723537 , even though both throw an "unfixable negative timestamp" error. The patch submitted to FFmpeg upstream does not solve this error on IMDb, nor does the workaround added to Chromium in  issue 723537 .

Yeah, the reason for that code is ffmpeg often lies about the start timestamp unfortunately. So a fix here might be to submit test media as a trac bug and indicate that the start time is being incorrectly determined by ffmpeg. I'll see if I can repro with ffmpeg master.

ffmpeg shows the start time as 0, then shows this as the first packet:

[PACKET]
codec_type=audio
stream_index=0
pts=-2048
pts_time=-0.046440
dts=-2048
dts_time=-0.046440
duration=1024
duration_time=0.023220
convergence_duration=N/A
convergence_duration_time=N/A
size=371
pos=61137
flags=KD
[SIDE_DATA]
side_data_type=Skip Samples
skip_samples=2048
discard_padding=0
skip_reason=0
discard_reason=0
[/SIDE_DATA]
[/PACKET]
[PACKET]
codec_type=audio
stream_index=0
pts=-1024
pts_time=-0.023220
dts=-1024
dts_time=-0.023220
duration=1024
duration_time=0.023220
convergence_duration=N/A
convergence_duration_time=N/A
size=372
pos=61508
flags=KD
[/PACKET]

I'll have to test some of the other samples we have, but it might be possible to drop our internal hunk and then when we encounter negative timestamps, require that they be marked with full discard (flags=D or skip_samples > negative packets) and set their timestamp to the start timestamp. I'll try to find some time to play around this since it'd be nice to simplify our internal start time code regardless.

I don't understand the details, but dropping the dependency on internal state would be nice. Thank you for looking into it!

Any chance the following CL is related to this issue, or could at least make it easier to fix?

https://chromium-review.googlesource.com/c/chromium/src/+/521757

It's definitely related, but not sufficient by itself. I poked at finishing this up along with that change, but ffmpeg still doesn't always mark the start time correctly without the custom hunk and the required work is more than I had time for at the moment. Will try to explore it more this Q though.

Interestingly enough, the example IMDb trailer from my original message works in Chromium 67.0.3396.40 compiled against system FFmpeg 4.0. However, I'm not sure if the fix happened on the Chromium or FFmpeg side, and can't use git bisect to find this out because Chromium 67 doesn't seem to play nice with FFmpeg 3.4. [1]

I'll probably release Chromium 67 to Arch with system FFmpeg 4.0 and see if I get any user complaints. :)

(If you happen to have any insight on what fixed this, I'm all ears! Perhaps the internal hunk could be dropped as well.)

[1] https://bugs.gentoo.org/654208

Thanks for the update. Probably something in ffmpeg changed. I'll see if we need our internal hunk anymore.

Yay, can be removed https://chromium-review.googlesource.com/c/chromium/src/+/1064538

Ah, kind of, ffmpeg is still broken for theora, but there's another workaround we can use, so it can still be removed!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b

commit f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu May 17 23:44:41 2018

Remove dependency on ffmpeg internals for start time calculations.

Some theora clips still have issues, but using first_dts where
we know it's the same as the first pts resolves the lingering
issues. It also looks like we don't need to use pts_buffer now.

This does the following small fixes:
- MEDIA_LOG(DEBUG) -> MEDIA_LOG(ERROR) for negative ts error.
- Enables previous disabled test in more limited state.
- Adds kNoFFmpegTimestamp so static_cast<int64_t>(AV_NOPTS_VALUE)
is no longer necessary everywhere. A followup patch set will use
this is more places.
- Removes pts_buffer and packet_buffer inspection.

BUG= 731766 
TEST=all tests pass.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I5aadf67a3b5ea2d2a8dd19bbddd7b107208094c5
Reviewed-on: https://chromium-review.googlesource.com/1064538
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559737}
[modify] https://crrev.com/f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b/media/ffmpeg/ffmpeg_common.h
[modify] https://crrev.com/f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b/media/filters/ffmpeg_demuxer.cc
[modify] https://crrev.com/f9535bd6d61d7e0b2cb452e6976a914d4ee62a2b/media/filters/ffmpeg_demuxer_unittest.cc

Should be fixed now, but will want to monitor to make sure this doesn't result in any increases of the DEMUXER_ERROR_COULD_NOT_PARSE error code.

Internal link for monitoring over the next week:
https://uma.googleplex.com/p/chrome/timeline_v2/?sid=cebb194ed38c7b0deffa8dafbd3b8df5

Thanks for the fix! Fingers crossed there won't be an increase. 🤞

The NextAction date has arrived: 2018-05-21

Looks normal so far, will check again next week.

The NextAction date has arrived: 2018-05-29

Still no increases, so looks like we can call this fixed!