Bug 76838 – [chromium] Cull occluded tiles during paint

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Cull occluded tiles during paint

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

(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.

[Adrienne Walker]

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?

[Dana Jansens]

(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.

[Dana Jansens]

Created attachment 123796 [details]
Patch

comments addressed.

[Dana Jansens]

Created attachment 124156 [details]
Patch

- rebased on the new paint occlusion per-RS patch.
- removed the unused param from needsIdlePaint()

[Dana Jansens]

Created attachment 124168 [details]
Patch

rebased and using Region& instead of Region* since can't be null.

[Dana Jansens]

Created attachment 124211 [details]
Patch

rebased on latest paint occlusion tracking

[Dana Jansens]

Created attachment 124387 [details]
Patch

rebased

[Adrienne Walker]

Comment on attachment 124387 [details]
Patch

LGTM.  :)

[Dana Jansens]

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?

[Dana Jansens]

Created attachment 124661 [details]
Patch

rebased

[James Robinson]

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

[Dana Jansens]

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

[Dana Jansens]

Created attachment 124667 [details]
Patch

comments addressed. CCLayerTreeHostLayerOcclusion unit test fixed.

[Dana Jansens]

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.

[Dana Jansens]

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

[Dana Jansens]

Created attachment 125879 [details]
Patch

rebased.

[Dana Jansens]

Created attachment 129954 [details]
Patch

Updated. Using CCOcclusionTracker. Tested on Aura.

[Dana Jansens]

Oh I should say the unit test I removed will get coverage again in https://bugs.webkit.org/show_bug.cgi?id=80173

[Adrienne Walker]

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?

[Dana Jansens]

(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?

[James Robinson]

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.

[James Robinson]

The higher-level goal being able to tell from a trace if culling is doing anything on a page or not.

[Dana Jansens]

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.

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

(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

[WebKit Review Bot]

Comment on attachment 130655 [details]
Patch

Clearing flags on attachment: 130655

Committed r110088: <http://trac.webkit.org/changeset/110088>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.