Bug 78962 - [Qt][WK2] Draw tiles of previous contents-scale for non-opaque layers if they don't intersect with tiles in new content-scale
Summary: [Qt][WK2] Draw tiles of previous contents-scale for non-opaque layers if they...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-18 05:13 PST by Noam Rosenthal
Modified: 2012-02-21 15:33 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.33 KB, patch)
2012-02-20 11:09 PST, Noam Rosenthal
kenneth: review+
Details | Formatted Diff | Diff
Patch (2.27 KB, patch)
2012-02-21 10:46 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-02-18 05:13:41 PST
[Qt][WK2] Draw tiles of previous contents-scale for opaque layers if they don't intersect with previous tiles
Comment 1 Noam Rosenthal 2012-02-20 11:09:20 PST
Created attachment 127834 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-02-20 13:13:41 PST
Comment on attachment 127834 [details]
Patch

I don't get it fully. Maybe previous tile is a too overloaded word.
Comment 3 Noam Rosenthal 2012-02-20 15:10:58 PST
To reproduce, open the morphing-cubes demo in Qt's MiniBrowser, and change the content scale.
The cube sides always disappear and then reappear - that's because they're painted with semi-transparency and the tiles from the previous contents scale are discarded before the new ones are ready.
Comment 4 Kenneth Rohde Christiansen 2012-02-21 02:55:37 PST
Yeah I understand the issue, but just not disregarding ones not intersecting sounds like it only solves this in some cases. What do you do when they infact do intersect and you still don't have the new content ready? clip it?
Comment 5 Kenneth Rohde Christiansen 2012-02-21 08:45:17 PST
Comment on attachment 127834 [details]
Patch

Would be nice with a few better comments in the code, but r=me
Comment 6 Jocelyn Turcotte 2012-02-21 08:52:55 PST
Comment on attachment 127834 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:120
> +        coveredRect.unite(tile.rect());

Why do you need to count the previous scale tiles in the covered rect?
Comment 7 Noam Rosenthal 2012-02-21 08:57:49 PST
(In reply to comment #6)
> (From update of attachment 127834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127834&action=review
> 
> > Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:120
> > +        coveredRect.unite(tile.rect());
> 
> Why do you need to count the previous scale tiles in the covered rect?

hmm... I guess that line is unnecessary.
Comment 8 Noam Rosenthal 2012-02-21 09:00:01 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 127834 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=127834&action=review
> > 
> > > Source/WebKit2/UIProcess/qt/LayerBackingStore.cpp:120
> > > +        coveredRect.unite(tile.rect());
> > 
> > Why do you need to count the previous scale tiles in the covered rect?
> 
> hmm... I guess that line is unnecessary.

Though if things are fast enough there might be a situation where we have previous tiles from several scales... in that case we would still want to discard them if there's opacity.
Comment 9 Jocelyn Turcotte 2012-02-21 09:12:24 PST
(In reply to comment #8)
> Though if things are fast enough there might be a situation where we have previous tiles from several scales... in that case we would still want to discard them if there's opacity.

I think that would be weird, since we should only send createTile while rendering tiles and normally at this point the previous backing store should already have sent all the removeTile messages for its tiles, once its smart pointer was reset.

It would also mean that you would discard tiles randomly from one scale or the other depending on which one sets the coveredRect first, and mix them. So I think we should avoid it if it really can happen.
Comment 10 Noam Rosenthal 2012-02-21 09:35:13 PST
(In reply to comment #9)
> (In reply to comment #8)
> > Though if things are fast enough there might be a situation where we have previous tiles from several scales... in that case we would still want to discard them if there's opacity.
> 
> I think that would be weird, since we should only send createTile while rendering tiles and normally at this point the previous backing store should already have sent all the removeTile messages for its tiles, once its smart pointer was reset.
> 
> It would also mean that you would discard tiles randomly from one scale or the other depending on which one sets the coveredRect first, and mix them. So I think we should avoid it if it really can happen.

OK, I see what you mean. But that line is harmless, and I use it in the next patch to make sure we clip if we painted tiles that are not contained in the targetRect.
Otherwise I need to calculate two coverRects, which seems like an overkill.

Any objection to submitting the patch as is?
Comment 11 Jocelyn Turcotte 2012-02-21 09:50:46 PST
(In reply to comment #10)
> Any objection to submitting the patch as is?

Sorry no problem, I just wanted to make sure that it has more use than it's costing :)
Comment 12 Noam Rosenthal 2012-02-21 10:46:13 PST
Created attachment 127996 [details]
Patch
Comment 13 WebKit Review Bot 2012-02-21 15:33:35 PST
Comment on attachment 127996 [details]
Patch

Clearing flags on attachment: 127996

Committed r108408: <http://trac.webkit.org/changeset/108408>
Comment 14 WebKit Review Bot 2012-02-21 15:33:40 PST
All reviewed patches have been landed.  Closing bug.