Playback of fragmented mp4s sometimes downloads the whole file first. |
||||||||
Issue descriptionSee the clip in issue 568336 . This occurs because we enabled 'sidx' box processing in the most recent ffmpeg roll, but disable mov_seek_fragment(). Playback of the clip in that issue shows long delays in initial playback while waiting for download. Instead we should just entirely disabled sidx processing, but leave mov_seek_fragment() alone. Concurrently I've reached out to ffmpeg about the issue since it's still reproducible with ffplay: http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213025.html We used to have constrained network tests which would catch this, but no longer do, so we should instead add a test which ensures not too much of a clip is read when seeking.
,
Jul 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f762d2d7fae21aa756013a35e4612281c0cbb054 commit f762d2d7fae21aa756013a35e4612281c0cbb054 Author: Dale Curtis <dalecurtis@chromium.org> Date: Sat Jul 01 00:47:27 2017 Roll ffmpeg deps to pick sidx processing fix. BUG= 738595 TEST=manual TBR=jrummell Change-Id: Ib361c92408303d9f455f874f7b076e290ae63270 Reviewed-on: https://chromium-review.googlesource.com/558625 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: John Rummell <jrummell@chromium.org> Commit-Queue: Dale Curtis <dalecurtis@chromium.org> Cr-Commit-Position: refs/heads/master@{#483876} [modify] https://crrev.com/f762d2d7fae21aa756013a35e4612281c0cbb054/DEPS
,
Jul 14 2017
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/third_party/ffmpeg/+/d19b0ad9b26a4def3411e32a8b780540afbd6a6e commit d19b0ad9b26a4def3411e32a8b780540afbd6a6e Author: Dale Curtis <dalecurtis@chromium.org> Date: Thu Jul 20 03:27:17 2017 Fix trampling of ctts during seeks when sidx support is enabled. When sidx box support is enabled, the code will skip reading all trun boxes (each containing ctts entries for samples inthat box). If seeks are attempted before all ctts values are known, the old code would dump ctts entries into the wrong location. These are then used to compute pts values which leads to out of order and incorrectly timestamped packets. This patch fixes ctts processing by always using the index returned by av_add_index_entry() as the ctts_data index. When the index gains new entries old values are reshuffled as appropriate. This approach makes sense since the mov demuxer is already relying on the mapping of AVIndex entries to samples for correct demuxing. Notes for future improvement: Probably there are other boxes (stts, stsc, etc) that are impacted by this issue... this patch only attempts to fix ctts since it completely breaks packet timestamping. This patch continues using an array for the ctts data, which is not the most ideal given the rearrangement that needs to happen (via memmove as new entries are read in). Ideally AVIndex and the ctts data would be set-type structures so addition is always worst case O(lg(n)) instead of the O(n^2) that exists now; this slowdown is noticeable during seeks. Additionally since ctts samples from trun boxes always have a count of 1, there's probably an opportunity to avoid the post-seek fixup that iterates O(n) over all samples with an O(1) when no non-1 count samples are present. Bug: 738595 Change-Id: I3b519a817be03fa54cad95c0bd17057f8172aec5 Reviewed-on: https://chromium-review.googlesource.com/572585 Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org> [modify] https://crrev.com/d19b0ad9b26a4def3411e32a8b780540afbd6a6e/libavformat/isom.h [modify] https://crrev.com/d19b0ad9b26a4def3411e32a8b780540afbd6a6e/libavformat/mov.c [modify] https://crrev.com/d19b0ad9b26a4def3411e32a8b780540afbd6a6e/chromium/patches/README
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a269a002a61058e83c710d49b43c5674df7efbe commit 5a269a002a61058e83c710d49b43c5674df7efbe Author: Dale Curtis <dalecurtis@chromium.org> Date: Thu Jul 20 05:06:39 2017 Roll DEPS to pick up ffmpeg sidx box fixes. BUG= 738595 TEST=existing tests pass TBR=wolenetz Change-Id: I1076fc1a74b7064bff021c5c124f20216fc0867e Reviewed-on: https://chromium-review.googlesource.com/578656 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Commit-Queue: Dale Curtis <dalecurtis@chromium.org> Cr-Commit-Position: refs/heads/master@{#488140} [modify] https://crrev.com/5a269a002a61058e83c710d49b43c5674df7efbe/DEPS
,
Jul 24 2017
Should be fixed in M61 now.
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94d248f20319c705b7fda1f2074515c72b053485 commit 94d248f20319c705b7fda1f2074515c72b053485 Author: hubbe <hubbe@chromium.org> Date: Thu Jul 27 22:40:55 2017 media: Add UMA stats for bytes going in and out of multibuffer cache This will help us diagnose regressions which cause us to read more data than we need. BUG= 738595 Review-Url: https://codereview.chromium.org/2983733002 Cr-Commit-Position: refs/heads/master@{#490024} [modify] https://crrev.com/94d248f20319c705b7fda1f2074515c72b053485/media/blink/resource_multibuffer_data_provider.cc [modify] https://crrev.com/94d248f20319c705b7fda1f2074515c72b053485/media/blink/url_index.cc [modify] https://crrev.com/94d248f20319c705b7fda1f2074515c72b053485/media/blink/url_index.h [modify] https://crrev.com/94d248f20319c705b7fda1f2074515c72b053485/tools/metrics/histograms/histograms.xml
,
Jul 28 2017
I'd like to merge the UMA stat CL to M61
,
Jul 28 2017
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 29 2017
Before we approve merge to M61 for CL listed at #7 (UMA stat), could you please confirm change is well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61?
,
Jul 31 2017
The UMA change was checked in ~4 days ago, it's a very simple change and should be completely safe to merge.
,
Jul 31 2017
Approving merge to M61 for CL listed at #7 (UMA stat) based on comment #11.
,
Aug 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3c3b41ba2cfbe20c88dfff019f06fff08ae54149 commit 3c3b41ba2cfbe20c88dfff019f06fff08ae54149 Author: Fredrik Hubinette <hubbe@google.com> Date: Tue Aug 01 18:56:29 2017 media: Add UMA stats for bytes going in and out of multibuffer cache This will help us diagnose regressions which cause us to read more data than we need. BUG= 738595 TBR=hubbe@chromium.org (cherry picked from commit 94d248f20319c705b7fda1f2074515c72b053485) Review-Url: https://codereview.chromium.org/2983733002 Cr-Original-Commit-Position: refs/heads/master@{#490024} Change-Id: Ib60849fc3a0c433e672be90290940b718dec4824 Reviewed-on: https://chromium-review.googlesource.com/596610 Reviewed-by: Fredrik Hubinette <hubbe@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#216} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/3c3b41ba2cfbe20c88dfff019f06fff08ae54149/media/blink/resource_multibuffer_data_provider.cc [modify] https://crrev.com/3c3b41ba2cfbe20c88dfff019f06fff08ae54149/media/blink/url_index.cc [modify] https://crrev.com/3c3b41ba2cfbe20c88dfff019f06fff08ae54149/media/blink/url_index.h [modify] https://crrev.com/3c3b41ba2cfbe20c88dfff019f06fff08ae54149/tools/metrics/histograms/histograms.xml
,
Aug 3 2017
Verified this issue on Ubuntu 14.04, Windows-10 and Mac OS 10.12.6 using chrome latest M61-61.0.3163.29. By opening the below link https://storage.googleapis.com/chromiumos-test-assets-public/Shaka-Dash/480.mp4 observed able to play video immediately without any long delay at first instance. Confirming as per comment #13 fix is working fine and adding TE-Verified label for M-61. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bugdroid1@chromium.org
, Jun 30 2017