Issue metadata
Sign in to add a comment
|
HLS video playback sometimes stalls out before playback can begin
Reported by
agd...@amazon.com,
Aug 13
|
||||||||||||||||||||||
Issue descriptionExample URL: video.foxnews.com Steps to reproduce the problem: 1. Launch Chrome on Android 2. Enable "Request Desktop Sites" 3. Navigate to video.foxnews.com 4. The first video that loads will attempt to autoplay and will be not be allowed to do so (expected) 5. Scroll down to the list of "Editor's Picks" and select a video - the video should load and attempt to begin playing 6. If the video begins playing without issue, repeat step 5 by selecting a different video What is the expected behavior? What went wrong? On my Pixel 2 (running Android P), the video will fail to play once in every 8 attempts or so. When it does fail, without further input, the spinner over the video will spin indefinitely. It's possible to get the video to play by tapping on the video timeline to seek the video. I built the chrome_public_apk target in an Android workspace on the tip of master with a lot of logging added to the PipelineImpl, PipelineController, and WMPI, and found that the difference between a video playing and failing to play is whether or not OnBufferingStateChange() was called prior to OnTrackChangeComplete(). Reverting da82b60044 locally appears to cause this issue to go away. Did this work before? Yes v67 Is it a problem with Flash or HTML5? HTML5 Does this work in other browsers? N/A Chrome version: v68+ (after da82b60044) Channel: n/a OS Version: P Flash Version: Contents of chrome://gpu: I noticed an issue with HLS video playback sometimes stalling out before playback can begin in Chrome on Android after da82b60044. As far as I can tell, this issue is triggered by PipelineImpl::OnBufferingStateChange(BUFFERING_HAVE_ENOUGH) [1] getting called prior to PipelineController::OnTrackChangeComplete() [2]. This causes the call to WMPI::OnBufferingStateChangeInternal() to return early because the PipelineController is not stable [3], and the pipeline is not stable because OnTrackChangeComplete() has not been called yet. After OnTrackChangeComplete() is finally called, there's nothing to trigger playback to actually begin (this is normally accomplished by OnBufferingStateChange() getting called). This has been easiest to reproduce on video.foxnews.com with "Request Desktop Sites" enabled (required to get served HLS video), but I've seen the same issue on espn.com.
,
Aug 13
,
Aug 16
,
Aug 17
We should still see if there's a bug in the pipeline controller, but blink shouldn't be trying to enable tracks here. https://chromium-review.googlesource.com/c/chromium/src/+/1180347
,
Aug 17
,
Aug 20
This is breaking HLS playback for a number of sites, we should make sure we get a fix in for M69.
,
Aug 20
,
Aug 21
I've been trying a few things: preventing track changes from happening before the pipeline is set up: https://chromium-review.googlesource.com/c/chromium/src/+/1182445 this doesn't work, since the track change is happening after setup anyway - I put in some tracing with this patch and got: 08-20 17:48:34.462 15234 15306 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 17:48:34.642 15234 15306 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=video, state=enough) 08-20 17:48:34.646 15234 15249 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 17:48:43.346 15234 15306 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 17:48:43.587 15234 15306 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=video, state=enough) 08-20 17:48:44.719 15234 15249 I chromium: [INFO:pipeline_controller.cc(238)] Triggering track change 08-20 17:48:44.736 15234 15249 I chromium: [INFO:pipeline_controller.cc(394)] OnTrackChangeComplete 08-20 17:48:44.736 15234 15249 I chromium: [INFO:pipeline_controller.cc(238)] Triggering track change 08-20 17:48:44.737 15234 15249 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 17:48:44.737 15234 15249 I chromium: [INFO:webmediaplayer_impl.cc(1784)] OnBufferingStateChangeInternal Unstable!!! 08-20 17:48:44.780 15234 15249 I chromium: [INFO:pipeline_controller.cc(394)] OnTrackChangeComplete 08-20 17:48:44.813 15234 15306 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 17:48:44.990 15234 15306 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=video, state=enough) 08-20 17:48:44.999 15234 15249 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal Piping the track change thru MojoRenderer: can't make this compile, due to a lack of knowledge about how to pass params of type vector<DemuxerStream*> in mojo also not sure if this would work, since HLS should be supporting track changes like that anyway. With Dale's CL from comment #4 applied though, the same tracing looks like: 08-20 18:38:34.396 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:35.583 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 18:38:35.719 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=video, state=enough) 08-20 18:38:35.787 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:38.314 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=nothing) 08-20 18:38:39.054 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:39.167 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 18:38:39.491 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:39.841 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=nothing) 08-20 18:38:40.077 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:41.201 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 18:38:41.226 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=video, state=nothing) 08-20 18:38:41.451 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=video, state=enough) 08-20 18:38:41.583 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:42.714 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=nothing) 08-20 18:38:43.239 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal 08-20 18:38:44.366 17949 18036 I chromium: [INFO:renderer_impl.cc(668)] OnBufferingStateChange(type=audio, state=enough) 08-20 18:38:44.514 17949 17964 I chromium: [INFO:webmediaplayer_impl.cc(1778)] OnBufferingStateChangeInternal and the videos all play fine; this seems to avoid the problem entirely, and I think might be the right choice. Ideally I would like to see the mojo solution work, or at least figure out a way to prevent track changes entirely when mojo_renderer is in use.
,
Aug 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/742212b82e34205ea0d985cc60e159ddb87ec47d commit 742212b82e34205ea0d985cc60e159ddb87ec47d Author: Ted Meyer <tmathmeyer@chromium.org> Date: Wed Aug 22 17:21:42 2018 Dont drop buffering updates from track switches Buffering state changes should only be dropped when two seek operations are queued, as we dont want the state change from the first to be misinterpreted as the second one finishing. This manifested itself as a severe issue with HLS playback (especially on android phones) where track switches were causing the buffering updates used to signal startup to be dropped. Bug: 873837 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: I3930bb446f69a2c166419aedd0b5a97d3200ce49 Reviewed-on: https://chromium-review.googlesource.com/1183893 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Ted Meyer <tmathmeyer@chromium.org> Cr-Commit-Position: refs/heads/master@{#585123} [modify] https://crrev.com/742212b82e34205ea0d985cc60e159ddb87ec47d/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/742212b82e34205ea0d985cc60e159ddb87ec47d/media/filters/pipeline_controller.cc [modify] https://crrev.com/742212b82e34205ea0d985cc60e159ddb87ec47d/media/filters/pipeline_controller.h
,
Aug 22
,
Aug 22
[Auto-generated comment by a script] We noticed that this issue is targeted for M-69; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-69 label, otherwise remove Merge-TBD label. Thanks.
,
Aug 22
Once verified on canary we should add a Merge-Request-69 since this is breaking HLS playbacks on Android for a variety of sites.
,
Aug 22
This bug requires manual review: We are only 12 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ad7d871be92550674b3bdab80352b3c7e3edfdbd commit ad7d871be92550674b3bdab80352b3c7e3edfdbd Author: Dale Curtis <dalecurtis@chromium.org> Date: Thu Aug 23 00:32:52 2018 Don't attempt to enable non-existant placeholder tracks. The media pipeline does not expect to be called about track changes prior to HaveCurrentData. There's also no reason for the element to create disabled video+audio tracks and then tell the renderer to enable those very same non-existant tracks. Just create them as enabled instead. It's possible we can remove even the creation of placeholder tracks, but that's a bigger web facing change, so I don't want to hold up this bug fix on that analysis. BUG= 873837 TEST=new unittest, twitter videos no longer hang. Change-Id: I9863ae4feca29a58140e6dad1532bc8b7a380c3b Reviewed-on: https://chromium-review.googlesource.com/1180347 Commit-Queue: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Cr-Commit-Position: refs/heads/master@{#585334} [modify] https://crrev.com/ad7d871be92550674b3bdab80352b3c7e3edfdbd/third_party/blink/renderer/core/html/media/html_media_element.cc [modify] https://crrev.com/ad7d871be92550674b3bdab80352b3c7e3edfdbd/third_party/blink/renderer/core/html/media/html_media_element.h [modify] https://crrev.com/ad7d871be92550674b3bdab80352b3c7e3edfdbd/third_party/blink/renderer/core/html/media/html_media_element_test.cc
,
Aug 23
Approved for merge into 69, branch 3497.
,
Aug 23
,
Aug 23
Will handle the merge in the next hour.
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/67cb48b1b830e3e1c2a3390bf4f52e627c05cacc commit 67cb48b1b830e3e1c2a3390bf4f52e627c05cacc Author: Ted Meyer <tmathmeyer@chromium.org> Date: Thu Aug 23 19:44:55 2018 Dont drop buffering updates from track switches Buffering state changes should only be dropped when two seek operations are queued, as we dont want the state change from the first to be misinterpreted as the second one finishing. This manifested itself as a severe issue with HLS playback (especially on android phones) where track switches were causing the buffering updates used to signal startup to be dropped. Bug: 873837 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: I3930bb446f69a2c166419aedd0b5a97d3200ce49 Reviewed-on: https://chromium-review.googlesource.com/1183893 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Ted Meyer <tmathmeyer@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#585123}(cherry picked from commit 742212b82e34205ea0d985cc60e159ddb87ec47d) Reviewed-on: https://chromium-review.googlesource.com/1187165 Cr-Commit-Position: refs/branch-heads/3497@{#789} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/67cb48b1b830e3e1c2a3390bf4f52e627c05cacc/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/67cb48b1b830e3e1c2a3390bf4f52e627c05cacc/media/filters/pipeline_controller.cc [modify] https://crrev.com/67cb48b1b830e3e1c2a3390bf4f52e627c05cacc/media/filters/pipeline_controller.h
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/108d4ef3f8e90fcf40d39bd8496e6ebbb3c87ea6 commit 108d4ef3f8e90fcf40d39bd8496e6ebbb3c87ea6 Author: Dale Curtis <dalecurtis@chromium.org> Date: Thu Aug 23 19:45:17 2018 Don't attempt to enable non-existant placeholder tracks. The media pipeline does not expect to be called about track changes prior to HaveCurrentData. There's also no reason for the element to create disabled video+audio tracks and then tell the renderer to enable those very same non-existant tracks. Just create them as enabled instead. It's possible we can remove even the creation of placeholder tracks, but that's a bigger web facing change, so I don't want to hold up this bug fix on that analysis. BUG= 873837 TEST=new unittest, twitter videos no longer hang. Change-Id: I9863ae4feca29a58140e6dad1532bc8b7a380c3b Reviewed-on: https://chromium-review.googlesource.com/1180347 Commit-Queue: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#585334}(cherry picked from commit ad7d871be92550674b3bdab80352b3c7e3edfdbd) Reviewed-on: https://chromium-review.googlesource.com/1187337 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#790} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/108d4ef3f8e90fcf40d39bd8496e6ebbb3c87ea6/third_party/blink/renderer/core/html/media/html_media_element.cc [modify] https://crrev.com/108d4ef3f8e90fcf40d39bd8496e6ebbb3c87ea6/third_party/blink/renderer/core/html/media/html_media_element.h [modify] https://crrev.com/108d4ef3f8e90fcf40d39bd8496e6ebbb3c87ea6/third_party/blink/renderer/core/html/media/html_media_element_test.cc
,
Aug 23
,
Aug 23
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by dalecur...@chromium.org
, Aug 13Owner: tmathmeyer@chromium.org
Status: Assigned (was: Unconfirmed)