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

Fri Nov 18 15:03:36 PST 2011

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

--- Comment #5 from Eric Penner <epenner at chromium.org>  2011-11-18 15:03:36 PST ---
(From update of attachment 115730)
View in context: https://bugs.webkit.org/attachment.cgi?id=115730&action=review

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:129
>> +        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.

Thanks for the info! I had some of these issues in the back of my head but this clarifies everything. It looks like we need the current paintLayerContents pass which will reserve all crucial tiles, followed by another pass for pre-painting that can fail separately from the visible pass. 

In the second pass, when should we actually pre-paint? Would it be acceptable for pre-painting to be a no-op if there was a visible paint in the first pass? With a setNeedsCommit to defer the prepainting until later?  Also, do you agree with expanding the the pre-paint by one row/column at a time with setNeedsCommit in-between?

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

Agreed on the back and forth conversions. I was going to break up that function into two versions, but I hadn't thought of using it as the detector as well. 

Regarding reserving tiles, It sounds like that would also help with reserving pre-paint tiles, since it would slowly reserve tiles in order of importance.

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

I thought there might be a valid distinction between tiles that already have something something valid uploaded from a previous frame as opposed to an uninitialized texture or checkerboard. I thought worst case there would be a seam between the newly painted tiles and the old dirty tiles (until the dirty tiles get painted). Do you think it would be worse than that? 

In any event, that is secondary to the other suggested changes, so I'll leave it for later (if it even becomes an issue).

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