Issue metadata
Sign in to add a comment
|
Patch "Prevents compositor frames from being sent when SurfaceLayer is not visible" caused video regression |
||||||||||||||||||||||
Issue descriptionOS : windows 10, 10.0.17134.1 Platform: Intel i7-8700k with HD630 GPU and Intel Xeon E5-2699 with NVIDIA GeForce GTX 1060 GPU Steps to reproduce : 1. build chromium after revision 575833; 2. open https://threejs.org/examples/#webvr_video expected: video plays smoothly what happens video plays with significant frame lost. With some bisect work, I find patch Reland "Prevents compositor frames from being sent when SurfaceLayer is not visible." (https://chromium-review.googlesource.com/c/chromium/src/+/1140458) cause this regression. Details : 1. I downloaded chromium through bisect tools(revision 579310, 575839), and they can reproduce this issue on Intel GPU and Nvidia GPU. I also built chromium myself without the patch and downloaded chromium through bisect tools(revision 575822), and they all work fine on Intel GPU and Nvidia GPU.
,
Jul 31
,
Jul 31
Raising to P1. Might be related to Issue 868008 so blocking on that one. Making this R-B-S so it is investigated. The CLs like https://chromium-review.googlesource.com/1140458 should have had an associated bug ID so there was history to look at.
,
Jul 31
i can repro this locally. i'll add some instrumentation to try to figure out where the difference is.
,
Jul 31
when we're using surface layer, VideoFrameCompositor always has a VideoFrameProvider::Client, even for off-screen video. this causes it to avoid updating the current frame as often, believing that the sink will do it.
,
Jul 31
,
Aug 1
,
Aug 1
,
Aug 1
i'm working on a patch for this. the idea is simple, but i'm trying to fix a fix that's not too flaky nor changes the current VideoFrameProvider API too much.
,
Aug 7
Friendly ping! Could you please provide any update on this issue as it has been marked as a stable blocker. Thank You!
,
Aug 7
the change is still in review: https://chromium-review.googlesource.com/c/chromium/src/+/1159274
,
Aug 7
M69 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Aug 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f14f5166959cbae16d85221514cd6c8a973455d8 commit f14f5166959cbae16d85221514cd6c8a973455d8 Author: liberato@chromium.org <liberato@chromium.org> Date: Wed Aug 08 17:09:26 2018 Allow VideoFrameProvider::Client to avoid driving frame updates. Previously, VideoFrameCompositor assumed that, if it had a client, that the client would drive VideoFrame updates based on the vsync clock. If an external consumer (e.g., WebGL) painted the frame, VFC would still not update the current frame, since the client would take care of it. VideoFrameSubmitter is a client that sometimes elides frame updates, if it believes that no consumer is present (e.g., if the corresponding SurfaceLayer is not in the layer tree, or is not visible). However, this would break external consumers, since they would no longer get current VideoFrames. This CL allows a VideoFrameProvider::Client to signal that it isn't doing frame updates. In this case, VFC will still try to move the frame forward to match the wall clock, so that external consumers get current frames. Additionally, this CL causes SurfaceLayerImpl to signal that it's no longer visible upon destruction. This catches cases where a video element is removed from the DOM. Bug: 869277 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ia53a349af4add6d4792ef16e0306aa7c139ab70c Reviewed-on: https://chromium-review.googlesource.com/1159274 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Commit-Queue: Frank Liberato <liberato@chromium.org> Cr-Commit-Position: refs/heads/master@{#581605} [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/cc/layers/surface_layer.h [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/cc/layers/surface_layer_impl.cc [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/cc/layers/surface_layer_impl.h [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/cc/layers/video_frame_provider.h [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/cc/layers/video_frame_provider_client_impl.cc [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/cc/layers/video_frame_provider_client_impl.h [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/content/renderer/media/stream/webmediaplayer_ms_unittest.cc [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/media/blink/video_frame_compositor.cc [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/media/blink/video_frame_compositor.h [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/media/blink/video_frame_compositor_unittest.cc [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc [modify] https://crrev.com/f14f5166959cbae16d85221514cd6c8a973455d8/third_party/blink/renderer/platform/graphics/video_frame_submitter.h
,
Aug 8
,
Aug 8
CL listed at #13 is not yet made it to canary. Pls update the bug with canary result tomorrow. Also pls justify the merge, how safe it is?
,
Aug 8
should be pretty safe, in that it just matches the SurfacesForVideo behavior to the previous VideoLayer behavior. will verify in canary tomorrow and see.
,
Aug 8
OK, thank you liberato@. I will wait for canary result.
,
Aug 9
The NextAction date has arrived: 2018-08-09
,
Aug 9
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review 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 9
chrome dev on windows (3514) had the bug, but the latest canary (3517) does not.
,
Aug 9
Approving merge to M69 branch 3497 based on comment #16 and #20.
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a commit 7dfc1e59cd2b548a615ecdc8710df9644f56ba7a Author: liberato@chromium.org <liberato@chromium.org> Date: Fri Aug 10 14:59:06 2018 Allow VideoFrameProvider::Client to avoid driving frame updates. Previously, VideoFrameCompositor assumed that, if it had a client, that the client would drive VideoFrame updates based on the vsync clock. If an external consumer (e.g., WebGL) painted the frame, VFC would still not update the current frame, since the client would take care of it. VideoFrameSubmitter is a client that sometimes elides frame updates, if it believes that no consumer is present (e.g., if the corresponding SurfaceLayer is not in the layer tree, or is not visible). However, this would break external consumers, since they would no longer get current VideoFrames. This CL allows a VideoFrameProvider::Client to signal that it isn't doing frame updates. In this case, VFC will still try to move the frame forward to match the wall clock, so that external consumers get current frames. Additionally, this CL causes SurfaceLayerImpl to signal that it's no longer visible upon destruction. This catches cases where a video element is removed from the DOM. Bug: 869277 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ia53a349af4add6d4792ef16e0306aa7c139ab70c Reviewed-on: https://chromium-review.googlesource.com/1159274 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Commit-Queue: Frank Liberato <liberato@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#581605}(cherry picked from commit f14f5166959cbae16d85221514cd6c8a973455d8) Reviewed-on: https://chromium-review.googlesource.com/1169801 Reviewed-by: Frank Liberato <liberato@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#538} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/cc/layers/surface_layer.h [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/cc/layers/surface_layer_impl.cc [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/cc/layers/surface_layer_impl.h [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/cc/layers/video_frame_provider.h [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/cc/layers/video_frame_provider_client_impl.cc [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/cc/layers/video_frame_provider_client_impl.h [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/content/renderer/media/stream/webmediaplayer_ms_unittest.cc [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/media/blink/video_frame_compositor.cc [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/media/blink/video_frame_compositor.h [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/media/blink/video_frame_compositor_unittest.cc [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/third_party/blink/renderer/platform/graphics/video_frame_submitter.cc [modify] https://crrev.com/7dfc1e59cd2b548a615ecdc8710df9644f56ba7a/third_party/blink/renderer/platform/graphics/video_frame_submitter.h
,
Aug 10
Thank you Frank! :)
,
Aug 10
,
Aug 10
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by shaobo....@intel.com
, Jul 31