Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2737653002:
Buffer 2 seconds of data in the ffmpeg demuxer. (Closed)
|
Created:
3 years, 9 months ago by hubbe Modified:
3 years, 9 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionBuffer 2 seconds of data in the ffmpeg demuxer.
This has been shown to reduce jank in badly muxed mp4 files.
BUG=540898
Review-Url: https://codereview.chromium.org/2737653002
Cr-Original-Commit-Position: refs/heads/master@{#455551}
Committed: https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb0094968481d6e00514
Review-Url: https://codereview.chromium.org/2737653002
Cr-Commit-Position: refs/heads/master@{#459565}
Committed: https://chromium.googlesource.com/chromium/src/+/83bbe324e42ebe5b1205d6caeb1ea4149bad31f1
Patch Set 1 #Patch Set 2 : fixed tests #Patch Set 3 : adjust layout tests #Patch Set 4 : clear error on read abort #
Total comments: 2
Patch Set 5 : UnmarkEndOfStream -> UnmarkEndOfStreamAndClearError #
Messages
Total messages: 47 (28 generated)
The CQ bit was checked by hubbe@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...
hubbe@chromium.org changed reviewers: + dalecurtis@google.com
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Did you test this? It did not help when I investigated this issue previously.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/03/07 00:08:16, DaleCurtis wrote: > See http://crbug.com/681138 I'm back with more data (https://memegen.googleplex.com/5703417742753792) Telemetry results from various bots: https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... Since there doesn't seem to be a sane way to aggregate these results, I wrote one: https://drive.google.com/open?id=0B3fTtFWXDMr0UW5UWVN5UlJpOXc Here is the output: https://drive.google.com/open?id=0B3fTtFWXDMr0cVUwOGVjNXVXbUE Basically, it doesn't seem to change anything that much, however there are a few outliers: dropped_frame_count goes down significantly in a few cases. time_to_play goes down significantly in a few cases. buffering_time goes up OR down significantly in a few cases. Not sure if these outliers are noise or not. Obviously I also need to fix the tests if I'm going to check this in.
On 2017/03/07 at 21:49:00, hubbe wrote: > On 2017/03/07 00:08:16, DaleCurtis wrote: > > See http://crbug.com/681138 > > I'm back with more data (https://memegen.googleplex.com/5703417742753792) > > Telemetry results from various bots: > https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... > https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... > https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... > https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... > https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... > https://console.developers.google.com/m/cloudstorage/b/chromium-telemetry/o/h... > > Since there doesn't seem to be a sane way to aggregate these results, I wrote one: > > https://drive.google.com/open?id=0B3fTtFWXDMr0UW5UWVN5UlJpOXc > > Here is the output: https://drive.google.com/open?id=0B3fTtFWXDMr0cVUwOGVjNXVXbUE > > Basically, it doesn't seem to change anything that much, however there are a few outliers: > > dropped_frame_count goes down significantly in a few cases. > time_to_play goes down significantly in a few cases. > buffering_time goes up OR down significantly in a few cases. > > Not sure if these outliers are noise or not. > > Obviously I also need to fix the tests if I'm going to check this in. Nice work, seems mostly a wash so if we have actual bugs this fixes I think it's worth landing and re-checking the bots after.
Tests updated. PTAL.
lgtm
The CQ bit was checked by hubbe@chromium.org
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hubbe@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.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2737653002/#ps40001 (title: "adjust layout tests")
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": 1489009991374440,
"parent_rev": "17c41c581b3eab40fd9a5eb95b108617bd9f7840", "commit_rev":
"e23907eaba82f5595b74fb0094968481d6e00514"}
Message was sent while issue was closed.
Description was changed from ========== Buffer 2 seconds of data in the ffmpeg demuxer. This has been shown to reduce jank in badly muxed mp4 files. BUG=540898 ========== to ========== Buffer 2 seconds of data in the ffmpeg demuxer. This has been shown to reduce jank in badly muxed mp4 files. BUG=540898 Review-Url: https://codereview.chromium.org/2737653002 Cr-Commit-Position: refs/heads/master@{#455551} Committed: https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb009496... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb009496...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2740863002/ by sammc@chromium.org. The reason for reverting is: Broke layout test compositing/video/video-poster.html on Mac, Linux ASAN: https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Mac10.9/43534 https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Mac10.10/31122 https://luci-milo.appspot.com/buildbot/chromium.webkit/WebKit%20Linux%20Trust....
Message was sent while issue was closed.
Description was changed from ========== Buffer 2 seconds of data in the ffmpeg demuxer. This has been shown to reduce jank in badly muxed mp4 files. BUG=540898 Review-Url: https://codereview.chromium.org/2737653002 Cr-Commit-Position: refs/heads/master@{#455551} Committed: https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb009496... ========== to ========== Buffer 2 seconds of data in the ffmpeg demuxer. This has been shown to reduce jank in badly muxed mp4 files. BUG=540898 Review-Url: https://codereview.chromium.org/2737653002 Cr-Commit-Position: refs/heads/master@{#455551} Committed: https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb009496... ==========
clear error on read abort
The CQ bit was checked by hubbe@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...
PTAL Finally figured out why this brakes some asan tests. When a seek happens, we abort the reads and return EIO. ffmpeg remembers this error until we get to the end of the stream and returns it back then, which causes a demuxer failure. I added code to make it clear the error when a read is aborted, which makes the tests work.
dalecurtis@chromium.org changed reviewers: - dalecurtis@google.com
lgtm https://codereview.chromium.org/2737653002/diff/60001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2737653002/diff/60001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:254: static void UnmarkEndOfStream(AVFormatContext* format_context) { Rename function?
https://codereview.chromium.org/2737653002/diff/60001/media/filters/ffmpeg_de... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/2737653002/diff/60001/media/filters/ffmpeg_de... media/filters/ffmpeg_demuxer.cc:254: static void UnmarkEndOfStream(AVFormatContext* format_context) { On 2017/03/24 19:13:13, DaleCurtis wrote: > Rename function? UnmarkEndofStreamAndClearError?
sgtm
The CQ bit was checked by hubbe@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.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2737653002/#ps80001 (title: "UnmarkEndOfStream -> UnmarkEndOfStreamAndClearError")
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": 80001, "attempt_start_ts": 1490391388190840,
"parent_rev": "095bbad98260feae3a161c19ca1e9d9809dd544b", "commit_rev":
"83bbe324e42ebe5b1205d6caeb1ea4149bad31f1"}
Message was sent while issue was closed.
Description was changed from ========== Buffer 2 seconds of data in the ffmpeg demuxer. This has been shown to reduce jank in badly muxed mp4 files. BUG=540898 Review-Url: https://codereview.chromium.org/2737653002 Cr-Commit-Position: refs/heads/master@{#455551} Committed: https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb009496... ========== to ========== Buffer 2 seconds of data in the ffmpeg demuxer. This has been shown to reduce jank in badly muxed mp4 files. BUG=540898 Review-Url: https://codereview.chromium.org/2737653002 Cr-Original-Commit-Position: refs/heads/master@{#455551} Committed: https://chromium.googlesource.com/chromium/src/+/e23907eaba82f5595b74fb009496... Review-Url: https://codereview.chromium.org/2737653002 Cr-Commit-Position: refs/heads/master@{#459565} Committed: https://chromium.googlesource.com/chromium/src/+/83bbe324e42ebe5b1205d6caeb1e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/83bbe324e42ebe5b1205d6caeb1e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
