[chromium] Cull occluded tiles during paint
Created attachment 123625 [details] Patch For @enne review. - Mostly plumbing the currently occluded screen space around to the paint functions. - Don't reserve textures for, or paint, tiles that are contained in the occluded region, by leaving their m_updateRect empty. - Bunch of unit tests to check that occlusion prevents paints in the right ways.
Comment on attachment 123625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123625&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:410 > if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialTextureUpdates) > layerTreeHost()->deleteTextureAfterCommit(tile->managedTexture()->steal()); > > + // When not idle painting, if the tile is occluded don't reserve a texture or mark it for update. This will also avoid painting > + // the tile in the next loop, below. > + if (!idle) { > + IntRect tileRect = m_tiler->tileBounds(i, j); > + if (regionContainsRect(occludedScreenSpace, contentToScreenSpace.mapRect(tileRect))) > + continue; > + } I think this new conditional block maybe needs to go above the deleteTextureAfterCommit call above, since the textures aren't being updated? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:89 > + void prepareToUpdateIdle(const IntRect& layerRect, const Region* occludedScreenSpace); Is this param needed? > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:92 > + bool needsIdlePaint(const IntRect& layerRect, const Region* occludedScreenSpace); Is this param needed?
(In reply to comment #2) > (From update of attachment 123625 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123625&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:410 > > if (tile->isDirty() && layerTreeHost() && !layerTreeHost()->settings().partialTextureUpdates) > > layerTreeHost()->deleteTextureAfterCommit(tile->managedTexture()->steal()); > > > > + // When not idle painting, if the tile is occluded don't reserve a texture or mark it for update. This will also avoid painting > > + // the tile in the next loop, below. > > + if (!idle) { > > + IntRect tileRect = m_tiler->tileBounds(i, j); > > + if (regionContainsRect(occludedScreenSpace, contentToScreenSpace.mapRect(tileRect))) > > + continue; > > + } > > I think this new conditional block maybe needs to go above the deleteTextureAfterCommit call above, since the textures aren't being updated? k. > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:89 > > + void prepareToUpdateIdle(const IntRect& layerRect, const Region* occludedScreenSpace); > > Is this param needed? no, but it a) avoids a null pointer in prepareToUpdateTiles and b) is needed in the CCLayerTreeHost methods for both idle and non-idle and c) allows for some decisions to be made in prioritizing prepainting based on occlusion in the future. do you want it gone and pass null to prepareToUpdateTiles() instead? > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:92 > > + bool needsIdlePaint(const IntRect& layerRect, const Region* occludedScreenSpace); > > Is this param needed? ditto.
Comment on attachment 123625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123625&action=review >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:89 >>> + void prepareToUpdateIdle(const IntRect& layerRect, const Region* occludedScreenSpace); >> >> Is this param needed? > > no, but it a) avoids a null pointer in prepareToUpdateTiles and b) is needed in the CCLayerTreeHost methods for both idle and non-idle and c) allows for some decisions to be made in prioritizing prepainting based on occlusion in the future. > > do you want it gone and pass null to prepareToUpdateTiles() instead? Ok, sure. Leaving it seems reasonable, then. >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:92 >>> + bool needsIdlePaint(const IntRect& layerRect, const Region* occludedScreenSpace); >> >> Is this param needed? > > ditto. This one really does look unused?
(In reply to comment #4) > >>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.h:92 > >>> + bool needsIdlePaint(const IntRect& layerRect, const Region* occludedScreenSpace); > >> > >> Is this param needed? > > > > ditto. > > This one really does look unused? Oh! Yeh this one should def go, thank you. Legacy from preventing prepainting of certain tiles and needing this thing to match, but it was a bad idea to do that at all.
Created attachment 123796 [details] Patch comments addressed.
Created attachment 124156 [details] Patch - rebased on the new paint occlusion per-RS patch. - removed the unused param from needsIdlePaint()
Created attachment 124168 [details] Patch rebased and using Region& instead of Region* since can't be null.
Created attachment 124211 [details] Patch rebased on latest paint occlusion tracking
Created attachment 124387 [details] Patch rebased
Comment on attachment 124387 [details] Patch LGTM. :)
Comment on attachment 124387 [details] Patch This is going to fail EWS until the occlusion tracking lands, but it's a pretty simple CL and mostly plumbing, so maybe reviewable?
Created attachment 124661 [details] Patch rebased
Comment on attachment 124387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124387&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:405 > + continue; can you add a very noticable TRACE_EVENT here? if we (heaven forbid) start getting bug reports i want it to be really easy to have people record a trace and then easily say "no, culling is definitely not an issue on this page". similarly i want to be able to easily look at a page and tell how effective paint culling is > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:415 > + IntRect fixedSizeTileRectWithBorders = m_tiler->tileRect(tile); > + tile->m_dirtyRect = fixedSizeTileRectWithBorders; this temp isn't providing much value, certainly not enough to triple the line count of what was a one-liner > Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:63 > + Texture(PassOwnPtr<ManagedTexture> texture, FakeLayerTextureUpdater* updater) : LayerTextureUpdater::Texture(texture), m_updater(updater) { } this is too long for one line now, please expand it out to one initialization per line
Comment on attachment 124387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124387&action=review >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:405 >> + continue; > > can you add a very noticable TRACE_EVENT here? if we (heaven forbid) start getting bug reports i want it to be really easy to have people record a trace and then easily say "no, culling is definitely not an issue on this page". similarly i want to be able to easily look at a page and tell how effective paint culling is k >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:415 >> + tile->m_dirtyRect = fixedSizeTileRectWithBorders; > > this temp isn't providing much value, certainly not enough to triple the line count of what was a one-liner ah, this should already be done in the latest patch submitted to match the previous CL :) >> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:63 >> + Texture(PassOwnPtr<ManagedTexture> texture, FakeLayerTextureUpdater* updater) : LayerTextureUpdater::Texture(texture), m_updater(updater) { } > > this is too long for one line now, please expand it out to one initialization per line k
Created attachment 124667 [details] Patch comments addressed. CCLayerTreeHostLayerOcclusion unit test fixed.
Created attachment 124673 [details] Patch same patch, but without the unit test fix. bug #76858 was reverted and i've fixed the problem there already. leaving the previous version R? to try not clobber EWS.
Created attachment 125006 [details] Patch - using Region::contains() since that landed finally - cull tiles if their visible part is occluded (was previously testing if non-visible parts were occluded too) - added unit tests for the missed visibility interaction
Created attachment 125879 [details] Patch rebased.
Created attachment 129954 [details] Patch Updated. Using CCOcclusionTracker. Tested on Aura.
Oh I should say the unit test I removed will get coverage again in https://bugs.webkit.org/show_bug.cgi?id=80173
Comment on attachment 129954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129954&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:434 > + TRACE_EVENT("TiledLayerChromium::prepareToUpdateTiles::tileCulled", this, 0); How many trace events is this going to generate? This seems less like an event, and more like a job for TRACE_COUNTER, or maybe some level of aggregation and reporting in the data section of a higher level trace event?
(In reply to comment #22) > (From update of attachment 129954 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129954&action=review > > > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:434 > > + TRACE_EVENT("TiledLayerChromium::prepareToUpdateTiles::tileCulled", this, 0); > > How many trace events is this going to generate? This seems less like an event, and more like a job for TRACE_COUNTER, or maybe some level of aggregation and reporting in the data section of a higher level trace event? jamesr requested something like this back when I first wrote this so we could tell if culling was related to bug reports. I'm not sure how relevant it is now, especially when draw and paint culling match 1:1, as we'll see compositor blue in the same places as paint culling. But I didn't know about TRACE_COUNTER, that might be better, if it can be used in the same way for debugging. @jamesr what do you think?
I want there to be some indication in the trace of what's happening, but a counter or a higher-level aggregated thing would be a lot better than putting a full event on every tile. The tracing system can't really handle that level of spamming.
The higher-level goal being able to tell from a trace if culling is doing anything on a page or not.
Created attachment 130623 [details] Patch - Paint-cull tiles if their dirty rect is occluded (instead of if their whole tile bounds are occluded). - Added unit test coverage for this. - Removed the TRACE_EVENT. I'd like to do the TRACE stuff in a separate CL. Thanks @enne for pointing out @wjmaclean's histogram stuff in CCQuadCuller now. I need to add some support for this with CCOcclusionTracker, and I'd like to cover the paint culling at the same time. Also, I'd like to push the data in his histograms into TRACE_COUNTER's as well for debugging, probably.
Created attachment 130655 [details] Patch - Don't paint-cull tiles if their dirty rect is occluded! - Added unit test coverage for this. If the tile is dirty, but party visible, paint-culling it would cause us to not push it to impl thread, making for bad times.
Comment on attachment 130655 [details] Patch I'm fine with punting the trace event to a different patch, but can you file a bug for it?
(In reply to comment #28) > (From update of attachment 130655 [details]) > I'm fine with punting the trace event to a different patch, but can you file a bug for it? Thanks! http://code.google.com/p/chromium/issues/detail?id=117155
Comment on attachment 130655 [details] Patch Clearing flags on attachment: 130655 Committed r110088: <http://trac.webkit.org/changeset/110088>
All reviewed patches have been landed. Closing bug.