Paint animated layers immediately to avoid animation hiccups.
Created attachment 156169 [details] Patch
Created attachment 156212 [details] tweaks
Created attachment 156214 [details] tweak
Comment on attachment 156214 [details] tweak View in context: https://bugs.webkit.org/attachment.cgi?id=156214&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:-387 > - // any single time through the function. Setting the persistent 'updated' flag here seems like a bug to me, since this loop can exit the function early (making any previously set 'updated' flags incorrect). The 'updatedTempFlag' is set unconditionally and only referenced once below, so it's value doesn't matter before or after this function.
View in context: https://bugs.webkit.org/attachment.cgi?id=156212&action=review Just so I can understand this change, let me say what it looks like this patch is doing. Previous special case behavior: * Entirely hidden but small animated layers with <= 9 tiles used to be entirely painted via idle painting * Entirely hidden but large animated layers had their outer tiles painted New special case behavior: * Hidden or unhidden animated layers that smaller than the viewport(ish) get entirely painted * If the animated layer is partially visible, the "entire painting" pass preempts "normal" painting of the partially visible part of the layer. I'm fine with tweaking this special case logic to be better about animations in the short term. However, I'd be happier if I had a better sense of the longer-term plan of how to handle prioritization of animated content, especially if it involved less special case logic in TiledLayerChromium. > Source/WebCore/ChangeLog:3 > + Paint animated layers immediately to avoid animation hiccups. [chromium] > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:81 > + , updatedTempFlag(false) This should go in a different patch. "Tweaks," eh? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:525 > + IntSize viewportSize = layer->layerTreeHost() ? layer->layerTreeHost()->viewportSize() : IntSize(); deviceViewportSize? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:528 > + return contentRect.width() <= viewportSize.width() + 64 > + && contentRect.height() <= viewportSize.height() + 64; 64?
(In reply to comment #5) > New special case behavior: > * Hidden or unhidden animated layers that smaller than the viewport(ish) get entirely painted > * If the animated layer is partially visible, the "entire painting" pass preempts "normal" painting of the partially visible part of the layer. > Yep that's the idea. > I'm fine with tweaking this special case logic to be better about animations in the short term. However, I'd be happier if I had a better sense of the longer-term plan of how to handle prioritization of animated content, especially if it involved less special case logic in TiledLayerChromium. My thinking is that the long term beneficial behavior we want to keep is that animated layers get entirely painted if at all possible. However, it might be possible for that behavior to just fall out naturally with some added heuristics about distance and animation direction. - Distance: don't paint all animated layers, but rather 'close' animated layers - Direction: remove checks for 'small' and 'animated' and replace the animation pass with a pass that tries to paint all tiles that 'will be visible soon', before reverting to visible tiles. > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:81 > > + , updatedTempFlag(false) > > This should go in a different patch. "Tweaks," eh? > Sorry, I assumed you would only read the later patch since it was quick. I should have put a better description (to insure it doesn't get overlooked). I'll put this loop cleanup as a refactoring patch before this one. > deviceViewportSize? > I'll change it, but can you describe the difference here? Is this a unit change or is there actually different viewports? > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:528 > > + return contentRect.width() <= viewportSize.width() + 64 > > + && contentRect.height() <= viewportSize.height() + 64; > > 64? I had it be exact initially, but on Android there was a small delta sometimes (a few pixels) between the animated layer size and the viewport size.
Comment on attachment 156214 [details] tweak Attachment 156214 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13421633 New failing tests: TiledLayerChromiumTest.pushIdlePaintedOccludedTiles TiledLayerChromiumTest.tilesNotPaintedWithoutInvalidation
Created attachment 156227 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Alas there was a bug in the "tweaks". That is fixed in a follow up patch here: https://bugs.webkit.org/show_bug.cgi?id=93059. I'll rebase this on whatever lands there.
Created attachment 156842 [details] rebase_on_93059
Comment on attachment 156842 [details] rebase_on_93059 View in context: https://bugs.webkit.org/attachment.cgi?id=156842&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591 > + if (m_tiler->hasEmptyBounds()) > + return; > + Why is this needed now? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614 > + for (int j = top; j <= bottom; ++j) > + for (int i = left; i <= right; ++i) > + if (!tileAt(i, j)) > + createTile(i, j); style nit: use braces for any indentation that is longer than one line. > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:660 > + // FIXME: Make this based on distance once distance can be calculated for offscreen layers. > + // For now, prioritize all small animated layers after 512 pixels of pre-painting. This is encroaching on uncomfortable special-casey-ness I think.. They have priority=distance(512) for texture allocation, but priority=visible for paint ordering. Everything else has paint ordering == texture allocation, right? Can we do something cleaner here? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:662 > + if (smallAnimatedLayer) > + priority = CCPriorityCalculator::maxPriority(priority, priorityCalc.priorityFromDistance(512, drawsToRoot)); Why not use else if, and then you can set it directly instead of needing maxPriority()? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:701 > + // Animation pre-paint. If the layer is small, try to paint it all > + // immediately to avoid paint/upload hiccups while it is animating. If the priority of the layer is higher than we don't need to special-case it here, right? Can we solve this problem (in this method) with priorities instead? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:711 > + if (contentRect.isEmpty()) > + return; Does this change mean that a layer that is scrolled just outside of view will not prepaint at all? It will have an empty visibleContentRect right? Will this regress that case?
Comment on attachment 156842 [details] rebase_on_93059 View in context: https://bugs.webkit.org/attachment.cgi?id=156842&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591 >> + > > Why is this needed now? Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway. >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614 >> + createTile(i, j); > > style nit: use braces for any indentation that is longer than one line. Each indentation is indeed only one line? ;) Seriously though webkit-patch doesn't complain, which style guide is this in? >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:660 >> + // For now, prioritize all small animated layers after 512 pixels of pre-painting. > > This is encroaching on uncomfortable special-casey-ness I think.. They have priority=distance(512) for texture allocation, but priority=visible for paint ordering. Everything else has paint ordering == texture allocation, right? Can we do something cleaner here? This will get a lot cleaner iff we have location/direction of offscreen layers (lack of which caused this special case in the first place). I just don't want to wait for that since this has such a big impact. Paint-order and priority-order being different might help for a generic algorithm at which point this case can be removed. >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:662 >> + priority = CCPriorityCalculator::maxPriority(priority, priorityCalc.priorityFromDistance(512, drawsToRoot)); > > Why not use else if, and then you can set it directly instead of needing maxPriority()? It would probably be fine, but becoming visible could actually lower the priority which would be kind of weird. >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:701 >> + // immediately to avoid paint/upload hiccups while it is animating. > > If the priority of the layer is higher than we don't need to special-case it here, right? Can we solve this problem (in this method) with priorities instead? Yes, I think with both location and direction we probably can, and this special case should just fall-out of some to-be-implemented generic behavior. However, I'd like to get Android on par to m18 in at least a few cases. It's completely busted right now and this patch should let us determine if there is further work or bugs that need to be addressed. >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:711 >> + return; > > Does this change mean that a layer that is scrolled just outside of view will not prepaint at all? It will have an empty visibleContentRect right? Will this regress that case? We have never pre-painted layers that are not visible :( (with the exception of this special case for animated layers). We don't know where the off-screen layers are relative to being visible, so we don't know how to prioritize them.
Comment on attachment 156842 [details] rebase_on_93059 View in context: https://bugs.webkit.org/attachment.cgi?id=156842&action=review >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591 >>> + >> >> Why is this needed now? > > Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway. With as many loops as we have at this point, we should probably just have an iterator that takes a rect. >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614 >>> + createTile(i, j); >> >> style nit: use braces for any indentation that is longer than one line. > > Each indentation is indeed only one line? ;) > Seriously though webkit-patch doesn't complain, which style guide is this in? http://www.webkit.org/coding/coding-style.html#braces-one-line
Comment on attachment 156842 [details] rebase_on_93059 View in context: https://bugs.webkit.org/attachment.cgi?id=156842&action=review >>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591 >>>> + >>> >>> Why is this needed now? >> >> Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway. > > With as many loops as we have at this point, we should probably just have an iterator that takes a rect. I was thinking along the same lines. Or maybe it could fill a vector that you pass in with the tiles, which we could then pass around to the various functions instead of doing all the double for-loops. >>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:614 >>>> + createTile(i, j); >>> >>> style nit: use braces for any indentation that is longer than one line. >> >> Each indentation is indeed only one line? ;) >> Seriously though webkit-patch doesn't complain, which style guide is this in? > > http://www.webkit.org/coding/coding-style.html#braces-one-line My take on that is that only this be wrong: for (int j = top; j <= bottom; ++j) for (int i = left; i <= right; ++i) if (!tileAt(i, j)) createTile(i, j); But I'm surely wrong so I've changed it. Thanks for the link :) To see the effect of this patch, go here on a debug build, and use 'flow': http://jquerymobile.com/demos/1.1.0/docs/pages/page-transitions.html I see unpainted tiles during the 'flow' transition without this patch. Do we not have the logic to block frames anymore? In any event blocking frames wasn't sufficient on Android as it would just jump to the end.
Created attachment 157346 [details] rebase_on_refactor
Created attachment 157369 [details] rebase_to_master
Comment on attachment 157369 [details] rebase_to_master Addressed previous comments, unless there is remaining issues, should be gtg.
Comment on attachment 156842 [details] rebase_on_93059 View in context: https://bugs.webkit.org/attachment.cgi?id=156842&action=review >>>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:591 >>>>> + >>>> >>>> Why is this needed now? >>> >>> Without this we will create a tile below I think. contentRectToTileIndices will return a bunch of 0's which will result in createTile(0,0). This might be a weakness of contentRectToTileIndices/tileXIndexFromSrcCoord since it can't return empty sets of tiles. Although it's probably a good idea to early-out when we can anyway. >> >> With as many loops as we have at this point, we should probably just have an iterator that takes a rect. > > I was thinking along the same lines. Or maybe it could fill a vector that you pass in with the tiles, which we could then pass around to the various functions instead of doing all the double for-loops. Btw, I added an ASSERT in contentRectToTileIndices to catch this for now. It didn't get hit in unit tests or chrome so that's good.
Comment on attachment 157369 [details] rebase_to_master View in context: https://bugs.webkit.org/attachment.cgi?id=157369&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:600 > + m_tiler->contentRectToTileIndices(createTilesRect, left, top, right, bottom); > + for (int j = top; j <= bottom; ++j) { > + for (int i = left; i <= right; ++i) { Can you explain why this loop operates on createTilesRect but the partial update / buffered update pass directly below this operates on visibleContentRect? Shouldn't this special case also go down that latter path too?
Comment on attachment 157369 [details] rebase_to_master View in context: https://bugs.webkit.org/attachment.cgi?id=157369&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:600 >> + for (int i = left; i <= right; ++i) { > > Can you explain why this loop operates on createTilesRect but the partial update / buffered update pass directly below this operates on visibleContentRect? Shouldn't this special case also go down that latter path too? Hmm, that sounds like an improvement yes! This would only help in the case of a layer that changes it's content while becoming visible, but since we now have a bound on the size we can do better than normal pre-painting here. I'll see if it's easy to change that. Possibly this could be mirrored in the generic pre-painting case, but that seems a lot harder since the size is unbounded and something still has to give when painting is too slow. So maybe a special case will always be warranted even in the more general case, which enables double-buffering when the pre-paint size is bounded and reasonable.
Created attachment 157561 [details] Patch
Comment on attachment 157561 [details] Patch R=me.
Created attachment 157584 [details] fix_assert
Comment on attachment 157584 [details] fix_assert View in context: https://bugs.webkit.org/attachment.cgi?id=157584&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:146 > + if (size.isEmpty()) { Without this we were hitting the ASSERT. Looks like a layer that is resized to zero could keep a tile without this.
Comment on attachment 157584 [details] fix_assert If you are okay with the ASSERT fix, gtg.
Comment on attachment 157584 [details] fix_assert R=me. Yeah, that totally looks like a minor bug in setBounds(), thanks. :)
Comment on attachment 157584 [details] fix_assert Clearing flags on attachment: 157584 Committed r125230: <http://trac.webkit.org/changeset/125230>
All reviewed patches have been landed. Closing bug.
This change caused a compile error on Chromium-mac bots, and I landed a fix: http://trac.webkit.org/changeset/125238 Please take a look and ping me if my fix was not okay.
Turned out that this change has caused some chromiumos browser_tests to time out: http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20ChromiumOS%20Tests%20%282%29&number=5902 BrowserNonClientFrameViewAshTest.UseShortHeader LauncherAppBrowserTest.LaunchMaximized These tests failed consistently on Chromium's main waterfall, and, after some local testing, I confirmed this change (r125230) is the culprit. I'm going to ROLL OUT this change to make WebKit roll possible. Please fix the above issue and land the patch again. I appreciate your understanding.
Re-opened since this is blocked by 93698
Created attachment 157769 [details] fix_typo
Fixed the typo mentioned. That was had a pretty minor effect so there must be an issue on ChromeOS that I'm not seeing. Gonna look into that now.
Comment on attachment 157769 [details] fix_typo Removing r? until I can sort out what went wrong on ChromeOS
Created attachment 157836 [details] fix_idlePaintRect
Comment on attachment 157836 [details] fix_idlePaintRect View in context: https://bugs.webkit.org/attachment.cgi?id=157836&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:802 > + // Don't inflate an empty rect. This was the only change. I removed the special old case but didn't add back the previous code.
Comment on attachment 157836 [details] fix_idlePaintRect Sorry 'bout that. Should be gtg.
(In reply to comment #37) > (From update of attachment 157836 [details]) > Sorry 'bout that. Should be gtg. Can you explain more what went wrong?
(In reply to comment #38) > (In reply to comment #37) > > (From update of attachment 157836 [details] [details]) > > Sorry 'bout that. Should be gtg. > > Can you explain more what went wrong? IntRect::inflate() in idlePaintRect will happily inflate an empty visible rect into a non-empty rect (without the special check I just added back). Then we would think there is more painting to do in needsIdlePaint, but we would never paint anything because we correctly exit early when actually painting.
(In reply to comment #39) > (In reply to comment #38) > > (In reply to comment #37) > > > (From update of attachment 157836 [details] [details] [details]) > > > Sorry 'bout that. Should be gtg. > > > > Can you explain more what went wrong? > > IntRect::inflate() in idlePaintRect will happily inflate an empty visible rect into a non-empty rect (without the special check I just added back). Then we would think there is more painting to do in needsIdlePaint, but we would never paint anything because we correctly exit early when actually painting. I see that the inflate in the previous patch is wrong, but I don't understand how this fixed the test failures. Were you able to repro those timeouts locally, or did you run a try job to check if this fixed them?
(In reply to comment #40) > (In reply to comment #39) > > (In reply to comment #38) > > > (In reply to comment #37) > > > > (From update of attachment 157836 [details] [details] [details] [details]) > > > > Sorry 'bout that. Should be gtg. > > > > > > Can you explain more what went wrong? > > > > IntRect::inflate() in idlePaintRect will happily inflate an empty visible rect into a non-empty rect (without the special check I just added back). Then we would think there is more painting to do in needsIdlePaint, but we would never paint anything because we correctly exit early when actually painting. > > I see that the inflate in the previous patch is wrong, but I don't understand how this fixed the test failures. Were you able to repro those timeouts locally, or did you run a try job to check if this fixed them? I was able to build the failing test using chromeos=1 locally and see that the fixed worked. This just barely escapes the unit tests so I was looking at adding one that would catch this as well.
Created attachment 158127 [details] fix_unit_tests_and_rebase
Comment on attachment 158127 [details] fix_unit_tests_and_rebase There was two different bugs in idlePaintNonVisibleLayers (there wasn't enough memory to allocate the tile, and we didn't call updateTextures(), both of which make it impossible for the test to fail). So I fixed that up, watched it fail/pass along with the chromeos test, and added some extra checks that we are keeping memory after layers become invisible.
I'm still missing a piece of this. Why were the ChromeOS tests failing?
(In reply to comment #44) > I'm still missing a piece of this. Why were the ChromeOS tests failing? It was in an endless painting loop in webkit. I'm not sure what part of the test checks that webkit is done working (on desktop this would just be a battery drain but otherwise fine), but I could see it spinning away thinking it needed to paint and off-screen layer but then never doing so.
(In reply to comment #45) > (In reply to comment #44) > > I'm still missing a piece of this. Why were the ChromeOS tests failing? > > It was in an endless painting loop in webkit. I'm not sure what part of the test checks that webkit is done working (on desktop this would just be a battery drain but otherwise fine), but I could see it spinning away thinking it needed to paint and off-screen layer but then never doing so. Rather, it was in an endless sequence of setNeedsCommit()'s. This sums it up: - PaintSomething -> Does nothing - NeedIdlePaint? -> Yes -> setNeedsCommit - repeat.
Comment on attachment 158127 [details] fix_unit_tests_and_rebase R=me. The bugfix looks correct to me. I'm still not sure I understand why endless committing would cause the test to timeout, but it sounds plausible.
(In reply to comment #47) > (From update of attachment 158127 [details]) > R=me. The bugfix looks correct to me. I'm still not sure I understand why endless committing would cause the test to timeout, but it sounds plausible. I kind of jumped to the bug introduced by this CL, but didn't investigate why endless commits would cause it to time-out or if that is expected.
Comment on attachment 158127 [details] fix_unit_tests_and_rebase Clearing flags on attachment: 158127 Committed r125498: <http://trac.webkit.org/changeset/125498>