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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Adrienne Walker <enne at google.com>  2011-11-18 13:04:26 PST ---
(From update of attachment 115730)
View in context: https://bugs.webkit.org/attachment.cgi?id=115730&action=review

> Source/WebCore/ChangeLog:3
> +        Need to prepaint tiles in TileLayerChomium

In WebKit, if you're only touching Chromium code, you should prefix the bug title with [chromium] so that other ports know they can ignore it.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

This should really get some unit tests.  See Source/WebKit/chromium/tests/.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:115
> +    prepaintRect.inflate(256); // TODO: use tile size or other metric

You should just expose this from TiledLayerChromium if you need it, but I kind of think TiledLayerChromium should be responsible for the expansion.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:129
> +    if (visibleDirty) {
> +        prepareToUpdate(layerRect);

I think this conditional will interact poorly with texture reservation.  You need to prepareToUpdate with the visible rect always, even if it's not dirty.  That's the mechanism through which all the visible textures get reserved.  So, maybe it would make sense to either have prepareToUpdate have a return value or maybe just inquire directly with m_paintRect.isEmpty() about whether anything has been painted and is pending upload.

On the subject of texture reservation, I think we need a second pass through the tree, parallel to CCLayerTreeHost::paintLayerContents, where we give each layer a chance to prepaint.  In that step, a layer should reserving non-dirty tiles with valid textures that already exist (maybe anything within +/- one viewport of the current viewport, clamped to content bounds).  Then, start calculating what else to prepaint using something similar to this mechanism.

If you paint + prepaint in the same step, you are prioritizing the prepainted textures over textures from later layers that are actually on screen.  Worse, those layers will flicker as texture pressure changes based on prepainting.  If you don't reserve all the prepainted tiles that you have ahead of time, in cases where you're near the memory limit you might needlessly do work to prepaint extra tiles each frame, only to evict some other prepainted tiles.  If you do reserve, then texture allocation will fail, and you can early out, as we do now.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:348
> +              return m_tiler->tileIndicesToContentRect(left, i, right, i);

It feels a little cumbersome to go from content rect -> indices -> content rect -> indices.

Here's a refactoring suggestion that might make this code cleaner.  You might want to consider breaking "void prepareToUpdate(contentRect)" into " bool prepareToUpdate(contentRect)" and "bool prepareToUpdate(indices)".  The painting function already does the check for missing/dirty tiles, so that would eliminate the containsMissingTiles and containsDirtyTiles functions.  You could simply say "try painting these indices" and if that fails to find anything to paint you could iterate through different sets of indices until you find something valid.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:358
> +    // Exactly the same as above but includes dirty tiles.

I don't think there's any difference between missing and dirty tiles from the perspective of prepaint.  There's really just valid and invalid content.  As soon as you commit a new frame, any tiles that are dirty but not updated might as well be missing, because we can't display them without displaying potentially incorrect content from a previous frame.  (We incorrectly do this now; jamesr is in the middle of fixing this.)

It'd be fairly complex to track whether or not a missing tile could be uploaded or not without also not updating a dirty tile--you'd need to track which frame the invalidations happened so that the old dirty tile contents weren't out of date with the new contents you drew in the missing tile.  I'm not convinced it's worth the complexity.

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