Bug 76838

Summary: [chromium] Cull occluded tiles during paint
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: backer, cc-bugs, enne, jamesr, piman, reveman, senorblanco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76858    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dana Jansens 2012-01-23 10:12:55 PST
[chromium] Cull occluded tiles during paint
Comment 1 Dana Jansens 2012-01-23 14:42:11 PST
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 2 Adrienne Walker 2012-01-24 11:07:51 PST
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?
Comment 3 Dana Jansens 2012-01-24 12:26:23 PST
(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 4 Adrienne Walker 2012-01-24 12:42:22 PST
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?
Comment 5 Dana Jansens 2012-01-24 12:55:09 PST
(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.
Comment 6 Dana Jansens 2012-01-24 13:37:19 PST
Created attachment 123796 [details]
Patch

comments addressed.
Comment 7 Dana Jansens 2012-01-26 11:47:15 PST
Created attachment 124156 [details]
Patch

- rebased on the new paint occlusion per-RS patch.
- removed the unused param from needsIdlePaint()
Comment 8 Dana Jansens 2012-01-26 13:10:17 PST
Created attachment 124168 [details]
Patch

rebased and using Region& instead of Region* since can't be null.
Comment 9 Dana Jansens 2012-01-26 16:33:42 PST
Created attachment 124211 [details]
Patch

rebased on latest paint occlusion tracking
Comment 10 Dana Jansens 2012-01-27 15:51:03 PST
Created attachment 124387 [details]
Patch

rebased
Comment 11 Adrienne Walker 2012-01-27 19:04:35 PST
Comment on attachment 124387 [details]
Patch

LGTM.  :)
Comment 12 Dana Jansens 2012-01-30 17:44:38 PST
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?
Comment 13 Dana Jansens 2012-01-30 20:09:59 PST
Created attachment 124661 [details]
Patch

rebased
Comment 14 James Robinson 2012-01-30 20:16:04 PST
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 15 Dana Jansens 2012-01-30 20:46:11 PST
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
Comment 16 Dana Jansens 2012-01-30 21:07:02 PST
Created attachment 124667 [details]
Patch

comments addressed. CCLayerTreeHostLayerOcclusion unit test fixed.
Comment 17 Dana Jansens 2012-01-30 22:41:06 PST
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.
Comment 18 Dana Jansens 2012-02-01 13:34:10 PST
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
Comment 19 Dana Jansens 2012-02-07 11:08:28 PST
Created attachment 125879 [details]
Patch

rebased.
Comment 20 Dana Jansens 2012-03-02 13:59:16 PST
Created attachment 129954 [details]
Patch

Updated. Using CCOcclusionTracker. Tested on Aura.
Comment 21 Dana Jansens 2012-03-02 15:12:24 PST
Oh I should say the unit test I removed will get coverage again in https://bugs.webkit.org/show_bug.cgi?id=80173
Comment 22 Adrienne Walker 2012-03-06 17:51:44 PST
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?
Comment 23 Dana Jansens 2012-03-06 18:15:46 PST
(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?
Comment 24 James Robinson 2012-03-06 18:21:26 PST
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.
Comment 25 James Robinson 2012-03-06 18:22:13 PST
The higher-level goal being able to tell from a trace if culling is doing anything on a page or not.
Comment 26 Dana Jansens 2012-03-07 07:33:50 PST
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.
Comment 27 Dana Jansens 2012-03-07 11:23:11 PST
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 28 Adrienne Walker 2012-03-07 11:31:38 PST
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?
Comment 29 Dana Jansens 2012-03-07 11:35:14 PST
(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 30 WebKit Review Bot 2012-03-07 12:46:25 PST
Comment on attachment 130655 [details]
Patch

Clearing flags on attachment: 130655

Committed r110088: <http://trac.webkit.org/changeset/110088>
Comment 31 WebKit Review Bot 2012-03-07 12:46:32 PST
All reviewed patches have been landed.  Closing bug.