Bug 63499 - Overlap map for compositing should ignore empty layers
Summary: Overlap map for compositing should ignore empty layers
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
: 78943 (view as bug list)
Depends on: 84630
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-27 17:51 PDT by Adrienne Walker
Modified: 2013-04-08 11:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.36 KB, patch)
2011-06-27 18:05 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Rebased (5.75 KB, patch)
2012-03-05 23:12 PST, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-06-27 17:51:31 PDT
Overlap map for compositing should ignore empty layers
Comment 1 Adrienne Walker 2011-06-27 18:05:40 PDT
Created attachment 98833 [details]
Patch
Comment 2 Adrienne Walker 2011-06-27 18:07:45 PDT
While I'm looking at overlap issues, I might as well clean this tiny little wart too.

As far as I can tell, there's no reason to expand the bounds of empty layers to be 1x1 in the overlap map.  It just causes layers to be composited when they don't need to be.  These layers should be skipped, as anything overlapping them shouldn't cause compositing.
Comment 3 Simon Fraser (smfr) 2011-06-27 18:22:26 PDT
Did you do archaeology on this one? I know I added that for a reason.
Comment 4 Adrienne Walker 2011-06-28 10:59:54 PDT
(In reply to comment #3)
> Did you do archaeology on this one? I know I added that for a reason.

It came from here: https://bugs.webkit.org/show_bug.cgi?id=34065. The test added with that patch continues to pass without any assertions, at least on Chromium Linux and Safari Snow Leopard.

I still do not understand that bug fix.  If the embed is empty, overlaps shouldn't be composited.  If the embed is not empty, then (1,1) is probably the wrong size.
Comment 5 Simon Fraser (smfr) 2011-06-28 11:06:55 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > Did you do archaeology on this one? I know I added that for a reason.
> 
> It came from here: https://bugs.webkit.org/show_bug.cgi?id=34065. The test added with that patch continues to pass without any assertions, at least on Chromium Linux and Safari Snow Leopard.
> 
> I still do not understand that bug fix.  If the embed is empty, overlaps shouldn't be composited.  If the embed is not empty, then (1,1) is probably the wrong size.

Ah, https://bugs.webkit.org/show_bug.cgi?id=34065#c4 tries to explain the issue. I would expect that changing this would cause the assertion to start firing again.
Comment 6 Adrienne Walker 2011-06-29 17:41:03 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Did you do archaeology on this one? I know I added that for a reason.
> > 
> > It came from here: https://bugs.webkit.org/show_bug.cgi?id=34065. The test added with that patch continues to pass without any assertions, at least on Chromium Linux and Safari Snow Leopard.
> > 
> > I still do not understand that bug fix.  If the embed is empty, overlaps shouldn't be composited.  If the embed is not empty, then (1,1) is probably the wrong size.
> 
> Ah, https://bugs.webkit.org/show_bug.cgi?id=34065#c4 tries to explain the issue. I would expect that changing this would cause the assertion to start firing again.

It doesn't start firing again with this change.  The clip rects and roots for all layers in the second test case from bug 34065 are identical with and without this patch.  The clip rects and roots for all layers in empty-embed-rects are also identical with and without this patch.
Comment 7 Adrienne Walker 2011-07-13 15:59:01 PDT
smfr: I don't quite follow the explanation in that comment and how that particular fix solves the issue.

If you think bug 34065 is still an issue, can you make a test case that fails? The test you submitted with that patch passes with and without the code that went with it right now.
Comment 8 Simon Fraser (smfr) 2011-12-21 13:53:33 PST
Comment on attachment 98833 [details]
Patch

r-, needs more investigation based on my comments.
Comment 9 Adrienne Walker 2011-12-21 14:00:21 PST
(In reply to comment #8)
> (From update of attachment 98833 [details])
> r-, needs more investigation based on my comments.

Are you saying that you're going to investigate this, or that I should?

I've seen this overlap entry create a lot of extra unneeded layers on Google Maps.
Comment 10 Adrienne Walker 2012-03-05 23:12:03 PST
Created attachment 130302 [details]
Rebased
Comment 11 Adrienne Walker 2012-03-05 23:37:33 PST
(In reply to comment #10)
> Created an attachment (id=130302) [details]
> Rebased

smfr: I'd like to get this change landed.  It continues to cause extra unnecessary layers to be created on a number of sites due to overlap.

I feel like I've done a fair amount of investigation already on this bug.  Removing these lines of code does not cause the assert to start triggering again in Chromium or in Safari.  As I mentioned earlier, the clip rects and clip roots for both tests attached to bug 34065 are identical with and without this patch.

If you believe this code is still useful and shouldn't be removed, can you give me any pointers to write a test case that would fail? It's hard to propose an alternate solution when all of the existing test cases continue to pass without this code.
Comment 12 Simon Fraser (smfr) 2012-03-06 12:55:04 PST
Comment on attachment 130302 [details]
Rebased

OK, let's land it and see if anything breaks. Keep an eye out for new assertions related to clip rects etc.
Comment 13 Adrienne Walker 2012-03-06 15:33:57 PST
Comment on attachment 130302 [details]
Rebased

Thanks, Simon.
Comment 14 WebKit Review Bot 2012-03-06 17:06:09 PST
Comment on attachment 130302 [details]
Rebased

Clearing flags on attachment: 130302

Committed r109981: <http://trac.webkit.org/changeset/109981>
Comment 15 WebKit Review Bot 2012-03-06 17:06:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Xianzhu Wang 2012-03-14 14:32:46 PDT
*** Bug 78943 has been marked as a duplicate of this bug. ***
Comment 17 Tim Horton 2012-04-23 13:49:06 PDT
It looks like this caused https://bugs.webkit.org/show_bug.cgi?id=84558; I'm going to roll it out for now. Keep this regression in mind when fixing the patch.
Comment 18 Adrienne Walker 2012-05-15 17:07:47 PDT
(In reply to comment #17)
> It looks like this caused https://bugs.webkit.org/show_bug.cgi?id=84558; I'm going to roll it out for now. Keep this regression in mind when fixing the patch.

My patch changed which render layers became composited that just happened to unearth a repaint issue for one port on one platform.  Reverting this patch is just papering over the real problem.

smfr, timothy_horton: please make a smaller repro case so that this repainting problem can be fixed.  My patch was fixing real issues with too many layers being created on certain sites and I'd like to re-land it and not be blocked by Safari Lion repainting bugs.
Comment 19 Adrienne Walker 2012-07-26 15:25:37 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > It looks like this caused https://bugs.webkit.org/show_bug.cgi?id=84558; I'm going to roll it out for now. Keep this regression in mind when fixing the patch.
> 
> My patch changed which render layers became composited that just happened to unearth a repaint issue for one port on one platform.  Reverting this patch is just papering over the real problem.
> 
> smfr, timothy_horton: please make a smaller repro case so that this repainting problem can be fixed.  My patch was fixing real issues with too many layers being created on certain sites and I'd like to re-land it and not be blocked by Safari Lion repainting bugs.

smfr, thorton: Have you had a chance to investigate this bug and create a smaller repro case? I still believe that this is a repaint problem with Safari's canvas implementation only and would like to re-land this patch.
Comment 20 Adrienne Walker 2013-04-08 11:29:09 PDT
smfr, thorton: If you want this patch, please feel free to land it yourself.  I am marking this as WONTFIX based on the previous regression that it was rolled out for.