Bug 72686 – [chromium] Need to prepaint tiles in TiledLayerChromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Penner]

Need to prepaint tiles in TileLayerChomium

[Eric Penner]

Created attachment 115730 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115730 [details]
Patch

Attachment 115730 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10477475

[Nat Duca]

Comment on attachment 115730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115730&action=review

Looks like you're going to need Enne's review on this one, since it touches the tiler system only.

I totally buy that we can make this all work behind the setNeedsCommit blanket, and am psyched to see it running.

Dont forget to get some unit tests for the things you touched, too, when you've got buy off from enne on the approach.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:113
> +    // TODO: Prepaint rect currently just pads the visible rect by one tile.

s/TODO/FIXME

[Adrienne Walker]

Comment on attachment 115730 [details]
Patch

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.

[Eric Penner]

Comment on attachment 115730 [details]
Patch

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

[Adrienne Walker]

(In reply to comment #5)

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

Yeah, as a first pass, each layer could locally decide if it wanted to do prepainting.  And, it could always call setNeedsCommit unless it knew it was done prepainting.  And, if we're at the texture limit, then we can just not request a commit until a commit gets kicked off by WebKit or from the compositor thread due to scrolling.

Prepainting one new row or column at a time seems like a reasonable heuristic.  If there's nothing else going on, the commit will be fast and we can prepaint more.

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

Hrm.  The other thing is that reserving a tile means that it will be allowed to persist until the next frame, even over the soft reclamation limit.  We have two reclamation limits, one soft (which we'll fill up and not every bother going below) and one hard (which we'll never go above, but will temporarily go above if they're a lot visible at once).  If we reserve with the same mechanism, then we will always end up filling up towards the hard limit rather than the soft one.  And, even worse, the LRU mechanism in the texture manager will consider the prepainted tiles as a higher priority for eviction than on-screen tiles if we hit the upper limit, because they're the last thing reserved in the frame.

So, after some thought, I'm also wondering if we need a texture manager change here too to get desirable behavior.  :(

Just as a quick sketch of how this change could look: the texture manager needs to have a set (of two, for now?) LRU queues of different priorities.  When reserving in the texture manager, you'll need to pass an appropriate priority level so that the texture goes in the right queue.  Reserving texture can also change its priority (such as a prepainted tile becoming visible). When reducing texture memory in the texture memory (either to reserve something or during the evict stage), you should pull from the lower priority queue first until that's empty.  Reserving a lower priority texture should never reserve over the soft limit, whereas a high priority texture can.  That'll keep the texture limit sitting around the soft limit on average.

I don't mean to pile on an extra change here, but I'm not sure how else to make sure that we're not needlessly painting (i.e. still informing the texture manager about textures in use so we know when to stop prepainting because of texture limits) but also not interfere with normal texture operation (evicting things in the wrong LRU order with respect to least-recently-visible).

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

Yeah, that worst case is exactly the problem.  So, if a tile's dirty, we'd prefer to checkerboard over showing that seam.

[Eric Penner]

Created attachment 116306 [details]
Patch

[Eric Penner]

Created attachment 116308 [details]
Patch

[Eric Penner]

Created attachment 116316 [details]
Patch

[Eric Penner]

Created attachment 116442 [details]
Patch

[Eric Penner]

Created attachment 116562 [details]
Patch

[Adrienne Walker]

Comment on attachment 116562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116562&action=review

This is looking a lot better.  Thanks for all the changes.

I'm still worried about the TextureManager-related concerns that I brought up above.  For example, ManagedTexture::isValid (called via needsIdlePaint) touches the LRU queue, which will put offscreen textures behind onscreen textures regarding eviction.

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

> 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

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:42
> +size_t TextureManager::reclaimLimitBytes(const IntSize& viewportSize)
>  {
> +    // 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

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

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

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:447
> +    while ((l1 > l2 || t1 > t2 || r1 < r2 || b1 < b2) && !m_skipsIdlePaint) {
> +        if (b1 < b2) {
> +            ++b1;
> +            prepareToUpdateTiles(true, l1, b1, r1, b1);
> +            if (!m_paintRect.isEmpty() || m_skipsIdlePaint)
> +                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?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:477
> +    IntRect idlePaintContentRect = contentRect;
> +    idlePaintContentRect.inflate(idlePaintPaddingWidth);
> +    idlePaintContentRect.intersect(IntRect(IntPoint::zero(), contentBounds()));

If you're going to duplicate this logic, it should probably be its own function.

[Eric Penner]

Comment on attachment 116562 [details]
Patch

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.

[Adrienne Walker]

Comment on attachment 116562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116562&action=review

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

I'd be curious to hear what the eventual solution is.

In the short term, you should at least use m_tiler->tileSize() instead of defaultSize for idlePaintPaddingWidth.  Having a prepainted apron of even a single tile would still be a giant win for scrolling.

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

TiledLayerChromium::updateCompositorResources already asserts that all the textures that the paint rect intersects are reserved, so I think it's reasonable to not set the paint rect and just bail out if you can't reserve a whole row or column.

[Eric Penner]

Created attachment 117302 [details]
Patch

[Eric Penner]

Comment on attachment 117302 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117302&action=review

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

This now has a new TiledLayerChromiumTest and modifies CCLayerTreeHostTest. Should I modify this change log myself?

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:-109
> -    ASSERT(!m_textures.get(token).isProtected);

I remove this because we can call ::reserve() twice on the visible textures, which calls protectTexture. We reserve visible textures again when we update the expanded pre-paint area. 
I don't believe it should be a failure call protectTexture twice. Checking if it is protected first is just as expensive, and redundant in most use cases.

[James Robinson]

Yes, you need to modify the ChangeLog or an svn presubmit hook will reject your change. Ideally you would reference the tests in the ChangeLog

[Adrienne Walker]

(In reply to comment #16)
> (From update of attachment 117302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117302&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> This now has a new TiledLayerChromiumTest and modifies CCLayerTreeHostTest. Should I modify this change log myself?

Welcome to the wonderful world of ChangeLogs.  :(

I tend to just rerun prepare-ChangeLog and merge my previous changes with the new filelist.  There could probably be a smarter tool to be better about this, but nobody's written it yet.

> > Source/WebCore/platform/graphics/chromium/TextureManager.cpp:-109
> > -    ASSERT(!m_textures.get(token).isProtected);
> 
> I remove this because we can call ::reserve() twice on the visible textures, which calls protectTexture. We reserve visible textures again when we update the expanded pre-paint area. 
> I don't believe it should be a failure call protectTexture twice. Checking if it is protected first is just as expensive, and redundant in most use cases.

That seems totally reasonable to me, at least.  This made a lot more sense back in the day when we didn't just bulk unreserve all contents textures and we were trying to make sure that every reserved texture got unreserved first.

[James Robinson]

Comment on attachment 117302 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117302&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:340
> +    paintLayerContents(m_updateList, false /* idle */);
> +    // The second (idle) paint will be a no-op in layers where painting already occured above.
> +    paintLayerContents(m_updateList, true /* idle */);

we would normally use a two-state enum for the idle flag rather than a bool to make the callsite more readable

[Eric Penner]

Created attachment 117314 [details]
Patch

[Eric Penner]

Created attachment 117316 [details]
Patch

[Adrienne Walker]

Comment on attachment 117316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117316&action=review

Nice testing.  This is getting closer.

The biggest things missing in this patch are the TextureManager changes I described above.  In particular, non-visible tiles should be handled differently with respect to LRU.  Also, (in my opinion) we should only reserve textures for idle prepainting up to the reclaim limit.  Otherwise, we'll paint all the way up to the hard maximum limit and have no room for render surfaces.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:355
> +                m_skipsIdlePaint = true;

What happened to the idea of removing m_skipsIdlePaint and setting the paint rect to empty here? If you don't set the paint rect to empty here, you're going to get an assert in updateCompositorResources if you've painted any tiles.

On that note, can you add a test that involves idle painting hitting the memory limit?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:404
> +            // Primary paint could have set opacity to zero. Skip idle paint.
> +            if (paintType == PaintIdle && !layer->opacity())
> +                continue;

Can I ask you to not add this extra complexity to tree traversal? Layers becoming transparent during painting are very much an infrequent edge case. This sort of conditional during the idle painting tree traversal will not jive with reveman's patch that adds a ForEachCompositorResource-style iterator. Given the current heuristic of not idle painting when we've already painted a layer, this should be caught at the TiledLayerChromium level anyway.

[David Reveman]

What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?

I'm asking because when I think of pre-painting of tiles I make these two assumptions:

1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
2. The impl thread will be much better at predicting what tiles to pre-render.

Isn't that correct? Based on those assumptions it seem like this pre-rendering is best driven from CCThreadProxy and the TiledLayerChromium implementation being unaware if it's rendering regular or pre-render tiles.

[James Robinson]

(In reply to comment #23)
> What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> 
> I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> 
> 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.

I don't think this is correct - the impl thread does not do any sort of 'prioritization' of tiles today. All eviction and painting decisions are made on the main thread.

> 2. The impl thread will be much better at predicting what tiles to pre-render.

I disagree here as well, the impl thread might be better at prediction how much to pre-render but it can't make accurate decisions on a tile or even on a layer basis since the layer tree may change between commits.

> 
> Isn't that correct? Based on those assumptions it seem like this pre-rendering is best driven from CCThreadProxy and the TiledLayerChromium implementation being unaware if it's rendering regular or pre-render tiles.

[Eric Penner]

Comment on attachment 117316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117316&action=review

Thanks for all the feedback! Regarding the texture manager changes, if possible I'd really like to leave that for another CL.  I had some changes in one of my early patches, but it wasn't quite sufficient to resolve all of the issues that arise from a large idle painting regions. 

As an outline of what I think would be needed:
- Reserving/Protecting needs two modes (fail at hard memory limit, or fail at soft memory limit).
- Memory priority could be handled a few ways. We could have two protected modes (not-protected, soft-protected, strong-protected), where we evict in that order. Or potentially we could get rid of the LRU all together and evaluate an explicit priority every frame (eg. distance from view).
- We need to reserve incrementally in order to be fair to all layers. This could be done with multiple passes of pre-painting, where we widen the pre-paint area by one tile at each iteration.

>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:355
>> +                m_skipsIdlePaint = true;
> 
> What happened to the idea of removing m_skipsIdlePaint and setting the paint rect to empty here? If you don't set the paint rect to empty here, you're going to get an assert in updateCompositorResources if you've painted any tiles.
> 
> On that note, can you add a test that involves idle painting hitting the memory limit?

Sorry, I misunderstood and thought you were okay with it.

The empty paint-rect isn't sufficient to replace m_skipsIdlePaint. We need to detect if we are done idle painting in needsIdlePaint(). The function would always return true for an empty paint-rect, resulting in infinite SetNeedsCommit calls. m_skipsIdlePaint clarifies that we don't need to paint anymore, even though there is plenty of dirty tiles remaining outside the current paint rect. 

Also, note that we don't set m_requestedUpdateRect or m_paintRect if we return early from this function. This will result in early exit from updateCompositorResources on the first line.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:404
>> +                continue;
> 
> Can I ask you to not add this extra complexity to tree traversal? Layers becoming transparent during painting are very much an infrequent edge case. This sort of conditional during the idle painting tree traversal will not jive with reveman's patch that adds a ForEachCompositorResource-style iterator. Given the current heuristic of not idle painting when we've already painted a layer, this should be caught at the TiledLayerChromium level anyway.

I'm all for that. This was to prevent an assert in a unit test as I recall. I'll remove and see if it still hit that assert.

[Adrienne Walker]

(In reply to comment #23)
> What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> 
> I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> 
> 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.

I've been thinking about this in a slightly different way.  The impl side needs to distinguish between visible tiles and non-visible tiles, not between regular and pre-paint.

The use case I'm thinking of is that if the impl side scrolls, a pre-painted tile may become a visible tile and should have a higher priority for upload than a regular tile that's now offscreen.  In other words, the main thread hands over content, and the impl side decides how best to use that content based on the current state of the world.

> 2. The impl thread will be much better at predicting what tiles to pre-render.

I think both threads have an equivalent state of the world with respect to prerendering prediction.  They know scroll history on a per layer basis.  They can sync over animation data once scroll gestures are hooked up to that and know where layers will scroll in the future.  I've been seeing them as equivalent.  Am I missing something that makes the impl thread have a much more up-to-date view?

[Eric Penner]

(In reply to comment #24)

I'd also argue we can't really do anything from the impl thread since we can't paint anything from there. We might know sooner that we need to pre-paint, but we can't do anything with that information until webkit is ready to paint something. 

Regarding predictive painting, currently it just stupidly favors top/bottom, but potentially we could pass it a hint to tell it what direction to favor etc. This could come from the scroll direction/velocity. 

> (In reply to comment #23)
> > What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> > 
> > I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> > 
> > 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
> 
> I don't think this is correct - the impl thread does not do any sort of 'prioritization' of tiles today. All eviction and painting decisions are made on the main thread.
> 
> > 2. The impl thread will be much better at predicting what tiles to pre-render.
> 
> I disagree here as well, the impl thread might be better at prediction how much to pre-render but it can't make accurate decisions on a tile or even on a layer basis since the layer tree may change between commits.
> 
> > 
> > Isn't that correct? Based on those assumptions it seem like this pre-rendering is best driven from CCThreadProxy and the TiledLayerChromium implementation being unaware if it's rendering regular or pre-render tiles.

[David Reveman]

(In reply to comment #24)
> (In reply to comment #23)
> > What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> > 
> > I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> > 
> > 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
> 
> I don't think this is correct - the impl thread does not do any sort of 'prioritization' of tiles today. All eviction and painting decisions are made on the main thread.

Yes, today it doesn't but I was thinking of what we want in the future. Wouldn't it be very useful for the impl thread to know if an upload is for a pre-rendering tile or for a tile that is visible after commit?

> 
> > 2. The impl thread will be much better at predicting what tiles to pre-render.
> 
> I disagree here as well, the impl thread might be better at prediction how much to pre-render but it can't make accurate decisions on a tile or even on a layer basis since the layer tree may change between commits.

Yes, it can't make accurate decisions. But does that also mean that the predictions it can make are not at all useful for deciding what tiles to pre-render?

I'm not trying to push this in a different direction or anything. Just trying to figure out in what way my assumptions are wrong.

[David Reveman]

(In reply to comment #26)
> (In reply to comment #23)
> > What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> > 
> > I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> > 
> > 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
> 
> I've been thinking about this in a slightly different way.  The impl side needs to distinguish between visible tiles and non-visible tiles, not between regular and pre-paint.

Yes, I'm with you on that. FYI, with my paint-in-parallel-with-upload patch what's visible and not is unknown to the impl thread when the new uploads start taking place.

> 
> The use case I'm thinking of is that if the impl side scrolls, a pre-painted tile may become a visible tile and should have a higher priority for upload than a regular tile that's now offscreen.  In other words, the main thread hands over content, and the impl side decides how best to use that content based on the current state of the world.

Sounds good. Painting a few extra tiles which we don't know if they will be visible or not is starting to make sense to me know. I guess the hard part is deciding which ones to paint.

> 
> > 2. The impl thread will be much better at predicting what tiles to pre-render.
> 
> I think both threads have an equivalent state of the world with respect to prerendering prediction.  They know scroll history on a per layer basis.  They can sync over animation data once scroll gestures are hooked up to that and know where layers will scroll in the future.  I've been seeing them as equivalent.  Am I missing something that makes the impl thread have a much more up-to-date view?

I was thinking of information about whatever smooth scrolling animations that could be taking place on the impl thread side. You know this stuff much better than me so if think the impl thread doesn't have much to offer in form of pre-render prediction, neither do I :) That also makes things much easier.

One last question about this patch. I haven't looked at the details of this patch yet but did you guys consider re-factoring the layer painting so that we could use the exact same code path for painting pre-render tiles as regular tiles. Maybe by replacing setVisibleRect with setRenderRect? In what ways do the pre-render tiles need to be treated differently than regular tiles within TiledLayerChromium?

[Eric Penner]

(In reply to comment #29)
> (In reply to comment #26)
> > (In reply to comment #23)
> > > What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> > > 
> > > I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> > > 
> > > 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
> > 
> > I've been thinking about this in a slightly different way.  The impl side needs to distinguish between visible tiles and non-visible tiles, not between regular and pre-paint.
> 
> Yes, I'm with you on that. FYI, with my paint-in-parallel-with-upload patch what's visible and not is unknown to the impl thread when the new uploads start taking place.
> 
> > 
> > The use case I'm thinking of is that if the impl side scrolls, a pre-painted tile may become a visible tile and should have a higher priority for upload than a regular tile that's now offscreen.  In other words, the main thread hands over content, and the impl side decides how best to use that content based on the current state of the world.
> 
> Sounds good. Painting a few extra tiles which we don't know if they will be visible or not is starting to make sense to me know. I guess the hard part is deciding which ones to paint.
> 
> > 
> > > 2. The impl thread will be much better at predicting what tiles to pre-render.
> > 
> > I think both threads have an equivalent state of the world with respect to prerendering prediction.  They know scroll history on a per layer basis.  They can sync over animation data once scroll gestures are hooked up to that and know where layers will scroll in the future.  I've been seeing them as equivalent.  Am I missing something that makes the impl thread have a much more up-to-date view?
> 
> I was thinking of information about whatever smooth scrolling animations that could be taking place on the impl thread side. You know this stuff much better than me so if think the impl thread doesn't have much to offer in form of pre-render prediction, neither do I :) That also makes things much easier.
> 
> One last question about this patch. I haven't looked at the details of this patch yet but did you guys consider re-factoring the layer painting so that we could use the exact same code path for painting pre-render tiles as regular tiles. Maybe by replacing setVisibleRect with setRenderRect? In what ways do the pre-render tiles need to be treated differently than regular tiles within TiledLayerChromium?

Externally it could look the same, but internally pre-painting uses a lot of extra logic to decide what to paint. Whereas the visible paint just paints the entire visible dirty area, pre-painting tries to avoid massive paints (eg from a diagonal scroll), and does the painting over the course of several commits.  This way if the state changes during any of the idle commits it can bail out and start over with the highest priority pre-painting on the next commit.

[David Reveman]

(In reply to comment #27)
> (In reply to comment #24)
> 
> I'd also argue we can't really do anything from the impl thread since we can't paint anything from there. We might know sooner that we need to pre-paint, but we can't do anything with that information until webkit is ready to paint something. 

My question was, are we not interested in passing this information from the impl thread to the main thread and use it for predicting tiles to pre-render. I guess the answer is no.

> 
> Regarding predictive painting, currently it just stupidly favors top/bottom, but potentially we could pass it a hint to tell it what direction to favor etc. This could come from the scroll direction/velocity. 

Yes, this is exactly what I was referring to. This kind of hint (which I'm now convinced wouldn't be coming from the impl thread) is coming from somewhere. It's a hint that is used for a paint pass. It doesn't seem to me like something that belong as part of the Layer. Which is why I'm wondering if the area to pre-paint shouldn't be controlled from somewhere else than inside the TileLayerChromium implementation itself.

> 
> > (In reply to comment #23)
> > > What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> > > 
> > > I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> > > 
> > > 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
> > 
> > I don't think this is correct - the impl thread does not do any sort of 'prioritization' of tiles today. All eviction and painting decisions are made on the main thread.
> > 
> > > 2. The impl thread will be much better at predicting what tiles to pre-render.
> > 
> > I disagree here as well, the impl thread might be better at prediction how much to pre-render but it can't make accurate decisions on a tile or even on a layer basis since the layer tree may change between commits.
> > 
> > > 
> > > Isn't that correct? Based on those assumptions it seem like this pre-rendering is best driven from CCThreadProxy and the TiledLayerChromium implementation being unaware if it's rendering regular or pre-render tiles.

[David Reveman]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #26)
> > > (In reply to comment #23)
> > > > What's the reason this pre-painting is currently controlled from within the TiledLayerChromium class?
> > > > 
> > > > I'm asking because when I think of pre-painting of tiles I make these two assumptions:
> > > > 
> > > > 1. We eventually want the impl thread to distinguish between regular tiles and pre-paint tiles so that it can prioritize them differently.
> > > 
> > > I've been thinking about this in a slightly different way.  The impl side needs to distinguish between visible tiles and non-visible tiles, not between regular and pre-paint.
> > 
> > Yes, I'm with you on that. FYI, with my paint-in-parallel-with-upload patch what's visible and not is unknown to the impl thread when the new uploads start taking place.
> > 
> > > 
> > > The use case I'm thinking of is that if the impl side scrolls, a pre-painted tile may become a visible tile and should have a higher priority for upload than a regular tile that's now offscreen.  In other words, the main thread hands over content, and the impl side decides how best to use that content based on the current state of the world.
> > 
> > Sounds good. Painting a few extra tiles which we don't know if they will be visible or not is starting to make sense to me know. I guess the hard part is deciding which ones to paint.
> > 
> > > 
> > > > 2. The impl thread will be much better at predicting what tiles to pre-render.
> > > 
> > > I think both threads have an equivalent state of the world with respect to prerendering prediction.  They know scroll history on a per layer basis.  They can sync over animation data once scroll gestures are hooked up to that and know where layers will scroll in the future.  I've been seeing them as equivalent.  Am I missing something that makes the impl thread have a much more up-to-date view?
> > 
> > I was thinking of information about whatever smooth scrolling animations that could be taking place on the impl thread side. You know this stuff much better than me so if think the impl thread doesn't have much to offer in form of pre-render prediction, neither do I :) That also makes things much easier.
> > 
> > One last question about this patch. I haven't looked at the details of this patch yet but did you guys consider re-factoring the layer painting so that we could use the exact same code path for painting pre-render tiles as regular tiles. Maybe by replacing setVisibleRect with setRenderRect? In what ways do the pre-render tiles need to be treated differently than regular tiles within TiledLayerChromium?
> 
> Externally it could look the same, but internally pre-painting uses a lot of extra logic to decide what to paint. Whereas the visible paint just paints the entire visible dirty area, pre-painting tries to avoid massive paints (eg from a diagonal scroll), and does the painting over the course of several commits.  This way if the state changes during any of the idle commits it can bail out and start over with the highest priority pre-painting on the next commit.

I wondering if we couldn't make it look the same internally, and instead look different externally instead of the other way around. Basically have all this extra logic be outside the layer implementation.

[Adrienne Walker]

(In reply to comment #25)
> (From update of attachment 117316 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117316&action=review
> 
> Thanks for all the feedback! Regarding the texture manager changes, if possible I'd really like to leave that for another CL.

Thinking out loud--given that we only reserve a single-tile apron around the viewport, we're not going to go too far over the soft reclamation limit.  I'm not that happy with the LRU behavior this causes, but maybe it's ok to evict previously visible content to make scrolling faster. We probably need a larger discussion about what sort of LRU behavior the texture manager needs.

I'd been thinking that it was necessary to have those texture manager changes first, but I think I've convinced myself that this could land first.

> >> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:355
> >> +                m_skipsIdlePaint = true;
> > 
> > What happened to the idea of removing m_skipsIdlePaint and setting the paint rect to empty here? If you don't set the paint rect to empty here, you're going to get an assert in updateCompositorResources if you've painted any tiles.
> > 
> > On that note, can you add a test that involves idle painting hitting the memory limit?
> 
> Sorry, I misunderstood and thought you were okay with it.
> 
> The empty paint-rect isn't sufficient to replace m_skipsIdlePaint. We need to detect if we are done idle painting in needsIdlePaint(). The function would always return true for an empty paint-rect, resulting in infinite SetNeedsCommit calls. m_skipsIdlePaint clarifies that we don't need to paint anymore, even though there is plenty of dirty tiles remaining outside the current paint rect. 

Ok, sure--sorry for bringing this quibble back up.  Using the flag is fine as-is, although I'd still love a test where we hit the texture limit and stop idle painting so that nobody breaks this in the future.  :)

> Also, note that we don't set m_requestedUpdateRect or m_paintRect if we return early from this function. This will result in early exit from updateCompositorResources on the first line.

Oh, quite right.  Sorry for my misunderstanding.

[Adrienne Walker]

(In reply to comment #31)
> (In reply to comment #27)

> > Regarding predictive painting, currently it just stupidly favors top/bottom, but potentially we could pass it a hint to tell it what direction to favor etc. This could come from the scroll direction/velocity. 
> 
> Yes, this is exactly what I was referring to. This kind of hint (which I'm now convinced wouldn't be coming from the impl thread) is coming from somewhere. It's a hint that is used for a paint pass. It doesn't seem to me like something that belong as part of the Layer. Which is why I'm wondering if the area to pre-paint shouldn't be controlled from somewhere else than inside the TileLayerChromium implementation itself.

It's a hint that the layer could calculate itself, I think.  It knows historical scroll positions.  It could be aware of how it's being animated, but that system doesn't exist yet so I'm just making that up.

(In reply to comment #32)
> (In reply to comment #30)
> > Externally it could look the same, but internally pre-painting uses a lot of extra logic to decide what to paint. Whereas the visible paint just paints the entire visible dirty area, pre-painting tries to avoid massive paints (eg from a diagonal scroll), and does the painting over the course of several commits.  This way if the state changes during any of the idle commits it can bail out and start over with the highest priority pre-painting on the next commit.
> 
> I wondering if we couldn't make it look the same internally, and instead look different externally instead of the other way around. Basically have all this extra logic be outside the layer implementation.

I would like to do it this way, but a prerequisite is making painting into an SkPicture the default path.  That way we don't have to pay the rasterization costs for a very large paint rect.

However, getting prepainting in the codebase to make scrolling faster is a more immediate priority.  I didn't want to require writing a paint aggregator to handle multiple paint regions that we'd just throw away when we moved to your SkPicture-based painting as the default.  So, to solve this in the short term, prepainting only happens when other painting hasn't happened (which the layer knows about) and prepainting only paints a row or column (to prevent large regions of unneeded rasterization).

I think my future view of prepainting (insert handwaving here) is that something else would hand the layer a visible rect and a prepaint rect.  The union of these could be painted into an SkPicture.  Then the layer itself could selectively decide which of these to rasterize, based on proximity to the visible rect and whether or not tiles are valid or not and maybe also based on some scheduling decision of how much rasterization to do.

[James Robinson]

Comment on attachment 117316 [details]
Patch

This looks good to me. It looks like Enne had one more outstanding comment that you were intending to follow up on. Also, this patch didn't apply cleanly on to ToT on the cr-linux EWS bot, so I suspect it doesn't apply to ToT currently.

Eric - can you address that last comment and upload a new patch that's rebaselined to ToT so the bots can verify it?  Then I think we're ready to land.

[Eric Penner]

(In reply to comment #34)
> (In reply to comment #31)
> > (In reply to comment #27)
> 
> > > Regarding predictive painting, currently it just stupidly favors top/bottom, but potentially we could pass it a hint to tell it what direction to favor etc. This could come from the scroll direction/velocity. 
> > 
> > Yes, this is exactly what I was referring to. This kind of hint (which I'm now convinced wouldn't be coming from the impl thread) is coming from somewhere. It's a hint that is used for a paint pass. It doesn't seem to me like something that belong as part of the Layer. Which is why I'm wondering if the area to pre-paint shouldn't be controlled from somewhere else than inside the TileLayerChromium implementation itself.
> 
> It's a hint that the layer could calculate itself, I think.  It knows historical scroll positions.  It could be aware of how it's being animated, but that system doesn't exist yet so I'm just making that up.
> 

On a related note, I initially planned on setting the pre-paint rect from somewhere outside TiledLayerChromium, but it wasn't necessary yet because it could be trivially calculated internally. Also, as Enne mentioned, without the SkPicture solution we still need most of the code internally to chop up the paint etc.

Also, if we improve the texture manager, pre-painting prediction can stay dead simple by just expanding to fill the entire memory reclaim limit, with no external hints required, as there would be lots of pre-painted padding for scrolling.

> (In reply to comment #32)
> > (In reply to comment #30)
> > > Externally it could look the same, but internally pre-painting uses a lot of extra logic to decide what to paint. Whereas the visible paint just paints the entire visible dirty area, pre-painting tries to avoid massive paints (eg from a diagonal scroll), and does the painting over the course of several commits.  This way if the state changes during any of the idle commits it can bail out and start over with the highest priority pre-painting on the next commit.
> > 
> > I wondering if we couldn't make it look the same internally, and instead look different externally instead of the other way around. Basically have all this extra logic be outside the layer implementation.
> 
> I would like to do it this way, but a prerequisite is making painting into an SkPicture the default path.  That way we don't have to pay the rasterization costs for a very large paint rect.
> 
> However, getting prepainting in the codebase to make scrolling faster is a more immediate priority.  I didn't want to require writing a paint aggregator to handle multiple paint regions that we'd just throw away when we moved to your SkPicture-based painting as the default.  So, to solve this in the short term, prepainting only happens when other painting hasn't happened (which the layer knows about) and prepainting only paints a row or column (to prevent large regions of unneeded rasterization).
> 
> I think my future view of prepainting (insert handwaving here) is that something else would hand the layer a visible rect and a prepaint rect.  The union of these could be painted into an SkPicture.  Then the layer itself could selectively decide which of these to rasterize, based on proximity to the visible rect and whether or not tiles are valid or not and maybe also based on some scheduling decision of how much rasterization to do.

[Eric Penner]

(In reply to comment #35)
> (From update of attachment 117316 [details])
> This looks good to me. It looks like Enne had one more outstanding comment that you were intending to follow up on. Also, this patch didn't apply cleanly on to ToT on the cr-linux EWS bot, so I suspect it doesn't apply to ToT currently.
> 
> Eric - can you address that last comment and upload a new patch that's rebaselined to ToT so the bots can verify it?  Then I think we're ready to land.

Will upload shortly. Should I rebase off of webkit master? Or is the gclient branch okay?

[James Robinson]

master

[David Reveman]

(In reply to comment #34)
> (In reply to comment #31)
> > (In reply to comment #27)
> 
> > > Regarding predictive painting, currently it just stupidly favors top/bottom, but potentially we could pass it a hint to tell it what direction to favor etc. This could come from the scroll direction/velocity. 
> > 
> > Yes, this is exactly what I was referring to. This kind of hint (which I'm now convinced wouldn't be coming from the impl thread) is coming from somewhere. It's a hint that is used for a paint pass. It doesn't seem to me like something that belong as part of the Layer. Which is why I'm wondering if the area to pre-paint shouldn't be controlled from somewhere else than inside the TileLayerChromium implementation itself.
> 
> It's a hint that the layer could calculate itself, I think.  It knows historical scroll positions.  It could be aware of how it's being animated, but that system doesn't exist yet so I'm just making that up.

Ok, for some reason I felt like this system and these calculations would not belong in the layer itself. But I guess that was mainly based on the false assumption that we would be using information provided by the impl thread.

> 
> (In reply to comment #32)
> > (In reply to comment #30)
> > > Externally it could look the same, but internally pre-painting uses a lot of extra logic to decide what to paint. Whereas the visible paint just paints the entire visible dirty area, pre-painting tries to avoid massive paints (eg from a diagonal scroll), and does the painting over the course of several commits.  This way if the state changes during any of the idle commits it can bail out and start over with the highest priority pre-painting on the next commit.
> > 
> > I wondering if we couldn't make it look the same internally, and instead look different externally instead of the other way around. Basically have all this extra logic be outside the layer implementation.
> 
> I would like to do it this way, but a prerequisite is making painting into an SkPicture the default path.  That way we don't have to pay the rasterization costs for a very large paint rect.
> 
> However, getting prepainting in the codebase to make scrolling faster is a more immediate priority.  I didn't want to require writing a paint aggregator to handle multiple paint regions that we'd just throw away when we moved to your SkPicture-based painting as the default.  So, to solve this in the short term, prepainting only happens when other painting hasn't happened (which the layer knows about) and prepainting only paints a row or column (to prevent large regions of unneeded rasterization).

Ah, OK. Sounds like we have the same design in mind for the long term. Didn't know this was such an immediate priority. The per-tile painting definitely simplifies all this.

> 
> I think my future view of prepainting (insert handwaving here) is that something else would hand the layer a visible rect and a prepaint rect.  The union of these could be painted into an SkPicture.  Then the layer itself could selectively decide which of these to rasterize, based on proximity to the visible rect and whether or not tiles are valid or not and maybe also based on some scheduling decision of how much rasterization to do.

Sounds good. This is very much along the lines of how I imagined it would work.

[Adrienne Walker]

(In reply to comment #39)

> Ah, OK. Sounds like we have the same design in mind for the long term. Didn't know this was such an immediate priority. The per-tile painting definitely simplifies all this.

Oh, good.  I'm glad we're on the same page.

Regarding timing: Making per-tile painting the default requires getting Skia-on-Mac to stick and fixing some Skia bugs in SkPicture at least.  Even then, we'll probably want to live with both texture uploaders for a while so that we can safely turn it off if it kicks up too much dust.  I think there is general agreement for making per-tile painting the default path and ditching the current default texture uploader as soon as possible, though.

So, going down that path is probably a ways out, and I think we need prepainting for faster scrolling before that.  We've needed it for a while now, honestly.

[Eric Penner]

Comment on attachment 117316 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117316&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:406
>              ASSERT(layer->opacity());

I just remembered, this is the ASSERT that was occurring during unit tests. The unit test specifically sets opacity to zero during paint. Then for idle paint, this ASSERT is triggered.  I will remove this ASSERT with the above conditional.

[Eric Penner]

Created attachment 117543 [details]
Patch

[Adrienne Walker]

Comment on attachment 117543 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117543&action=review

Thanks for the extra test.  :)

> Source/WebCore/ChangeLog:6
> +        Reviewed by: [e-mail]

Whoa, there.  I'm not a WebKit reviewer. This should say "Reviewed by NOBODY (OOPS!)." When somebody flips the R+ bit on the patch and the commit queue lands it, it will update the ChangeLog automagically.

> Source/WebKit/chromium/ChangeLog:6
> +        Reviewed by: [e-mail]

Same here.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:122
> +    if (needsIdlePaint(layerRect))
> +        setNeedsCommit();

I did some manual testing of this patch, and I believe this is incorrect.  setNeedsCommit needs to be set after the commit has begun in updateCompositorResources, not during paint.  This is a no-op as written.

[Adrienne Walker]

(In reply to comment #43)

> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:122
> > +    if (needsIdlePaint(layerRect))
> > +        setNeedsCommit();
> 
> I did some manual testing of this patch, and I believe this is incorrect.  setNeedsCommit needs to be set after the commit has begun in updateCompositorResources, not during paint.  This is a no-op as written.

If you want to write a test for this behavior, CCLayerTreeHostTest (kind of misnamed, it's really a proxy test) would be a good place for it.

[Eric Penner]

(In reply to comment #43)
> (From update of attachment 117543 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117543&action=review
> 
> Thanks for the extra test.  :)
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by: [e-mail]
> 
> Whoa, there.  I'm not a WebKit reviewer. This should say "Reviewed by NOBODY (OOPS!)." When somebody flips the R+ bit on the patch and the commit queue lands it, it will update the ChangeLog automagically.
> 
> > Source/WebKit/chromium/ChangeLog:6
> > +        Reviewed by: [e-mail]
> 
> Same here.
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:122
> > +    if (needsIdlePaint(layerRect))
> > +        setNeedsCommit();
> 
> I did some manual testing of this patch, and I believe this is incorrect.  setNeedsCommit needs to be set after the commit has begun in updateCompositorResources, not during paint.  This is a no-op as written.

I gave that some thought (if the needs-commit call was late enough), but for me it was consistently pre-painting when I expected (I could tell via logging). Do you think that is just lucky and something else is triggering the additional commits?

[Adrienne Walker]

(In reply to comment #45)

> > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:122
> > > +    if (needsIdlePaint(layerRect))
> > > +        setNeedsCommit();
> > 
> > I did some manual testing of this patch, and I believe this is incorrect.  setNeedsCommit needs to be set after the commit has begun in updateCompositorResources, not during paint.  This is a no-op as written.
> 
> I gave that some thought (if the needs-commit call was late enough), but for me it was consistently pre-painting when I expected (I could tell via logging). Do you think that is just lucky and something else is triggering the additional commits?

I was logging with the threaded compositor enabled and didn't see any idle painting occurring except via commits triggered via scrolling.  I would have expected to see several idle paints in a row without any scrolling-related commits, but that didn't seem to occur.  It's possible I'm mistaken, or that there's a bug somewhere else?  I'll take another look.

A test case would make me more confident that this worked as expected, especially if it ran against both proxies.

[Adrienne Walker]

(In reply to comment #46)

> I was logging with the threaded compositor enabled and didn't see any idle painting occurring except via commits triggered via scrolling.

Ok.  I've convinced myself that setNeedsCommit is in the right place, but there's another bug that prevents it from working properly.

TiledLayerChromium::setNeedsCommit is implemented by LayerChromium::setNeedsCommit.  It calls CCLayerDelegate::notifySyncRequired.  This forwards to GraphicsLayerChromium::notifySyncRequired, which calls GraphicLayerClient::notifySyncRequired.  This forwards to NonCompositedContentHost::notifySyncRequired, which is a no-op.  Sad trombone noise.

Non-root GraphicsLayers have a RenderLayerCompositor as their client, which can talk to the chrome client, which can call WebViewImpl::setRootLayerNeedsDisplay, and then call CCLayerTreeHost::setNeedsCommit.

NonCompositedContentHost::notifySyncRequired somehow needs to forward to CCLayerTreeHost::setNeedsCommit, but I'm not sure how best to connect those pieces.  Maybe jamesr has a thought?

[Eric Penner]

(In reply to comment #47)
> (In reply to comment #46)
> 
> > I was logging with the threaded compositor enabled and didn't see any idle painting occurring except via commits triggered via scrolling.
> 
> Ok.  I've convinced myself that setNeedsCommit is in the right place, but there's another bug that prevents it from working properly.
> 
> TiledLayerChromium::setNeedsCommit is implemented by LayerChromium::setNeedsCommit.  It calls CCLayerDelegate::notifySyncRequired.  This forwards to GraphicsLayerChromium::notifySyncRequired, which calls GraphicLayerClient::notifySyncRequired.  This forwards to NonCompositedContentHost::notifySyncRequired, which is a no-op.  Sad trombone noise.
> 
> Non-root GraphicsLayers have a RenderLayerCompositor as their client, which can talk to the chrome client, which can call WebViewImpl::setRootLayerNeedsDisplay, and then call CCLayerTreeHost::setNeedsCommit.
> 
> NonCompositedContentHost::notifySyncRequired somehow needs to forward to CCLayerTreeHost::setNeedsCommit, but I'm not sure how best to connect those pieces.  Maybe jamesr has a thought?

In my testing it was doing what we expect. I think on "my machine" something is masking this bug.

[James Robinson]

(In reply to comment #47)
> (In reply to comment #46)
> 
> > I was logging with the threaded compositor enabled and didn't see any idle painting occurring except via commits triggered via scrolling.
> 
> Ok.  I've convinced myself that setNeedsCommit is in the right place, but there's another bug that prevents it from working properly.
> 
> TiledLayerChromium::setNeedsCommit is implemented by LayerChromium::setNeedsCommit.  It calls CCLayerDelegate::notifySyncRequired.  This forwards to GraphicsLayerChromium::notifySyncRequired, which calls GraphicLayerClient::notifySyncRequired.  This forwards to NonCompositedContentHost::notifySyncRequired, which is a no-op.  Sad trombone noise.
> 
> Non-root GraphicsLayers have a RenderLayerCompositor as their client, which can talk to the chrome client, which can call WebViewImpl::setRootLayerNeedsDisplay, and then call CCLayerTreeHost::setNeedsCommit.
> 
> NonCompositedContentHost::notifySyncRequired somehow needs to forward to CCLayerTreeHost::setNeedsCommit, but I'm not sure how best to connect those pieces.  Maybe jamesr has a thought?


Ooohh, this is fun and new and exciting :). So historically the only change on the NonCompositedContentHost's stuff that would trigger a new frame is invalidation, which is routed to WebViewImpl which kicks a new frame.  If we want to trigger a commit without any paint damage being done we'll need to wire up a new notification path. I don't think this is too difficult - we just need to wire up a path from NCCH to CCLTH.

Maybe this goes on the painter interface?

[Adrienne Walker]

(In reply to comment #49)
> (In reply to comment #47)
> > (In reply to comment #46)
> > 
> > > I was logging with the threaded compositor enabled and didn't see any idle painting occurring except via commits triggered via scrolling.
> > 
> > Ok.  I've convinced myself that setNeedsCommit is in the right place, but there's another bug that prevents it from working properly.
> > 
> > TiledLayerChromium::setNeedsCommit is implemented by LayerChromium::setNeedsCommit.  It calls CCLayerDelegate::notifySyncRequired.  This forwards to GraphicsLayerChromium::notifySyncRequired, which calls GraphicLayerClient::notifySyncRequired.  This forwards to NonCompositedContentHost::notifySyncRequired, which is a no-op.  Sad trombone noise.
> > 
> > Non-root GraphicsLayers have a RenderLayerCompositor as their client, which can talk to the chrome client, which can call WebViewImpl::setRootLayerNeedsDisplay, and then call CCLayerTreeHost::setNeedsCommit.
> > 
> > NonCompositedContentHost::notifySyncRequired somehow needs to forward to CCLayerTreeHost::setNeedsCommit, but I'm not sure how best to connect those pieces.  Maybe jamesr has a thought?
> 
> 
> Ooohh, this is fun and new and exciting :). So historically the only change on the NonCompositedContentHost's stuff that would trigger a new frame is invalidation, which is routed to WebViewImpl which kicks a new frame.  If we want to trigger a commit without any paint damage being done we'll need to wire up a new notification path. I don't think this is too difficult - we just need to wire up a path from NCCH to CCLTH.

What about having LayerChromium just tell LTH that it needs a commit directly? Ignoring the zoom transform stuff in WVI (which I think we can reroute), I don't think any work gets done along that path.

[Adrienne Walker]

Once bug 73711 lands, this patch will start causing assertions in DumpRenderTree.  The reason is painting will cause more idle painting to be scheduled and WebViewHost::paintInvalidatedRegion only allows this to happen 3 times before asserting that something is amiss.

You can repro this by building DumpRenderTree in debug and running "new-run-webkit-tests --debug --chromium compositing".

I'm not convinced that we should adjust the limits in the test, but we could just modify prepainting to not occur during the compositeAndReadback path.

[Eric Penner]

Created attachment 118421 [details]
Patch

[Eric Penner]

This patch fixes the DumpRenderTree issue but I have only changed CCSingleThreadProxy. I will change CCThreadProxy too, but I wanted to get feedback if this is what you had in mind.

[Eric Penner]

Created attachment 118436 [details]
Patch

[James Robinson]

Comment on attachment 118436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118436&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:217
> +    commitIfNeeded(true);

we use 2-value enums for this instead of bools because it's impossible to guess what 'true' or 'false' mean when looking at the callsite alone.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:301
> +    m_mainThreadProxy->postTask(createBeginFrameAndCommitTaskOnImplThread(true));

same comment here re. enum vs bool. what does 'true' mean here? I can't tell without looking up the definition of cBFACTOIT

[Eric Penner]

(In reply to comment #55)
> (From update of attachment 118436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118436&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:217
> > +    commitIfNeeded(true);
> 
> we use 2-value enums for this instead of bools because it's impossible to guess what 'true' or 'false' mean when looking at the callsite alone.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:301
> > +    m_mainThreadProxy->postTask(createBeginFrameAndCommitTaskOnImplThread(true));
> 
> same comment here re. enum vs bool. what does 'true' mean here? I can't tell without looking up the definition of cBFACTOIT

I will fix that to use an enum if I keep it that way. I'm also not a huge fan of needing the triggerIdlePaints parameter in the Proxy at all, so I've been trying to find a way to contain it in CCLayerTreeHost.

[Eric Penner]

Created attachment 118457 [details]
Patch

[Eric Penner]

I think this addresses everything (and much more simple than the last patch). 

I don't have a test to insure the pre-painting setNeedsCommit is indeed triggering a new commit, but I believe this is covered by this new test that insures setNeedsCommit is working:
https://bugs.webkit.org/show_bug.cgi?id=73711
checkNonCompositedContentPropertyChangeCausesCommit

[Adrienne Walker]

Comment on attachment 118457 [details]
Patch

This looks pretty reasonable to me.  James, do you have more thoughts?

[James Robinson]

Comment on attachment 118457 [details]
Patch

This does seem pretty reasonable. I need to take a more careful pass before officially setting the review flag, will hopefully get to that this weekend.

[Eric Penner]

Created attachment 118678 [details]
Minor_Tweak_Plus_Rebase

[James Robinson]

Comment on attachment 118678 [details]
Minor_Tweak_Plus_Rebase

Looks like a good start. R=me. Would you mind writing up the expected behavior of this somewhere? I believe I understand it today but I will probably forget soon.

[Eric Penner]

I'll write up a description in Google docs if that is okay. 

I just realized there is a potential bug with this CL in out of memory situations. I haven't reproduced it, but in theory it seems possible:

When almost out of memory, a fullscreen layer could evict some old pre-painted tiles (they aren't yet protected), but then run out of memory itself, returning the tiles to be used by pre-painting again.  This could result in endless commits.

Two solutions would be:
1.) Don't return memory when a visible layer runs out of memory
2.) Use my next CL in which pre-painting fails at a lower memory threshold. 

I can't really think of a unit test for this since it requires such specific circumstances. 

(In reply to comment #62)
> (From update of attachment 118678 [details])
> Looks like a good start. R=me. Would you mind writing up the expected behavior of this somewhere? I believe I understand it today but I will probably forget soon.

[Eric Penner]

Created attachment 119526 [details]
Set memory limit for prepainting

[Eric Penner]

Created attachment 119542 [details]
Added fix from 74471 and fixed unit tests

[Eric Penner]

Created attachment 119545 [details]
Rebasing

[James Robinson]

Comment on attachment 119545 [details]
Rebasing

Patch looks good, but the EWS bots say this doesn't apply to ToT. I probably gave you these merge conflicts. Sorry!

Would you mind rebasing+uploading again so the bots can land it?

[Eric Penner]

(In reply to comment #67)
> (From update of attachment 119545 [details])
> Patch looks good, but the EWS bots say this doesn't apply to ToT. I probably gave you these merge conflicts. Sorry!
> 
> Would you mind rebasing+uploading again so the bots can land it?

No prob, I'll rebase again now. I'm also curious if you know a better workflow for this. Last night I noticed it failed, but doing 'gclient sync' and 'sync-webkit-git.py' didn't pull the new changes (in either gclient or master branch).  So I did a 'git pull --rebase origin master' which did pull the new changes, but then webkit-patch upload pulled all the new changes into my patch!  Do you know what how to update the "base" used by webkit-patch upload?

[James Robinson]

(In reply to comment #68)
> (In reply to comment #67)
> > (From update of attachment 119545 [details] [details])
> > Patch looks good, but the EWS bots say this doesn't apply to ToT. I probably gave you these merge conflicts. Sorry!
> > 
> > Would you mind rebasing+uploading again so the bots can land it?
> 
> No prob, I'll rebase again now. I'm also curious if you know a better workflow for this. Last night I noticed it failed, but doing 'gclient sync' and 'sync-webkit-git.py' didn't pull the new changes (in either gclient or master branch).  So I did a 'git pull --rebase origin master' which did pull the new changes, but then webkit-patch upload pulled all the new changes into my patch!  Do you know what how to update the "base" used by webkit-patch upload?

webkit-patch tries to figure out the best base using git merge-base against your svn remote, if it's set, or refs/remotes/origin/master. You can always override it by providing a -g to webkit-patch upload (for example webkit-patch upload -g origin/master..HEAD)

My workflow is to have my 'origin' remote point to git://git.webkit.org/WebKit.git, keep a local branch 'master' that always refers to refs/heads/master without any local changes, and then to update a feature branch I pull to master and then merge master into my branch (or rebase the branch on top of master, if you prefer rebase to merge).

[Eric Penner]

(In reply to comment #69)
> (In reply to comment #68)
> > (In reply to comment #67)
> > > (From update of attachment 119545 [details] [details] [details])
> > > Patch looks good, but the EWS bots say this doesn't apply to ToT. I probably gave you these merge conflicts. Sorry!
> > > 
> > > Would you mind rebasing+uploading again so the bots can land it?
> > 
> > No prob, I'll rebase again now. I'm also curious if you know a better workflow for this. Last night I noticed it failed, but doing 'gclient sync' and 'sync-webkit-git.py' didn't pull the new changes (in either gclient or master branch).  So I did a 'git pull --rebase origin master' which did pull the new changes, but then webkit-patch upload pulled all the new changes into my patch!  Do you know what how to update the "base" used by webkit-patch upload?
> 
> webkit-patch tries to figure out the best base using git merge-base against your svn remote, if it's set, or refs/remotes/origin/master. You can always override it by providing a -g to webkit-patch upload (for example webkit-patch upload -g origin/master..HEAD)
> 
> My workflow is to have my 'origin' remote point to git://git.webkit.org/WebKit.git, keep a local branch 'master' that always refers to refs/heads/master without any local changes, and then to update a feature branch I pull to master and then merge master into my branch (or rebase the branch on top of master, if you prefer rebase to merge).

Thanks, I've got the same origin... It appears to work after doing a plain "git fetch" followed by git rebase origin/master, as opposed to my other method (git pull --rebase origin master). I bet my other method wasn't updating the refs/remotes/origin/master.  Will be uploaded in a sec here.

[Eric Penner]

Created attachment 119637 [details]
rebasing

[Eric Penner]

All green. Ready to go?

(In reply to comment #71)
> Created an attachment (id=119637) [details]
> rebasing

[James Robinson]

Comment on attachment 119637 [details]
rebasing

Yes! Flags flipped, bot should pick it up and land within a few hours.

[WebKit Review Bot]

Comment on attachment 119637 [details]
rebasing

Rejecting attachment 119637 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10900816

[James Robinson]

Comment on attachment 119637 [details]
rebasing

View in context: https://bugs.webkit.org/attachment.cgi?id=119637&action=review

Ah, nope - the ChangeLog formatting is off and the script is very pick about it.

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

You need to remove this line and describe the tests (naming the gtest covering this is good) - this (OOPS!) will fail an svn presubmit hook

You also need to have the "Reviewed by NOBODY (OOPS!)." line still in here. A script will replace this with "Reviewed by James Robinson." since I set the review+ flag.

> Source/WebCore/ChangeLog:9
> +This patch pre-paints a one tile border around the visible tiles when
> +no other visible painting occurs, and when memory is available.

this should be intended

[James Robinson]

(In reply to comment #75)
> this should be intended

s/intended/indented/

[Eric Penner]

Comment on attachment 119637 [details]
rebasing

View in context: https://bugs.webkit.org/attachment.cgi?id=119637&action=review

>> Source/WebCore/ChangeLog:6
>> +        No new tests. (OOPS!)
> 
> You need to remove this line and describe the tests (naming the gtest covering this is good) - this (OOPS!) will fail an svn presubmit hook
> 
> You also need to have the "Reviewed by NOBODY (OOPS!)." line still in here. A script will replace this with "Reviewed by James Robinson." since I set the review+ flag.

Arg. The one time I forgot!  Removing.

>> Source/WebCore/ChangeLog:9
>> +no other visible painting occurs, and when memory is available.
> 
> this should be intended

done.

[Eric Penner]

Created attachment 119666 [details]
Removing oops and indenting

[James Robinson]

Comment on attachment 119666 [details]
Removing oops and indenting

fingers crossed...

[WebKit Review Bot]

Comment on attachment 119666 [details]
Removing oops and indenting

Clearing flags on attachment: 119666

Committed r103129: <http://trac.webkit.org/changeset/103129>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Committed r103154: <http://trac.webkit.org/changeset/103154>