Bug 63499 – Overlap map for compositing should ignore empty layers

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Adrienne Walker]

Overlap map for compositing should ignore empty layers

[Adrienne Walker]

Created attachment 98833 [details]
Patch

[Adrienne Walker]

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.

[Simon Fraser (smfr)]

Did you do archaeology on this one? I know I added that for a reason.

[Adrienne Walker]

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

[Simon Fraser (smfr)]

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

[Adrienne Walker]

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

[Adrienne Walker]

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.

[Simon Fraser (smfr)]

Comment on attachment 98833 [details]
Patch

r-, needs more investigation based on my comments.

[Adrienne Walker]

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

[Adrienne Walker]

Created attachment 130302 [details]
Rebased

[Adrienne Walker]

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

[Simon Fraser (smfr)]

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.

[Adrienne Walker]

Comment on attachment 130302 [details]
Rebased

Thanks, Simon.

[WebKit Review Bot]

Comment on attachment 130302 [details]
Rebased

Clearing flags on attachment: 130302

Committed r109981: <http://trac.webkit.org/changeset/109981>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xianzhu Wang]

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

[Tim Horton]

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.

[Adrienne Walker]

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

[Adrienne Walker]

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

[Adrienne Walker]

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.