Bug 59020 - [chromium] Fix incorrect scissor rect for layers that render into a rendersurface
Summary: [chromium] Fix incorrect scissor rect for layers that render into a rendersur...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-20 13:22 PDT by Adrienne Walker
Modified: 2011-05-06 12:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.08 KB, patch)
2011-04-20 13:33 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2011-04-22 16:06 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (11.65 KB, patch)
2011-05-04 18:27 PDT, Adrienne Walker
jamesr: review+
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-04-20 13:22:24 PDT
[chromium] Fix incorrect scissor rect for layers that render into a rendersurface
Comment 1 Adrienne Walker 2011-04-20 13:33:05 PDT
Created attachment 90399 [details]
Patch
Comment 2 Adrienne Walker 2011-04-20 13:37:57 PDT
I will point out that those few lines of code are essentially the same as what's in ContentLayerChromium::visibleLayerRect.  However, given that bug 58840 is going to need to refactor this same code again, I saw no reason to try to do surgery and add a common code path for this calculation.

This comes with a test though, so we'll know when we break it next time.  :)
Comment 3 Adrienne Walker 2011-04-22 14:53:16 PDT
Oops, I never CC'd anybody on this bug.  No wonder it didn't get a review.

Vangelis needs this commit for some of the layer sorting work.
Comment 4 James Robinson 2011-04-22 15:12:34 PDT
Comment on attachment 90399 [details]
Patch

The test looks like a good candidate for layoutTestController.dumpAsText(true);
Comment 5 Adrienne Walker 2011-04-22 15:14:22 PDT
It's the scissor rect for drawing that's the problem, not the bounds on the layer itself.  Unless I'm mistaken, that doesn't get dumped?
Comment 6 James Robinson 2011-04-22 15:15:34 PDT
I meant using dumpAsText(true) to avoid creating a render tree dump, since that's irrelevant to the test.  The only useful output from this test is the pixel output.
Comment 7 Adrienne Walker 2011-04-22 15:18:05 PDT
Oh, my mistake.  How does that work for ports that don't do pixel tests? Would this test just be skipped?
Comment 8 James Robinson 2011-04-22 15:21:03 PDT
There's still text output, it's just the dumpAsText() output.  For this test it'd probably be empty and the test would pass (trivially) on platforms that don't run pixel tests.
Comment 9 Adrienne Walker 2011-04-22 16:06:49 PDT
Created attachment 90790 [details]
Patch
Comment 10 Adrienne Walker 2011-04-22 16:07:32 PDT
(In reply to comment #9)
> Created an attachment (id=90790) [details]
> Patch

Oh, great.  Thanks for the suggestion.  :)
Comment 11 Kenneth Russell 2011-04-26 17:11:36 PDT
Comment on attachment 90790 [details]
Patch

Looks fine assuming it's been tested.
Comment 12 Adrienne Walker 2011-04-27 13:21:20 PDT
Committed r85075: <http://trac.webkit.org/changeset/85075>
Comment 13 Adrienne Walker 2011-04-27 14:53:51 PDT
Committed r85100: <http://trac.webkit.org/changeset/85100>
Comment 14 Adrienne Walker 2011-04-27 14:54:19 PDT
This breaks deeply-nested-reflections.  Will investigate and reland.
Comment 15 Adrienne Walker 2011-05-03 08:41:55 PDT
(In reply to comment #14)
> This breaks deeply-nested-reflections.  Will investigate and reland.

Ah, I see what the trouble is.  Before my patch and with my patch it was trying to use the layer bounds as a scissor rect but it really needs to be scissoring to the render surface bounds instead.

Of course, the render surface bounds aren't known until we've iterated through the layer tree and unioned all the layer bounds that are going into that render surface.  However, the scissor rect for each layer is stored on that layer itself, rather than inherited from some higher level.  So, some refactoring is needed to not have to do a "scissor rect update" pass or to only store/set the scissor rect where it's really needed (on clipping layers).
Comment 16 Adrienne Walker 2011-05-04 18:27:17 PDT
Created attachment 92353 [details]
Patch
Comment 17 Kenneth Russell 2011-05-04 18:38:31 PDT
Comment on attachment 92353 [details]
Patch

Vangelis should do the unofficial review of this patch, but my only comment is to make sure the new test doesn't turn any of the other platforms' bots red.
Comment 18 Adrienne Walker 2011-05-04 18:43:02 PDT
Ok, here's another try.  updatePropertiesEtcEtcEtc is kind of entangled and hard to follow, so it took me a while to sort out how best to fix this.  I have some refactoring ideas though that can wait until after this patch.

Here's a quick summary of this change.  Prior to this bug, the scissor rect for some layers was being set incorrectly.  It just happened to work out that in the majority of cases the scissor was large enough to not clip anything accidentally, but in some cases it did, which is demonstrated by the new test case.

Setting the scissor rect based on the layer bounds as both the previous patch and the code before that patch tried to do is incorrect as the bounds of a layer don't contain its sublayers.  Instead, it really needs to be set to the render surface size.  However, this size is unfortunately not calculated until after we're done with the whole recursive updatePropertiesEtcEtcEtc call.

So instead, when rendering into a render surface, the scissor rect for the layer subtree that's going into that render surface gets cleared, because it no longer applies.  An empty scissor implies no scissor at all.  The fallout from that is a number of places where we need to check for empty scissor rects and use the render surface size as the visible portion of the layer instead.
Comment 19 Adrienne Walker 2011-05-04 18:44:34 PDT
(In reply to comment #17)
> (From update of attachment 92353 [details])
> Vangelis should do the unofficial review of this patch, but my only comment is to make sure the new test doesn't turn any of the other platforms' bots red.

As Chromium is the only port that does pixel tests and there's no text output from this test, this should work.  I ran it on OSX with Safari a while back and it ran cleanly.  It also didn't break anything but Chromium tests when it landed before, so I'm reasonably confident about it.
Comment 20 Vangelis Kokkevis 2011-05-05 11:07:16 PDT
Comment on attachment 92353 [details]
Patch

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

One small comment from me, otherwise LG.  Thanks for fixing this up.

> LayoutTests/platform/chromium/test_expectations.txt:3984
> +BUGENNE GPU : compositing/flat-with-transformed-child.html = FAIL MISSING

Since you are adding a baseline for linux, shouldn't the be PASS FAIL MISSING or maybe better use MAC WINDOWS qualifiers

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:912
> +    bool isLayerVisible = layer->scissorRect().isEmpty() || layer->scissorRect().intersects(layerRect);

Don't we need to check against the renderSurface's contentRect if the scissorRect is empty?
Comment 21 James Robinson 2011-05-05 15:27:50 PDT
Comment on attachment 92353 [details]
Patch

R=me - please address Vangelis' comments before landing.
Comment 22 Adrienne Walker 2011-05-06 10:41:56 PDT
Committed r85959: <http://trac.webkit.org/changeset/85959>
Comment 23 Adrienne Walker 2011-05-06 12:22:36 PDT
Committed r85970: <http://trac.webkit.org/changeset/85970>