[Webkit-unassigned] [Bug 72686] [chromium] Need to prepaint tiles in TiledLayerChromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72686





--- Comment #13 from Eric Penner <epenner at chromium.org>  2011-11-29 15:32:25 PST ---
(From update of attachment 116562)
View in context: https://bugs.webkit.org/attachment.cgi?id=116562&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> I'd love to see some unit tests for this.  If you need any pointers about where to look, what to mock out, or where to start, I'm happy to help.

I found them and saw the existing tests. I'll ping you on how to quick iterate on them.

>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:164
>> +    virtual void paintContentsIfDirty(bool idlePaint) { }
> 
> I know you're trying to reuse the layer iterating code in CCLayerTreeHost, but it's awkward to pass a boolean for custom behavior here.  reveman has a patch out that makes iterating through layers less painful, and I think it'll be cleaner to just have separate paintContentsIfDirty(void) and idlePaintContentsIfDirty(void) functions.  See: https://bugs.webkit.org/attachment.cgi?id=116293

Yeah I had that initially but changed it to reduce code. I'll change it back.

>> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:42
>> +    // FIXME: Choose a limit based on viewport size.
> 
> I think any adjusting of texture limits based on viewport size should be done in a separate patch.  jamesr did something similar here: https://bugs.webkit.org/show_bug.cgi?id=72202

Okay I will remove it from this patch.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:426
>> +    idlePaintContentRect.inflate(idlePaintPaddingWidth);
> 
> I don't think idlePaintPaddingWidth should explicitly be the viewport size, because that won't make any sense for tiled non-root layers.
> 
> The contentRect here is the part of the layer that's visible.  For the root layer, this will always be the clip layer, which is sized to the viewport.  For iframes, the contentRect will (most often, unless it's clipped by some other masking layer or is partially offscreen) be the size of its clip rect as well, which is the portion of it that will be visible.  I think it'd be sufficient to use contentRect as an approximation for the viewport size.

Good point. However, with a single tile of padding the effects on memory management are minimal, so possibly memory priority could be dealt with in another patch this way. I tried to resolve the memory priority issue in this patch but I found it difficult since there are a few conflicting problems:

- Memory priority taken from visible tiles due to LRU (as you mentioned)
- Layers that idle paint earlier can evict later layers' pre-painted tiles.
- Endless paint thrashing can occur if we don't protect existing pre-paint tiles before trying to reserve new ones (eg. we could thrash between two layers, or between top/bottom within one layer). 

I think I've got a decent solution for all of the above, but using a small padding with high priority gets around the issue for now.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:439
>> +    int l1, t1, r1, b1, l2, t2, r2, b2;
> 
> style nit: Abbreviations are frowned upon in WebKit.  Maybe visibleLeft or contentLeft vs. offscreenLeft or prepaintLeft?

I was thinking they could viewed as iterators (like 'i' in a for loop). I will change to left, top, right, bottom, prepaintLeft, etc.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:447
>> +                break;
> 
> m_skipsIdlePaint seems kind of misnamed.  It's more "idle painting ran out of memory", and it's possible that we still painted a few tiles and that it wasn't skipped at all.  Could you just return a boolean from prepareToUpdateTiles about whether painting occurred or not, rather than setting a flag?

I did that briefly, but needsIdlePaint needs to know about out-of-memory so we don't keep calling setNeedsCommit. I'm thinking:
a.) Maybe we should indeed skip the paint if part of the row/column can't be painted in full (I can reset the paintrect etc.)
b.) I can just rename it to needsIdlePaint.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:477
>> +    idlePaintContentRect.intersect(IntRect(IntPoint::zero(), contentBounds()));
> 
> If you're going to duplicate this logic, it should probably be its own function.

I'll add a function that takes contentRect since this will be needed in other places.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.