FrameSinkVideoCapturer doesn't capture all updates from child surfaces |
||||||||||||||||||
Issue descriptionWhat steps will reproduce the problem? (1) Build ToT with attached patch. (2) Install chrome/common/extensions/docs/examples/desktopChooser extension. (3) Launch (from chrome://extensions) and start capture of the browser window. (4) In the browser window being captured, play a video (or anything that continuously changes). What is the expected result? Continuous capture of updated tab content in result video. What happens instead? Capture freezes unless mouse cursor is continuously moved over the browser tab strip. Seems that the tab strip UI causes browser window composites that then allow capture of the updated tab content (which is in a child frame sink).
,
Mar 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fab9f78151c81b9243405e6b8b815234a430f627 commit fab9f78151c81b9243405e6b8b815234a430f627 Author: Yuri Wiitala <miu@chromium.org> Date: Tue Mar 20 22:16:38 2018 FrameSinkVideoCapture: BeginFrameArgs→DisplayScheduler timestamps In order to support capture of damage caused by child frame sinks, this change plumbs-through timestamps from the DisplayScheduler, through the SurfaceAggregator, and ultimately to CompostiorFrameSinkSupport and the FrameSinkVideoCapturer; to provide "expected presentation" timestamps. With this, all the old infrastructure for processing BeginFrameArgs and caching their timestamps is removed. Bug: 809867 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel Change-Id: I81253b94800f31fe8f4379395bec2fda119ebf9a Reviewed-on: https://chromium-review.googlesource.com/967601 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Eric Seckler <eseckler@chromium.org> Reviewed-by: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Saman Sami <samans@chromium.org> Cr-Commit-Position: refs/heads/master@{#544554} [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/display.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/display_scheduler.h [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/surface_aggregator.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/surface_aggregator.h [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/surface_aggregator_perftest.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/surface_aggregator_pixeltest.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/display/surface_aggregator_unittest.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/compositor_frame_sink_support.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/compositor_frame_sink_support.h [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/video_capture/capturable_frame_sink.h [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.h [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl_unittest.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/frame_sinks/video_detector_unittest.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/surfaces/surface.cc [modify] https://crrev.com/fab9f78151c81b9243405e6b8b815234a430f627/components/viz/service/surfaces/surface.h
,
Mar 20 2018
,
Apr 26 2018
Issue 836065 has been merged into this issue.
,
Apr 26 2018
Given that two users are seeing bugs for two separate major web sites in M66, I'm requesting a merge to stable. See bug 835945 and bug 836065 for use cases. FWIW, I would have requested the merge long ago, but I didn't realize the impact severity of the bug at the time I fixed it. It seems I've grossly underestimated that.
,
Apr 26 2018
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 26 2018
,
Apr 27 2018
What is exactly the issue? How prevalent is this issue? M66 is already being promoted to Stable.
,
Apr 27 2018
,
Apr 27 2018
The issue is that tab screen capture is broken for all web sites with cross-origin iframes. The bug only manifests when cross-site isolation is turned on, which seems to be the default now. So far, I've directly received reports that this has broken the screen capture of Google Drive video playback and some extensions (such as useloom.com). So, I expect the problem is much more wide-spread than I originally anticipated and would like to merge the fix for the next M66 stable update.
,
Apr 27 2018
Also, it's worth noting that everyone's confirmed the bug is gone in M67 branch, which suggests risk is low (bug is fixed and features are working).
,
Apr 28 2018
+ abdulsyed@, creis@ and nasko@, PTAL comment #10. Seems to be related to site isolation.
,
Apr 28 2018
,
Apr 28 2018
Yes, comment 10 does indicate this is a problem when Site Isolation is turned on. There's a 5% trial of Site Isolation on M66, but some enterprises also have it enabled by default in M66, which may explain some of the reports. Given the bake time r544554 has had on M67, the confirmation that it's working in M67, and the prominent impact to users who have Site Isolation enabled, it might make sense to merge to M66 (assuming there's a respin coming for other reasons).
,
Apr 30 2018
So far there is no M66 respin planned. Are there any client side workarounds that are available until M67? 67 is tentatively planned for stable on May 29th. Even though the fix has been baking in M67 for over a month, is it a safe and clean merge to 66?
,
Apr 30 2018
,
Apr 30 2018
Unfortunately, there aren't any client-side workarounds, except for website authors to re-design around not using iframes (I wouldn't expect this to be feasible). I took a look, and based on code changes on trunk between the branch point and the time of the fix, the fix should merge to M66 cleanly. Even if we decide not to respin 66 just for this fix, it might be good to do the merge so that it can go out alongside other, more severe fixes if there are ever any.
,
May 2 2018
+1 to miu@ suggestion to merge this to the 66 branch, but not force a respin on it's own and wait until some other required respin (if any) goes out. If no other respin happens then our guidance will be to direct users to Chrome 67 beta until it becomes stable.
,
May 2 2018
Approving merge for M66. branch:3359 - please note that no respin planned.
,
May 2 2018
#19 & 18 - if I understand this correctly, this won't get released in M66 unless there's another critical bug that goes out?
,
May 2 2018
#20 - yes
,
May 2 2018
May I ask what the hold off is on the respin? Genuinely asking so I understand the Chrome merge-back process. This is the first time we're actually getting a fix merged back, but it seems ineffective if the code potentially never gets released. Is it a risk mitigation strategy?
,
May 3 2018
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191 commit 7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191 Author: Adam Parker <amp@chromium.org> Date: Thu May 03 17:18:35 2018 Merge 66: FrameSinkVideoCapture: BeginFrameArgs→DisplayScheduler timestamps In order to support capture of damage caused by child frame sinks, this change plumbs-through timestamps from the DisplayScheduler, through the SurfaceAggregator, and ultimately to CompostiorFrameSinkSupport and the FrameSinkVideoCapturer; to provide "expected presentation" timestamps. With this, all the old infrastructure for processing BeginFrameArgs and caching their timestamps is removed. TBR=miu@chromium.org (cherry picked from commit fab9f78151c81b9243405e6b8b815234a430f627) Bug: 809867 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel Change-Id: I81253b94800f31fe8f4379395bec2fda119ebf9a Reviewed-on: https://chromium-review.googlesource.com/967601 Commit-Queue: Yuri Wiitala <miu@chromium.org> Reviewed-by: Eric Seckler <eseckler@chromium.org> Reviewed-by: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Saman Sami <samans@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#544554} Reviewed-on: https://chromium-review.googlesource.com/1042537 Reviewed-by: Adam Parker <amp@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#795} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/display.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/display_scheduler.h [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/surface_aggregator.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/surface_aggregator.h [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/surface_aggregator_perftest.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/surface_aggregator_pixeltest.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/display/surface_aggregator_unittest.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/compositor_frame_sink_support.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/compositor_frame_sink_support.h [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/video_capture/capturable_frame_sink.h [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl.h [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/video_capture/frame_sink_video_capturer_impl_unittest.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/frame_sinks/video_detector_unittest.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/surfaces/surface.cc [modify] https://crrev.com/7292bb3e6a1e6cd89d41aa5f52ecdbf030ba4191/components/viz/service/surfaces/surface.h
,
May 4 2018
,
May 14 2018
Looks like there was a respin for Chrome 66 with version 66.0.3359.170 which includes this fix. I just tested now and it works correctly for videos viewed on Google drive. |
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by m...@chromium.org
, Mar 15 2018