[chromium] Don't occlude pixels in a surface that are needed for a background filter blur
This catches #1 of the following 4 pieces: 1) Background filter blur needs valid drawn pixels (in the render surface) in a radius out from the pixels it reads. 2) Foreground filter blur needs valid painted pixels in a radius out from the pixels it will display. 3) Background filter blur moves damage in a parent around inside its child with a background blur up to the blur radius. 4) Foreground filter blur moves damage around in a surface up to the blur radius. Others will be separate CLs
Created attachment 137838 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 137838 [details] Patch Public API changes look fine (it's just updating a comment), the other change looks a bit beyond me.
Comment on attachment 137838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137838&action=review Just to check my understanding: we walk layers from front to back. Because readback filters read layers that have drawn underneath, we haven't actually visited those layers yet. So, you clear the occlusion from the the entire area that the filter could pull pixels from, in the hope that if the other layers completely opaquely fill that area, then we won't need gutter quads to fill that space. (I say gutter quads, since that's the only thing that cares about occlusion on the root layer, and only surfaces drawing into the root layer have background filters.) I think this is right for the edge cases of the entire under-filter area being opaque or non-opaque, but is incorrect if the region under the filter is only partially opaque? It seems to me like that partially opaque region needs to be deflated by the outsets to be correct. > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:184 > + removeOcclusionOverMovedPixels(oldTarget, newTarget, oldTarget->originTransform()); Why does this happen after merging the stack down? Opaque regions on a layer with a background filter should still apply to the layer below and not be erased by the removeOcclusion part?
(In reply to comment #5) > (From update of attachment 137838 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137838&action=review > > Just to check my understanding: we walk layers from front to back. Because readback filters read layers that have drawn underneath, we haven't actually visited those layers yet. Yes. > So, you clear the occlusion from the the entire area that the filter could pull pixels from, in the hope that if the other layers completely opaquely fill that area, then we won't need gutter quads to fill that space. (I say gutter quads, since that's the only thing that cares about occlusion on the root layer, and only surfaces drawing into the root layer have background filters.) I hadn't actually thought about gutter quads. But you're right, if under this layer's opaque stuff things are not opaque, we'll end up including gutter quads in the readback as those are drawn at the back of the root surface. > I think this is right for the edge cases of the entire under-filter area being opaque or non-opaque, but is incorrect if the region under the filter is only partially opaque? It seems to me like that partially opaque region needs to be deflated by the outsets to be correct. I am unsure what you mean here. We remove occlusion at the point of just below the blurring layer, so that we ensure pixels you can see through it are not considered occluded, and get drawn. I don't see how the opaque-ness/occlusion of things below this blurred layer matters at all here.. If things are opaque then they'll occlude other stuff that is further below the blurring layer. But that also implies that those pixels will be drawn below the blurring layer. This may cause more pixels to be drawn than is strictly needed but that's okay. > > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:184 > > + removeOcclusionOverMovedPixels(oldTarget, newTarget, oldTarget->originTransform()); > > Why does this happen after merging the stack down? Opaque regions on a layer with a background filter should still apply to the layer below and not be erased by the removeOcclusion part? The layer itself should not occlude the layer below in the area that it needs to read pixels from, nothing should occlude that. We need to ensure that it gets painted and drawn so we are able to read it back correctly. Think left half of layer is opaque, right half clear with background blur. It will need to read pixels from under the left half in the surface below, so those pixels are not occluded from view even though they are below this layers opaque half. But since the left half is opaque, it doesn't cause any pixels below it to become unoccluded, only the right half does.
Comment on attachment 137838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137838&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:184 >>> + removeOcclusionOverMovedPixels(oldTarget, newTarget, oldTarget->originTransform()); >> >> Why does this happen after merging the stack down? Opaque regions on a layer with a background filter should still apply to the layer below and not be erased by the removeOcclusion part? > > The layer itself should not occlude the layer below in the area that it needs to read pixels from, nothing should occlude that. We need to ensure that it gets painted and drawn so we are able to read it back correctly. > > Think left half of layer is opaque, right half clear with background blur. It will need to read pixels from under the left half in the surface below, so those pixels are not occluded from view even though they are below this layers opaque half. > > But since the left half is opaque, it doesn't cause any pixels below it to become unoccluded, only the right half does. Oh, so since we end up removing everything under the layer, it's maybe strange to do this after the merge. But in theory we could take the inverse of the occlusion under the oldTarget (so just its unoccluded part), expand that, and subtract that from occlusion in the new target. Maybe we should do that..
(In reply to comment #6) > > > I think this is right for the edge cases of the entire under-filter area being opaque or non-opaque, but is incorrect if the region under the filter is only partially opaque? It seems to me like that partially opaque region needs to be deflated by the outsets to be correct. > > I am unsure what you mean here. We remove occlusion at the point of just below the blurring layer, so that we ensure pixels you can see through it are not considered occluded, and get drawn. I don't see how the opaque-ness/occlusion of things below this blurred layer matters at all here.. > > If things are opaque then they'll occlude other stuff that is further below the blurring layer. But that also implies that those pixels will be drawn below the blurring layer. This may cause more pixels to be drawn than is strictly needed but that's okay. Here's my example: You have a viewport (0, 0, 20, 20). The root doesn't draw content (oops). The first child of the root is an opaque layer with absolute rect (5, 5, 10, 10). The second child of the root is a layer rect (0, 0, 20, 20) that perfectly covers the root and has a blur of 2 pixels radius. What's the final occlusion? With the algorithm in your patch, it seems like you'd unocclude everything from the layer with the background blur, then visit the other layer and apply the occlusion from the opaque layer underneath because you're walking front to back. The final occlusion is (5, 5, 10, 10), just the occlusion from the opaque layer. However...the final pixels in the viewport are all blurred because of the background filter, so really the only fully opaque values are in the rect (7, 7, 6, 6). I guess I'm saying that I'm wondering if it makes sense to just keep an ordered list of background filter rects until you've processed everything and *then* touch up the occlusion (somehow, if needed).
(In reply to comment #8) > (In reply to comment #6) > > > > > I think this is right for the edge cases of the entire under-filter area being opaque or non-opaque, but is incorrect if the region under the filter is only partially opaque? It seems to me like that partially opaque region needs to be deflated by the outsets to be correct. > > > > I am unsure what you mean here. We remove occlusion at the point of just below the blurring layer, so that we ensure pixels you can see through it are not considered occluded, and get drawn. I don't see how the opaque-ness/occlusion of things below this blurred layer matters at all here.. > > > > If things are opaque then they'll occlude other stuff that is further below the blurring layer. But that also implies that those pixels will be drawn below the blurring layer. This may cause more pixels to be drawn than is strictly needed but that's okay. > > Here's my example: You have a viewport (0, 0, 20, 20). The root doesn't draw content (oops). The first child of the root is an opaque layer with absolute rect (5, 5, 10, 10). The second child of the root is a layer rect (0, 0, 20, 20) that perfectly covers the root and has a blur of 2 pixels radius. > > What's the final occlusion? > > With the algorithm in your patch, it seems like you'd unocclude everything from the layer with the background blur, then visit the other layer and apply the occlusion from the opaque layer underneath because you're walking front to back. The final occlusion is (5, 5, 10, 10), just the occlusion from the opaque layer. > > However...the final pixels in the viewport are all blurred because of the background filter, so really the only fully opaque values are in the rect (7, 7, 6, 6). > > I guess I'm saying that I'm wondering if it makes sense to just keep an ordered list of background filter rects until you've processed everything and *then* touch up the occlusion (somehow, if needed). Yehhhh ok I see what you mean, thanks! We need to blur unoccluded areas out as well. I think it's important to determine what you need as you walk front to back, as the code queries the occlusion tracker at that time, not later. We probably need to store a list of background blur rects and apply them to shrink occlusion on everything below them.
Created attachment 139301 [details] Patch
Created attachment 139322 [details] Patch rebase
Comment on attachment 139322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139322&action=review I know my last comment on this bug suggested that occlusion underneath a filter also needed to be reduced, but I have reconvinced myself that's not actually the case. I think the algorithm should be the following: when leaving a surface with a background filter, do the reduceOcclusion() pass for the outsets/clip rect for the current filter. I think that's it; the filter info shouldn't be saved or reapplied. Here's why: if you have a background filter covering the entire page and underneath it, two opaque layers of exactly the same size and position, the top opaque layer *should* cover the bottom opaque layer. And, the only way this could happen is if occlusion for the top opaque layer wasn't reduced. If the order were switched to be top opaque layer, filter layer, bottom opaque layer, then the top opaque layer should be reduced when the filter layer is finished, because the bototm layer in this case isn't entirely occluded. What do you think? > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:230 > + occlusionReducedRect.pixelExpandedInTarget = filterOutsetsInTarget; > + occlusionReducedRect.effectedAreaInTarget = boundsInTarget; > + occlusionReducedRect.pixelExpandedInScreen = filterOutsetsInScreen; > + occlusionReducedRect.effectedAreaInScreen = boundsInScreen; (Not that it's wrong to be general here, but screen == target always, because of the way background filters are limited to only apply to surfaces with the default surface as a target?) > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:233 > +static inline void reduceOcclusion(const IntRect& effectedArea, const IntRect& pixelExpanded, Region& occlusion) :%s/effected/affected/g
Comment on attachment 139322 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139322&action=review Ok right. So things below the blur all get blurred together as one operation. So you're right, the two opaque layers below should have their edges blurred together, and only when leaving the surface. To make it general what should happen is: 1. On blur, blur all occlusion above and including the surface with blur. 2. Save the blur area. 3. On leaving the target surface, apply all saved blur areas to the surface's occlusion. Because this only happens on the root surface right now, 3. never happens. Should we just ignore 2 and 3 then? >> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:230 >> + occlusionReducedRect.effectedAreaInScreen = boundsInScreen; > > (Not that it's wrong to be general here, but screen == target always, because of the way background filters are limited to only apply to surfaces with the default surface as a target?) Yeh, tho this code doesn't know about the restriction. Are you asking to make this explicit or just clarifying for yourself. I'd rather keep this code general and not knowing about that.. but then, above comment is related. >> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:233 >> +static inline void reduceOcclusion(const IntRect& effectedArea, const IntRect& pixelExpanded, Region& occlusion) > > :%s/effected/affected/g ahhh. *>_<*
Created attachment 139666 [details] Patch
Alright patch updated! This reflects our acquired knowledge along the way to this point, making a pretty clean CL I think. We reduce all occlusion up to and including the contributing surface when we encounter a background blur filter. Hopefully this code makes lots of sense to you! I like that it's much simpler than last incarnation, and only requires file-static helper methods.
Comment on attachment 139666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139666&action=review Can you add one more test where the background filter is entirely occluded by something above it and the resulting occlusion isn't reduced? > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:159 > +static inline void reduceOcclusion(const IntRect& affectedArea, const IntRect& expandedPixel, Region& occlusion) Some comment about what "expandedPixel" means here would be helpful. > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2759 > + // Save the distance of influence for the blur effect. > + int outsetTop, outsetRight, outsetBottom, outsetLeft; > + filters.getOutsets(outsetTop, outsetRight, outsetBottom, outsetLeft); You don't seem to use this in this test.
Comment on attachment 139666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139666&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:159 >> +static inline void reduceOcclusion(const IntRect& affectedArea, const IntRect& expandedPixel, Region& occlusion) > > Some comment about what "expandedPixel" means here would be helpful. k >> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2759 >> + filters.getOutsets(outsetTop, outsetRight, outsetBottom, outsetLeft); > > You don't seem to use this in this test. yes thanks, since this is now testing non-reduction instead.
Created attachment 139905 [details] Patch The diff got larger due to a small API change on occlusion tracker. unoccludedContributingSurfaceContentRect(Layer*) became unoccludedContributingSurfaceContentRect(Surface*). It used to need the Layer* but Surface* has all the data it needs these days anyways. This lets us figure out which part of the contributing surface's background filter will be used, so I can write that unit test you asked for. There's a huge regex change in the CCOTTest.cpp because of the API change, to pass in the surface instead of the layer though. Here's the diff minus the test.cpp http://pastebin.com/iW4Q8bdS Added 2 new tests. One where the filter is completely occluded, and one where it is partially occluded.
Comment on attachment 139905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139905&action=review R=me, with a few small things to change before landing. Thanks for all the iterations. :) > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2743 > + // Make a surface and its replica, each 50x50, that are completely surrounded by opaque layers which are above them in the z-order. Not really true here. > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2792 > + // Make a surface and its replica, each 50x50, that are completely surrounded by opaque layers which are above them in the z-order. s/surrounded/occluded/ > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2840 > + // Make a surface and its replica, each 50x50, that are completely surrounded by opaque layers which are above them in the z-order. s/surrounded/partially occluded/ > Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2902 > + EXPECT_EQ_RECT(occlusionBesideSurface, targetRects[0]); > + EXPECT_EQ_RECT(occlusionBesideSurface, screenRects[0]); > + EXPECT_EQ_RECT(occlusionBesideReplica, targetRects[1]); > + EXPECT_EQ_RECT(occlusionBesideReplica, screenRects[1]); > + EXPECT_EQ_RECT(occlusionAboveSurface, targetRects[2]); > + EXPECT_EQ_RECT(occlusionAboveSurface, screenRects[2]); > + EXPECT_EQ_RECT(occlusionAboveReplica, targetRects[3]); > + EXPECT_EQ_RECT(occlusionAboveReplica, screenRects[3]); I don't think the ordering of rects in Region is something you should depend on. Maybe construct a region yourself with these rects and see if it equals occlusionInFooSpace?
Comment on attachment 139905 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139905&action=review thanks :) >> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:2902 >> + EXPECT_EQ_RECT(occlusionAboveReplica, screenRects[3]); > > I don't think the ordering of rects in Region is something you should depend on. Maybe construct a region yourself with these rects and see if it equals occlusionInFooSpace? yeh sounds good.
Created attachment 139930 [details] Patch for landing
Comment on attachment 139930 [details] Patch for landing Clearing flags on attachment: 139930 Committed r115930: <http://trac.webkit.org/changeset/115930>
All reviewed patches have been landed. Closing bug.