Issue metadata
Sign in to add a comment
|
Regression:Flickering is observed on clicking on 'no thanks' in extension overlay of 'Screencastify'
Reported by
shruti.j...@etouch.net,
Feb 14 2018
|
||||||||||||||||||||||
Issue descriptionChrome Version: 65.0.3325.73 (Official Build) (32-bit) Revision a20ac61a553221683d2f152384ea3a3d80e4c1e9-refs/branch-heads/3325@{#457}(32/64-bit) OS: Windows (7,8,8.1,10) Test URL:https://chrome.google.com/webstore/detail/screencastify-screen-vide/mmeijimgabbpbgpdklnllpncmdofkcpn?utm_source=chrome-ntp-icon Steps to reproduce: (1) Launch chrome and navigate to chrome://apps (2) Download Extension 'Screencastify - Screen Video Recorder'from above link. (3) Click on extension and click on 'no thanks' and observe Actual Result:Flickering is observed on clicking on 'no thanks' in extension overlay. Expected Result:Flickering should not be seen on clicking on 'no thanks'in extension overlay. This is a regression issue broken in ‘M-65’ and using per-revision bisect providing the bisect results, Good Build:65.0.3317.0(Revision:528120) Bad Build:65.0.3318.0(Revision:528541) You are probably looking for a change made after 528476 (known good), but no later than 528477 (first known bad). CHANGELOG URL: The script might not always return single CL as suspect as some perf builds might get missing due to failure. https://chromium.googlesource.com/chromium/src/+log/2140d23a8add3f894fd9c94f5916e92974feb435..966d29e791e4811124037ae509754c4277391fd1 Suspect:https://chromium.googlesource.com/chromium/src/+/966d29e791e4811124037ae509754c4277391fd1 @fsamuel: Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner. Note: 1.Issue is also reproducible on Dev #66.0.3343.3 and Canary #66.0.3346.0 Channels. 2.Issue is Win OS specific. Thank You!
,
Feb 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1ee6fb5c206f8f63176196bfc31aa40721b734c7 commit 1ee6fb5c206f8f63176196bfc31aa40721b734c7 Author: Fady Samuel <fsamuel@chromium.org> Date: Fri Feb 16 03:27:46 2018 Surface synchronization: Throttle auto-resize renderer side This is a short term fix until child-allocated LocalSurfaceIDs are implemented for auto-resize. Prior to surface synchronization, child renderers would allocate LocalSurfaceIds for auto-resize. When the renderer receives a DidReceiveCompositorFrameAck message then it knows the compositor has received the frame, and informs the browser process which then resizes the associated aura window to match the frame generated by the child. This ensured that the aura window would only be resized to a size that had a surface. With surface synchronization, every auto-resize resulted in a request for a LocalSurfaceId from the parent. The parent would immediately resize the aura window and embed the newly allocated LocalSurfaceId and then tell the child about it. In the meantime, the child may auto-resize again. Surface synchronization would block the parent on that LocalSurfaceId that never resolves until the child makes another request. After the deadline passes, the user sees flicker. This CL does not address the root cause of the problem (allocating IDs for surfaces that are never created), but it does significantly reduce the problem. Instead of immediately informing the browser when an auto-resize happens in the renderer. The ACK is posted as a task on the renderer's queue. The renderer may auto-resize multiple times until that task is handled, and so we effectively coalesce resize requests before making a request to the browser. This significantly reduces flicker. This is not the final state of auto-resize, though. In the future, the renderer will immediately allocate a LocalSurfaceId and submit a CompositorFrame, and then tell the browser about it to embed it. Occasionally, there will be a conflict between browser and renderer changes, but this should be very rare. Bug: 812127 , 811181 , 672962 Change-Id: I08839c4b498b68ba06bb5d818337738e191cf9be Reviewed-on: https://chromium-review.googlesource.com/920341 Commit-Queue: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Commit-Position: refs/heads/master@{#537173} [modify] https://crrev.com/1ee6fb5c206f8f63176196bfc31aa40721b734c7/content/renderer/render_view_impl.cc [modify] https://crrev.com/1ee6fb5c206f8f63176196bfc31aa40721b734c7/content/renderer/render_widget.cc [modify] https://crrev.com/1ee6fb5c206f8f63176196bfc31aa40721b734c7/content/renderer/render_widget.h
,
Feb 18 2018
,
Feb 18 2018
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 19 2018
Before we approve merge to M65, could you pls confirm followings? Is this critical to merge to M65? Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge? Any other imp details to justify the merge. Please note M65 is VERY close to Stable promotion so merge bar is very high. Thank you.
,
Feb 19 2018
1. 811181 lists this as a stable blocker. This is a UX regression. 2. There's definitely a bunch of test coverage to verify this doesn't regress anything. It seems this CL will hit canary on 66.0.3350.0. This doesn't look like an active canary yet. 3. This is a fairly small change and so the risks are fairly low.
,
Feb 19 2018
Thank you fsamuel@. This cl should be in 66.0.3349.0 or higher. Current live canary is 66.0.3350.0. Pls update bug with canary result.
,
Feb 19 2018
Strange, my local canary build was stuck at 3347.1 ASAN. I uninstalled and reinstalled Chrome canary. I can verify that the bug is gone. Thanks!
,
Feb 19 2018
Approving merge to M65 branch 3325 based on comments #6, #8 and #9. Please merge ASAP. Thank you.
,
Feb 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8769aa4fe6812af29b772a6d7f456ab15fe3cd1 commit e8769aa4fe6812af29b772a6d7f456ab15fe3cd1 Author: Fady Samuel <fsamuel@chromium.org> Date: Mon Feb 19 20:18:12 2018 Surface synchronization: Throttle auto-resize renderer side This is a short term fix until child-allocated LocalSurfaceIDs are implemented for auto-resize. Prior to surface synchronization, child renderers would allocate LocalSurfaceIds for auto-resize. When the renderer receives a DidReceiveCompositorFrameAck message then it knows the compositor has received the frame, and informs the browser process which then resizes the associated aura window to match the frame generated by the child. This ensured that the aura window would only be resized to a size that had a surface. With surface synchronization, every auto-resize resulted in a request for a LocalSurfaceId from the parent. The parent would immediately resize the aura window and embed the newly allocated LocalSurfaceId and then tell the child about it. In the meantime, the child may auto-resize again. Surface synchronization would block the parent on that LocalSurfaceId that never resolves until the child makes another request. After the deadline passes, the user sees flicker. This CL does not address the root cause of the problem (allocating IDs for surfaces that are never created), but it does significantly reduce the problem. Instead of immediately informing the browser when an auto-resize happens in the renderer. The ACK is posted as a task on the renderer's queue. The renderer may auto-resize multiple times until that task is handled, and so we effectively coalesce resize requests before making a request to the browser. This significantly reduces flicker. This is not the final state of auto-resize, though. In the future, the renderer will immediately allocate a LocalSurfaceId and submit a CompositorFrame, and then tell the browser about it to embed it. Occasionally, there will be a conflict between browser and renderer changes, but this should be very rare. TBR=fsamuel@chromium.org (cherry picked from commit 1ee6fb5c206f8f63176196bfc31aa40721b734c7) Bug: 812127 , 811181 , 672962 Change-Id: I08839c4b498b68ba06bb5d818337738e191cf9be Reviewed-on: https://chromium-review.googlesource.com/920341 Commit-Queue: Fady Samuel <fsamuel@chromium.org> Reviewed-by: Antoine Labour <piman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#537173} Reviewed-on: https://chromium-review.googlesource.com/925542 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Cr-Commit-Position: refs/branch-heads/3325@{#500} Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369} [modify] https://crrev.com/e8769aa4fe6812af29b772a6d7f456ab15fe3cd1/content/renderer/render_view_impl.cc [modify] https://crrev.com/e8769aa4fe6812af29b772a6d7f456ab15fe3cd1/content/renderer/render_widget.cc [modify] https://crrev.com/e8769aa4fe6812af29b772a6d7f456ab15fe3cd1/content/renderer/render_widget.h
,
Feb 19 2018
The fix has been merged back into M65. Marking bug as FIXED.
,
Feb 21 2018
#13: I think I'm confused about the issue you're referring to. I did see a little bit of bouncing clicking the menu button but I see a bit of bouncing in the original Expected video too (and when trying things out locally on M64 too). I don't believe this is a regression? I agree this behavior isn't ideal and it will be improved in M66/M67. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by shruti.j...@etouch.net
, Feb 14 2018Summary: Regression:Flickering is observed on clicking on 'no thanks' in extension overlay of 'Screencastify' (was: Flickering is observed on clicking on 'no thanks' in extension overlay of 'Screencastify')