[Texmap] Update a dirty region which is not covered with keepRect.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[JungJik Lee]

There is a dirty between out of keepRect and inside the tile rect. In this case, we can have a dirty loss by intersecting keepRect. And the dirty would not be drawn until the tile updates forcibly. So it is better to remove the intersect. because if the setKeepRect works, there will be no tiles out  of keepRect and we already check the invalidate rect inside tile.
I will file a new patch for this.

[JungJik Lee]

Created attachment 196084 [details]
Proposal patch

[JungJik Lee]

Created attachment 196146 [details]
Patch

[JungJik Lee]

Created attachment 196773 [details]
Patch

[JungJik Lee]

Created attachment 196822 [details]
Patch

[JungJik Lee]

I file a test case for this issue.

[Jocelyn Turcotte]

Comment on attachment 196822 [details]
Patch

r- because of the performance regression that this patch would introduce.
It seems like something is going wrong related to m_keepRect, please investigate.

IRC discussion:
10:01 < jturcotte> fnwinter: I'm not sure how it's possible that you have tiles outside the keepRect, could that be the issue instead?
10:02 < jturcotte> fnwinter: Your patch is going to itterate over all the possible tile positions, if you're zoomed in on a large page, this loop can take more than one second to run, you don't want this.
10:06 < fnwinter> jturcotte, it's not exactly outside of keepRect. actually it's between keepRect and tile area. there is a gap area to be missing the dirty.
10:08 < fnwinter> jturcotte, and so first time I thought to inflate the keepRect to 2 tile size. but we are checking the dirty in tile::invalidate function.
10:09 < fnwinter> keepRect does not fit to tiles size, that makes this issue.
10:24 < jturcotte> fnwinter: So normally, the code already there will invalidate all tiles that touches the dirtyRect. tileCoordinateForPoint(innerBottomRight(coveredDirtyRect)) should return the coordinate of the tile to the lower-right even if it touches the keepRect by only one pixel.
10:26 < jturcotte> Another thing that you have to make sure is that all tiles should intersect m_keepRect when invalidate is called. If you keep your patch and add something like ASSERT(currentTile->rect().intersects(m_keepRect)); It shouldn't fire.

[JungJik Lee]

Created attachment 197026 [details]
Patch

[JungJik Lee]

(In reply to comment #7)
> Created an attachment (id=197026) [details]
> Patch
I made a patch not to iterate all tiles.
for not to loss a dirty, I extend m_keepRect to fit tile grid.
I think this wouldn't make the performance regression.

[Jocelyn Turcotte]

Comment on attachment 197026 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        and the dirty will not be invalidated until the tile is updated 

dirty *region*
until the tile is *recreated*

> Source/WebCore/ChangeLog:12
> +        forcibly. We must make keepRect to fit to tile grid to append a

This sentence is difficult to read, suggestion:
"We must expand the keep rect to its intersecting tiles to make sure that the dirty region is applied to existing tiles."

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:85
> +    IntRect keepRectFitToTileSize = tileRectForCoordinate(tileCoordinateForPoint(m_keepRect.location()));
> +    keepRectFitToTileSize.unite(tileRectForCoordinate(tileCoordinateForPoint(innerBottomRight(m_keepRect))));

Ok I see the issue now, this makes sense.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:98
> +            ASSERT(currentTile->rect().intersects(m_keepRect));
> +

This was an investigation suggestion, I think that the ASSERT shouldn't be committed in the code.

> LayoutTests/ChangeLog:9
> +        If we do not miss a dirty outside of keepRect, the red box will be displayed.

If you see red in tests it usually means bad. Please make it a green box.

> LayoutTests/ChangeLog:11
> +        * compositing/tiling/update-tile-outside-keeprect-expected.html: Added.

I'm not sure what this is, you can't have an HTML expected file.

> LayoutTests/compositing/tiling/update-tile-outside-keeprect.html:50
> +        div.keepRect
> +        {
> +            height:1700px;

This kind of hard-coding makes me wonder about the value of this test. At least for Qt, this test only helped me understand your issue, it never showed the bug itself.
Maybe No'am could give his opinion, but I think that you would be better leaving out a test that doesn't test anything.

[JungJik Lee]

Comment on attachment 197026 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        and the dirty will not be invalidated until the tile is updated 
> 
> dirty *region*
> until the tile is *recreated*

first of all, thanks your comment. I'll fix them.

>> LayoutTests/ChangeLog:9
>> +        If we do not miss a dirty outside of keepRect, the red box will be displayed.
> 
> If you see red in tests it usually means bad. Please make it a green box.

Oh, I see. I'll change it to green.

>> LayoutTests/ChangeLog:11
>> +        * compositing/tiling/update-tile-outside-keeprect-expected.html: Added.
> 
> I'm not sure what this is, you can't have an HTML expected file.

it's for a pixel test. if we have a expected html, layouttest will take a png and compare the img with this expected html png. I heard that.

>> LayoutTests/compositing/tiling/update-tile-outside-keeprect.html:50
>> +            height:1700px;
> 
> This kind of hard-coding makes me wonder about the value of this test. At least for Qt, this test only helped me understand your issue, it never showed the bug itself.
> Maybe No'am could give his opinion, but I think that you would be better leaving out a test that doesn't test anything.

I usually test issues in EFL. I think there are some differences. So I will change this test page to work for Qt too. Thank you for the review.

[Jocelyn Turcotte]

(In reply to comment #10)
> it's for a pixel test. if we have a expected html, layouttest will take a png and compare the img with this expected html png. I heard that.

Ok I didn't know about it. The command I used to check if more of these were present in LayoutTests was wrong, sorry about that.

[JungJik Lee]

Created attachment 197264 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 197264 [details]
Patch

r=me, thanks for tracking this down.

[Jocelyn Turcotte]

*** Bug 73920 has been marked as a duplicate of this bug. ***

[JungJik Lee]

(In reply to comment #13)
> (From update of attachment 197264 [details])
> r=me, thanks for tracking this down.

It takes long time to file new patch because I want to check this issue on QtWebkit. Huu it was first-time building QtWebKit. Anyway I was trying to write a test page. However for some reasons, e.g) resizing window, openning popup was not permitted. Qt/EFL uses viewport tag differently in MiniBrowser. I couldn't write a test page. thanks for the review again.

[WebKit Commit Bot]

Comment on attachment 197264 [details]
Patch

Clearing flags on attachment: 197264

Committed r148094: <http://trac.webkit.org/changeset/148094>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.