Bug 72333 – [Chromium] Clear tile update rectangle properly before each update.

• Component: WebCore Misc.
• Status: RESOLVED DUPLICATE

[David Reveman]

UpdatableTile::m_updateRect needs to be cleared for each tile before starting a new update. TiledLayerChromium::updateCompositorResources should only use m_updateRect to determine the tiles that need updating.

[David Reveman]

Created attachment 115068 [details]
Patch

[James Robinson]

Comment on attachment 115068 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No new tests. Covered by the existing tests.

which tests cover this change?

[David Reveman]

(In reply to comment #2)
> (From update of attachment 115068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests. Covered by the existing tests.
> 
> which tests cover this change?

I'm working on layout test that will expose this problem. This shouldn't have been marked for review yet. Sorry.

[James Robinson]

OK that sounds great, just checking :)

[Adrienne Walker]

Comment on attachment 115068 [details]
Patch

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

Sorry for all the questions, but can you explain the changes to clearDirty() and createTile() as well? I don't quite follow why those need to get moved around.  I think if you expect tiles to be there, then there should be asserts about it.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:325
> +    for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) {
> +        UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get());
> +        tile->m_updateRect = IntRect();
> +    }
> +

Why not just clear the update rect in updateCompositorResources as you iterate through the tiles with non-zero update rects, rather than every tile on the layer?

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:382
> -            if (!tile)
> -                tile = createTile(i, j);
> -            else if (!tile->dirty())
> +            if (!tile || !tile->dirty())

How does this not break the default texture updater? If you don't create new tiles and consider them dirty, then how does m_paintRect get constructed to include new tiles?

[Nat Duca]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 115068 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review
> > 
> > > Source/WebCore/ChangeLog:13
> > > +        No new tests. Covered by the existing tests.
> > 
> > which tests cover this change?
> 
> I'm working on layout test that will expose this problem.

Is this unit testable?

[David Reveman]

(In reply to comment #5)
> (From update of attachment 115068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review
> 
> Sorry for all the questions, but can you explain the changes to clearDirty() and createTile() as well? I don't quite follow why those need to get moved around.  I think if you expect tiles to be there, then there should be asserts about it.

The idea is that we compute the paint area and the tile areas to update in prepareToUpdate. The result ends up in m_paintRect and UpdatableTile::m_updateRect. Once this is done, tiles can be invalidated without it affecting the behavior of the next call to updateCompositorResource. This is the reason for the move of clearDiry().

I intentionally didn't change the expectation of a tile's existence in this patch, though it's probably safe to do so. However, creating a tile when it doesn't exist is wrong in these cases which is why I included the changes related to that in this patch.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:325
> > +    for (CCLayerTilingData::TileMap::const_iterator iter = m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) {
> > +        UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get());
> > +        tile->m_updateRect = IntRect();
> > +    }
> > +
> 
> Why not just clear the update rect in updateCompositorResources as you iterate through the tiles with non-zero update rects, rather than every tile on the layer?

That works too. I was a bit forward looking here and changed it in a way that will work with my patch that allows initialization of compositor resources prior to painting. That patch turns updateCompositorResources into forEachCompositorResource and it's not necessarily called just once during an updated. I can move the clear into updateCompositorResources for now if that's preferred?

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:382
> > -            if (!tile)
> > -                tile = createTile(i, j);
> > -            else if (!tile->dirty())
> > +            if (!tile || !tile->dirty())
> 
> How does this not break the default texture updater? If you don't create new tiles and consider them dirty, then how does m_paintRect get constructed to include new tiles?

This is the second pass iterating over all tiles that might need updating. The first pass that create tiles if necessary is before the textureUpdater()->prepareToUpdate() call. Is there a chance that more tiles are dirty after the call to textureUpdater()->prepareToUpdate()? In that case we could allow the creation of them in the second pass but we can't call clearDirty() on them as m_paintRect might not include all of the dirty tile rect. In that case we need to subtract the m_paintRect from the tile's dirty rect.

The patch is not correct here. We need to either have an ASSERT(tile) or create tiles and subtract m_paintRect from the tile's dirty rect.

[Adrienne Walker]

Comment on attachment 115068 [details]
Patch

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

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 115068 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115068&action=review
> > 
> > Sorry for all the questions, but can you explain the changes to clearDirty() and createTile() as well? I don't quite follow why those need to get moved around.  I think if you expect tiles to be there, then there should be asserts about it.
> 
> The idea is that we compute the paint area and the tile areas to update in prepareToUpdate. The result ends up in m_paintRect and UpdatableTile::m_updateRect. Once this is done, tiles can be invalidated without it affecting the behavior of the next call to updateCompositorResource. This is the reason for the move of clearDiry().

It's my assumption that there'd be no invalidations once prepareToUpdate was called until after updateCompositorResources, so was wondering if there was a correctness issue you were addressing that I didn't quite understand.
 
> I intentionally didn't change the expectation of a tile's existence in this patch, though it's probably safe to do so. However, creating a tile when it doesn't exist is wrong in these cases which is why I included the changes related to that in this patch.

Oh, I see it now.  Because you're setting more state on these tiles, lazy creation will cause problems.

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:325
>>> +
>> 
>> Why not just clear the update rect in updateCompositorResources as you iterate through the tiles with non-zero update rects, rather than every tile on the layer?
> 
> That works too. I was a bit forward looking here and changed it in a way that will work with my patch that allows initialization of compositor resources prior to painting. That patch turns updateCompositorResources into forEachCompositorResource and it's not necessarily called just once during an updated. I can move the clear into updateCompositorResources for now if that's preferred?

No, not at all.  "Why" was just an honest question here.  If this meshes better with where you're going, then by all means.  :)

>>> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:382
>>> +            if (!tile || !tile->dirty())
>> 
>> How does this not break the default texture updater? If you don't create new tiles and consider them dirty, then how does m_paintRect get constructed to include new tiles?
> 
> This is the second pass iterating over all tiles that might need updating. The first pass that create tiles if necessary is before the textureUpdater()->prepareToUpdate() call. Is there a chance that more tiles are dirty after the call to textureUpdater()->prepareToUpdate()? In that case we could allow the creation of them in the second pass but we can't call clearDirty() on them as m_paintRect might not include all of the dirty tile rect. In that case we need to subtract the m_paintRect from the tile's dirty rect.
> 
> The patch is not correct here. We need to either have an ASSERT(tile) or create tiles and subtract m_paintRect from the tile's dirty rect.

Oops.  I entirely misread this diff, then.  Ignore my entirely incorrect worry about this breaking things.  Yeah, an assert in both of these places where it's not ok to lazily create tiles would be excellent.

For what it's worth, perhaps this should be better documented, but there should only be one case in which m_paintRect does not include all of the dirty tile rect: edge tiles on image layers.  It's a hack to just consider those tiles clean once they've been partially uploaded, but the layer is entirely invalidated when its bounds change.  That's why clearDirty() is called anyway in the old code, even though m_updateRect != dirtyRect.  The alternative is to have more complicated per-tile dirty regions or tiles of different sizes, but I just didn't want to go there.

[David Reveman]

Fixed in the original patch that was reverted.

*** This bug has been marked as a duplicate of bug 71388 ***