Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563183002:
Fix up missing timestamps in FFmpegDemuxer. (Closed)
|
Created:
4 years ago by liberato (no reviews please) Modified:
3 years, 12 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix up missing timestamps in FFmpegDemuxer.
Previously, we would DCHECK if FFmpeg provded no timestamp for a
buffer. We now replace this with zero for the first packet, and
with an advance over the previous buffer for later ones.
The rationale is that bad media might cause this, and we previously
had a DCHECK to catch it. This makes the behavior consistent even
in releaes builds.
BUG=665305, 673079
TEST=ffmpeg_regression_tests
Committed: https://crrev.com/722dfe42527ed575678e5145ef3e58f03013030a
Cr-Commit-Position: refs/heads/master@{#439933}
Patch Set 1 #
Total comments: 1
Patch Set 2 : throws an error instead #Patch Set 3 : added demuxer status to media log #
Total comments: 1
Messages
Total messages: 30 (21 generated)
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix up missing timestamps in FFmpegDemuxer. Previously, we would DCHECK if FFmpeg provded no timestamp for a buffer. We now replace this with zero for the first packet, and with an advance over the previous buffer for later ones. The rationale is that bad media might cause this, and we previously had a DCHECK to catch it. This makes the behavior consistent even in releaes builds. BUG=665305 TEST=ffmpeg_regression_tests ========== to ========== Fix up missing timestamps in FFmpegDemuxer. Previously, we would DCHECK if FFmpeg provded no timestamp for a buffer. We now replace this with zero for the first packet, and with an advance over the previous buffer for later ones. The rationale is that bad media might cause this, and we previously had a DCHECK to catch it. This makes the behavior consistent even in releaes builds. BUG=665305, 673079 TEST=ffmpeg_regression_tests ==========
liberato@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/2563183002/diff/1/media/filters/ffmpeg_demuxe... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2563183002/diff/1/media/filters/ffmpeg_demuxe... media/filters/ffmpeg_demuxer.cc:560: if (!fixup_negative_timestamps_ && !warned_about_missing_timestamps_) { "!fixup..." to prevent a warning when we wouldn't have before. no idea if we actually care about this.
Why fixup vs error out?
On 2016/12/12 23:04:25, DaleCurtis wrote: > Why fixup vs error out? i was planning to make it error, since it's simpler. i don't remember what made me not do that. keep calm and carry on, i guess. i'll try it the other way and see if i remember. thanks -fl
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
i remember now -- i wasn't sure if i was breaking existing behavior outside of the tests. however, i realize now that i wasn't, since it would fail anyway. anyway, error now makes more sense, so here it is. thanks -fl
lgtm though we'll want to watch the UMA statistics for this error to see if it spikes; if so we may need your original workaround.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by liberato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482267882604790,
"parent_rev": "ad00424f9940d61f45b63a929a0cbac1097f1495", "commit_rev":
"95c83f53e64766f3850fb8464bb2c01729521000"}
Message was sent while issue was closed.
Description was changed from ========== Fix up missing timestamps in FFmpegDemuxer. Previously, we would DCHECK if FFmpeg provded no timestamp for a buffer. We now replace this with zero for the first packet, and with an advance over the previous buffer for later ones. The rationale is that bad media might cause this, and we previously had a DCHECK to catch it. This makes the behavior consistent even in releaes builds. BUG=665305, 673079 TEST=ffmpeg_regression_tests ========== to ========== Fix up missing timestamps in FFmpegDemuxer. Previously, we would DCHECK if FFmpeg provded no timestamp for a buffer. We now replace this with zero for the first packet, and with an advance over the previous buffer for later ones. The rationale is that bad media might cause this, and we previously had a DCHECK to catch it. This makes the behavior consistent even in releaes builds. BUG=665305, 673079 TEST=ffmpeg_regression_tests Review-Url: https://codereview.chromium.org/2563183002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix up missing timestamps in FFmpegDemuxer. Previously, we would DCHECK if FFmpeg provded no timestamp for a buffer. We now replace this with zero for the first packet, and with an advance over the previous buffer for later ones. The rationale is that bad media might cause this, and we previously had a DCHECK to catch it. This makes the behavior consistent even in releaes builds. BUG=665305, 673079 TEST=ffmpeg_regression_tests Review-Url: https://codereview.chromium.org/2563183002 ========== to ========== Fix up missing timestamps in FFmpegDemuxer. Previously, we would DCHECK if FFmpeg provded no timestamp for a buffer. We now replace this with zero for the first packet, and with an advance over the previous buffer for later ones. The rationale is that bad media might cause this, and we previously had a DCHECK to catch it. This makes the behavior consistent even in releaes builds. BUG=665305, 673079 TEST=ffmpeg_regression_tests Committed: https://crrev.com/722dfe42527ed575678e5145ef3e58f03013030a Cr-Commit-Position: refs/heads/master@{#439933} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/722dfe42527ed575678e5145ef3e58f03013030a Cr-Commit-Position: refs/heads/master@{#439933}
Message was sent while issue was closed.
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
Message was sent while issue was closed.
For posterity, change description mismatches the change: (instead of fixup, it's really error-out). Already landed, so this is just a note for anyone who might similarly have been briefly confused :) https://codereview.chromium.org/2563183002/diff/40001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/2563183002/diff/40001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.h:238: // Allow FFmpegDemxuerStream to notify us about an error. nit: typo |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
