Invalidations without PictureLayer::Update are broken
Reported by
tabbw...@gmail.com,
Mar 3 2016
|
||||||||||||||||||||||||
Issue descriptionChrome Version : 51.0.2665.0 URLs (if applicable) : https://codepen.io/anon/pen/ONVOMG Other browsers tested: Chrome 48.0.2564.116: OK What steps will reproduce the problem? 1. Open the codepen provided above 2. Click the "Change Kitten" button What is the expected result? The orange kitten should disappear and then be replaced by several gray kittens What happens instead? The orange kitten disappears and reappears Please provide any additional information below. Attach a screenshot if possible. The codepen provided is manipulating the background image of an element while it is not visible on the screen. If that element also happens to have transform: translate3d(0,0,0), then the new background image is never displayed even though it looks correct if you inspect the DOM (screenshots attached).
,
Mar 7 2016
I can confirm the background image does not look right (it flashes for me) on pressing the "Change Kittens" button. Chrome Canary Version 51.0.2670.0 canary (64-bit) Win.
,
Mar 7 2016
I just upgraded Chrome today from 48.0.2564.116 to 49.0.2623.75, and in Chrome 49.0.2623.75 I see the problem (orange kitten disappears and reappears, instead of being replaced with gray kittens). It appears like this issue started somewhere between those two versions.
,
Mar 9 2016
Able to reproduce on Windows 7, Ubuntu 14.04 and Mac OS 10.11.3 using chrome stable M49 - 49.0.2623.87. Bisect Information: =================== Good build: 49.0.2585.0 Bad Build : 49.0.2587.0 Change Log URL: https://chromium.googlesource.com/chromium/src/+log/f37301d6d5018a0ee473ea3f89f8216d95072db4..e1858d8614321b276e3588a514262f1553a3c272 Unable to find the actual suspect from the above CL. Could anyone please look in to this and help us in assigning it to the right owner. Thanks!
,
Mar 9 2016
Looking for a change to Blink painting of images, and looking over the regression range, seem we have mostly chromium changes except for one possible paint change https://chromium.googlesource.com/chromium/src/+/bb9af00d48a32bd944985827bd2624ac9fe301cd so only punting here, chrishtr@ might advise us.
,
Mar 10 2016
I confirmed that my CL did in fact break this.
,
Mar 10 2016
Adding a single Clear() call after passing invalidation rects for raster seems to fix the bug. I think this might be a memory corruption issue in Skia.
,
Mar 10 2016
> I think this might be a memory corruption issue in Skia. Interesting. If the fix CL adds a layout test, then the MSan bots would eventually detect and report any memory corruption issue.
,
Mar 11 2016
,
Mar 11 2016
,
Mar 12 2016
There are a couple of things at play here: - There is a PictureLayer::Update function that updates the display list recording source (which becomes the raster source). - There is a PictureLayer::PushPropertiesTo that uses the recording source to make a raster source and at the same time consume the invalidation (take and clear) What happens in this case is that when we have an opacity 0 layer, we skip the Update call, but we still process the PushPropertiesTo call. This means that we have a stale raster source, but we consume the invalidation. When opacity becomes 1 again, we don't get a new invalidation so we early out of the Update call and don't update the raster source. Hence, we get wrong cats. The reason that the code before the patch in question worked is that it would only expose the invalidation in the Update function. That is to say, we could call PushPropertiesTo as many times as we wanted to, but the invalidation would always remain the same until Update. Once we call Update, then the invalidation would be published for the next PushPropertiesTo to consume. The solution here is either to re-add the code that publishes invalidation on Update, or always call Update even for opacity 0 layers. The latter has some cost associated with it, but I'm not sure how frequently we have complex subtrees of 0 opacity layer... +some peeps for other opinions.
,
Mar 12 2016
I think we should consider turning off the optimizations that avoid calling Layer::Update in various cases that are represented in cc::FindLayersThatNeedUpdates: basically always call it for layers that draw content. opacity:0 is one example. Others are drawing the back of a backface-visibility:hidden layer, layer with a non-invertible transform, and a few other cases. In the current code, the call to Layer::Update saves a non-trivial amount of time if the layer has an invalidation on it, or its visible rect has changed. In that case, we do some geometry, and also copy the display list over from the Blink output (but it does *not* execute paint to re-generate that display list). If those optimizations are present to avoid expensive paint, then that will need to be re-implemented in Blink instead.
,
Mar 14 2016
Removing this skipping sounds like a good idea, but first we should find out what impact this will have on the ui compositor. The ui compositor has calls to Layer::SetHideLayerAndSubtree to hide subtrees (by causing them to have effective opacity 0), and actual painting happens during the call to Layer::Update, so it's conceivable that subtree skipping is saving a lot.
,
Mar 14 2016
I'll look into it. Regarding comment 13: it appears the cost of painting a UI layer can be avoided by early-outing of its paint code if visible_ is false, will try that.
,
Mar 14 2016
What's wrong with just re-adding the step to swap invalidation during update? This seems like the easiest and least risk solution here. It seems like this is also most in line with what Blink will end up doing. If it does skip updating a subtree, it will need to generate that invalidation on a future frame.
,
Mar 17 2016
,
Mar 17 2016
,
Mar 17 2016
,
Mar 17 2016
Issue 593889 has been merged into this issue.
,
Mar 18 2016
Another one that we may have to merge into 49. :( Chrome 49 has too many bugs.
,
Mar 18 2016
Does this affect Android as well? Why is it a Pri-2 if it needs to be merged to a post-stable release? +sshruthi@ for desktop. Re: too many bugs, get ready for a postmortem once we're out of the woods.
,
Mar 18 2016
,
Mar 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c41aca7b408e13fdee88a3b39db1a93813bd9fc1 commit c41aca7b408e13fdee88a3b39db1a93813bd9fc1 Author: chrishtr <chrishtr@chromium.org> Date: Fri Mar 18 15:44:29 2016 Store recording invalidations in DisplayListRecordingSource, save them via Update. This fixes the referenced bug, as well as cleaning up the interaction between PictureLayer and DisplayListRecordingSource a little bit. This way both the invalidation and picture come from DisplayListRecordingSource, rather than one coming from PictureLayer and the other from DisplayListRecordingSource, with the latter modifying the invalidation during Update. Also renamed LayerImpl::GetInvalidationRegion() to LayerImpl::GetInvalidationRegionForDebugging() to make clear it's only used for that purpose. BUG= 591561 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review URL: https://codereview.chromium.org/1812733003 Cr-Commit-Position: refs/heads/master@{#381977} [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/base/invalidation_region.h [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/debug/debug_rect_history.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/layer_impl.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/layer_impl.h [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/picture_layer.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/picture_layer.h [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/picture_layer_impl.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/picture_layer_impl.h [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/picture_layer_impl_unittest.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/layers/picture_layer_unittest.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/playback/display_list_recording_source.cc [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/playback/display_list_recording_source.h [modify] https://crrev.com/c41aca7b408e13fdee88a3b39db1a93813bd9fc1/cc/test/fake_display_list_recording_source.h
,
Mar 18 2016
,
Mar 18 2016
,
Mar 18 2016
,
Mar 18 2016
Affects Android as well. Merge approved for M49 branch 2623 and M50 branch 2661 so that we can get a build moving for early qualification next week. Please keep a very close eye on canary crashes, UMA metrics, etc. on Monday in case something goes wrong, we don't have a lot of runway here.
,
Mar 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58cef5be289f5004932682c17694aaa7724bae70 commit 58cef5be289f5004932682c17694aaa7724bae70 Author: Chris Harrelson <chrishtr@chromium.org> Date: Fri Mar 18 20:45:47 2016 Store recording invalidations in DisplayListRecordingSource, save them via Update. This fixes the referenced bug, as well as cleaning up the interaction between PictureLayer and DisplayListRecordingSource a little bit. This way both the invalidation and picture come from DisplayListRecordingSource, rather than one coming from PictureLayer and the other from DisplayListRecordingSource, with the latter modifying the invalidation during Update. Also renamed LayerImpl::GetInvalidationRegion() to LayerImpl::GetInvalidationRegionForDebugging() to make clear it's only used for that purpose. BUG= 591561 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review URL: https://codereview.chromium.org/1812733003 Cr-Commit-Position: refs/heads/master@{#381977} (cherry picked from commit c41aca7b408e13fdee88a3b39db1a93813bd9fc1) Review URL: https://codereview.chromium.org/1811113004 . Cr-Commit-Position: refs/branch-heads/2623@{#637} Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907} [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/base/invalidation_region.h [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/debug/debug_rect_history.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/layer_impl.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/layer_impl.h [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/picture_layer.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/picture_layer.h [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/picture_layer_impl.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/picture_layer_impl.h [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/picture_layer_impl_unittest.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/layers/picture_layer_unittest.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/playback/display_list_recording_source.cc [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/playback/display_list_recording_source.h [modify] https://crrev.com/58cef5be289f5004932682c17694aaa7724bae70/cc/test/fake_display_list_recording_source.h
,
Mar 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b commit 0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b Author: Chris Harrelson <chrishtr@chromium.org> Date: Fri Mar 18 20:51:34 2016 Store recording invalidations in DisplayListRecordingSource, save them via Update. This fixes the referenced bug, as well as cleaning up the interaction between PictureLayer and DisplayListRecordingSource a little bit. This way both the invalidation and picture come from DisplayListRecordingSource, rather than one coming from PictureLayer and the other from DisplayListRecordingSource, with the latter modifying the invalidation during Update. Also renamed LayerImpl::GetInvalidationRegion() to LayerImpl::GetInvalidationRegionForDebugging() to make clear it's only used for that purpose. BUG= 591561 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review URL: https://codereview.chromium.org/1812733003 Cr-Commit-Position: refs/heads/master@{#381977} (cherry picked from commit c41aca7b408e13fdee88a3b39db1a93813bd9fc1) Review URL: https://codereview.chromium.org/1812733004 . Cr-Commit-Position: refs/branch-heads/2661@{#285} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/base/invalidation_region.h [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/layer_impl.cc [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/layer_impl.h [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/picture_layer.cc [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/picture_layer.h [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/picture_layer_impl.cc [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/picture_layer_impl.h [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/picture_layer_impl_unittest.cc [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/layers/picture_layer_unittest.cc [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/playback/display_list_recording_source.cc [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/playback/display_list_recording_source.h [modify] https://crrev.com/0b7080ff51fc8f6c7bc0f2d6d60c158ede2fdf9b/cc/test/fake_display_list_recording_source.h
,
Mar 18 2016
,
Mar 21 2016
Verified this fix on 49.0.2623.105 Samsung Galaxy S7 (SM-G930FD)/ MMB29K and Samsung Tab S / LRX22G.
,
Mar 22 2016
Verified this fix on 51.0.2687.0 on Nexus 6/ MOB29Z
,
Mar 23 2016
Verified this fix on 50.0.2661.49 on Samsung Galaxy S5 (G900H) / LRX21T
,
Mar 25 2016
Issue 595835 has been merged into this issue. |
||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||
Comment 1 by ashej...@chromium.org
, Mar 3 2016