Issue metadata
Sign in to add a comment
|
[Performance] Animation drops to 1FPS on retina screen during image loading
Reported by
konrad.d...@brainly.com,
Apr 20 2017
|
||||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36 Example URL: https://kdzwinel.github.io/fisheye-slow/ Steps to reproduce the problem: 1. Go to https://kdzwinel.github.io/fisheye-slow/ 2. In DevTools turn on network throttling (Regular 2G will do) 3. Reload page 4. Try scrolling while images are loading Repeat the above test four times: A) on a retina screen (tested on macOS, macbook) while images are loading B) on a retina screen when images are loaded (skip #2 and wait for images to load) C) on a non-retina screen while images are loading (tested on same machine - external screen) D) on a non-retina screen when images are loaded What is the expected behavior? What went wrong? Animation drops to <1FPS only in the scenario A) Does it occur on multiple sites: Yes Is it a problem with a plugin? No Did this work before? No Does this work in other browsers? Yes Chrome version: 58.0.3029.81 Channel: stable OS Version: OS X 10.12.4 Flash Version: Performance panel in DevTools shows nothing - see attached image. Even though this animation is particularly badly written (on purpose) it's working (more or less) smoothly in scenarios B), C) and D). Safari has issues in all scenarios but also does especially bad in A). Firefox works smoothly for all scenarios.
,
Apr 21 2017
I can't repro on my MacBook Pro, can you try if it repros on other MacBook Pro you may have around?
,
Apr 21 2017
Unable to reproduce the issue on Mac Retina-10.12.4 and MacBook Air-10.12.4 using chrome stable version 58.0.3029.81 with the steps mentioned in comment#0. Please find the attached screen cast and let us know if anything missed here to reproduce the issue. Thanks.
,
Apr 21 2017
@kojii yes, the difference in animation when images are loading vs when they are loaded is clearly visible on other retina devices @sureshkumari the problem is visible on your video - animation isn't responsive. Please compare the animation while images are loading with animation when all images are loaded. Not sure why FPS counter shows otherwise, but if you use DevTools Timeline/Performance panel you'll get info that only 1FPS is rendered. For clarity I'm attaching video showing scenarios B and A: https://drive.google.com/file/d/0B8kmdXb2YOGHczdzM001SzVJVWs/view?usp=sharing
,
Apr 21 2017
Thank you for providing more feedback. Adding requester "sureshkumari@chromium.org" to the cc list and removing "Needs-Feedback" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 21 2017
Interesting observation: start loading on non-retina screen and then drug window to retina, animation drops to 1fps. But if start on retina and drug to non-retina, animation speed increase to normal
,
Apr 24 2017
Able to reproduce the issue on Mac Retina-10.12.4 using chrome stable version 58.0.3029.81. This is regression issue, broken in M57. Manual Bisect: -------------- Good Build-56.0.2924.99 Bad Build-57.0.2925.0 God and Bad builds are from different milestones, so providing CL through Omahaproxy https://chromium.googlesource.com/chromium/src/+log/56.0.2924.0..57.0.2925.0?pretty=fuller&n=10000 Review-Url: https://codereview.chromium.org/2517463005 caseq@ Kindly take a look and please help us to reassign this issue to a right owner if not with respect to this change. Note:Issue is specific to Mac Retina Thanks.!
,
Apr 24 2017
,
Apr 28 2017
I don't think these bisect results are valid -- I just deleted the experiment that (1) is off by default and (2) in Timeline, that does not run and is not even supposed to be loaded during the described scenario.
,
Apr 28 2017
,
Apr 28 2017
After rebisecting the issue ,below is the manual bisect information Good build-57.0.2931.0-(Revision-434310) Bad build-57.0.2933.0—(Revision-434540) After running bisect script got the below Change log: https://chromium.googlesource.com/chromium/src/+log/14076c954628a8dc4bdefbd080cb80851d43d4ac..387c97d5cf93a15e1b547bf9f96c8c441fe9d035 Not sure about the suspect, hence marking it as Untriaged and not assigning to the owner got from CL. Thanks.
,
Apr 28 2017
Assigning to the Cl owner, looks like above suspect is correct not sure though. If possible can we get this fixed before M60 hits stable.
,
May 9 2017
Hmm... I couldn't reproduce this on Macbook Air (non-retina). I'll try again on Macbook Pro (retina) later (because it's now at home).
,
May 9 2017
sureshkumari@: Let me make sure c#11... did you reproduce this on Windows? What kind of machine/display did you use?
,
May 9 2017
Issue is not reproducing on Windows. Thanks.
,
May 9 2017
sureshkumari@: Thanks! I'm able to reproduce this on Macbook Pro. As c#6, this happens on retina main display, but not on non-retina second display. Interesting... I'll take a closer look tomorrow (in JST).
,
May 10 2017
tzik@ helped me to investigate this (thank you!) and now we may understand the cause. My change removed 0.1 ms delay from PostTask in ScrollAnimatorMac::startScrollbarPaintTimer[1] because new PostTask interface didn't support micro sec delay (type of |delay| was changed from "double" to "long long"). This gives scroll animation tasks more chances to run before task cancellation, and probably task contention happens. I created a patch[2] to re-add 1 ms delay (not 0.1 ms because the current PostTask doesn't support micro sec delay as mentioned above) and confirmed it improved the scroll animation in the demo page. [1] https://codereview.chromium.org/2514733002/ [2] https://codereview.chromium.org/2868783006/
,
May 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/570b67158dabf2d9f10bb12ac68762e973ed63ad commit 570b67158dabf2d9f10bb12ac68762e973ed63ad Author: nhiroki <nhiroki@chromium.org> Date: Thu May 11 05:36:12 2017 Re-add delay to PostTask in StartScrollbarPaintTimer() My previous CL removed 0.1 ms delay from PostTask in ScrollAnimatorMac::startScrollbarPaintTimer[1] because new PostTask interface didn't support micro sec delay (type of |delay| was changed from "double" to "long long"). This seems to give tasks more chances to run before task cancellation and probably contention happens. This CL re-adds 1 ms delay (not 0.1 ms because the current PostTask doesn't support micro sec delay as mentioned above) and confirmed it improved the scroll animation in the demo page (see the issue). [1] https://codereview.chromium.org/2514733002/ BUG= 713669 Review-Url: https://codereview.chromium.org/2868783006 Cr-Commit-Position: refs/heads/master@{#470823} [modify] https://crrev.com/570b67158dabf2d9f10bb12ac68762e973ed63ad/third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm
,
May 11 2017
A fix was landed. I'll keep this issue open for a while to monitor the impact of the change and to decide if we should merge this into the previous milestones.
,
May 16 2017
Tested the issue on Mac-Retina 10.12.4 using chrome dev version #60.0.3100.0 with the steps mentioned in comment #0. Observed that the fix is working as expected. Hence adding TE-Verified labels. Please find the attached screencast for the same. Thanks!!
,
May 22 2017
The NextAction date has arrived: 2017-05-22
,
May 24 2017
MERGE REQUEST: Is it still possible to merge the fix on c#19 into M59? It was verified on the canary as c#21. Thanks! sureshkumari@: Thank you for verifying it :)
,
May 24 2017
,
May 24 2017
This bug requires manual review: We are only 12 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 24 2017
nhiroki - this seems like a fairly quite large change. Can you please confirm if this is a safe merge overall, and if yes, is there enough automated testing coverage for this? What are the implications if we wait until M60?
,
May 26 2017
> Can you please confirm if this is a safe merge overall? The fix affects only Mac. I manually tested it on my Macbook Pro and TE also verified it (see c#21). > Is there enough automated testing coverage for this? I think there are no automated tests that directly check this behavior because this happened under poor/overloaded environments (e.g., 2G network). As for typical scroll animation tests, I haven't received perf-regression reports on them so far. > What are the implications if we wait until M60? Until M60, scroll animation on Mac could slow down in poor/overloaded environments like c#21.
,
May 26 2017
Thanks for confirming. Based on comment 27, approving this merge for M59.
,
May 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b04ee85f34a041db2af572aca65a7ffc36822fe commit 3b04ee85f34a041db2af572aca65a7ffc36822fe Author: Hiroki Nakagawa <nhiroki@chromium.org> Date: Sat May 27 06:04:07 2017 [Merge to M59] Re-add delay to PostTask in StartScrollbarPaintTimer() My previous CL removed 0.1 ms delay from PostTask in ScrollAnimatorMac::startScrollbarPaintTimer[1] because new PostTask interface didn't support micro sec delay (type of |delay| was changed from "double" to "long long"). This seems to give tasks more chances to run before task cancellation and probably contention happens. This CL re-adds 1 ms delay (not 0.1 ms because the current PostTask doesn't support micro sec delay as mentioned above) and confirmed it improved the scroll animation in the demo page (see the issue). [1] https://codereview.chromium.org/2514733002/ BUG= 713669 Review-Url: https://codereview.chromium.org/2868783006 Cr-Original-Commit-Position: refs/heads/master@{#470823} Review-Url: https://codereview.chromium.org/2907013002 . Cr-Commit-Position: refs/branch-heads/3071@{#713} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [modify] https://crrev.com/3b04ee85f34a041db2af572aca65a7ffc36822fe/third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm
,
May 31 2017
Tested the issue on Mac-Retina 10.12.4 using latest Beta Chrome version #59.0.3071.82 with the steps mentioned in comment #0. Observed the fix is working as expected. Hence adding TE-Verified labels. Please find the below screen cast. Thanks!! |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by ligim...@chromium.org
, Apr 20 2017Labels: -Pri-2 Needs-Triage-M58 Needs-Bisect Prestable-58.0.3029.81 Pri-1