- Only drop occlusion leaving subtrees with a filter that actually changes opaque pixels to not be. - Only drop occlusion entering subtrees with a filter that moves pixels around in a target.
Created attachment 126887 [details] Patch This catches the interaction between the new impl filters and occlusion tracking during paint. The pieces most relevant to reviewers are, I think: @senorblanco The filters part of this (especially which filters can change alpha values in pixels and which can move color values from one pixel to another). @jamesr The remaining changes, which are to use knowledge of filters on surfaces to affect occlusion. Thanks!
Comment on attachment 126887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126887&action=review I don't quite follow the 'nearest moves pixels surface' logic - why can't we use the same sort of logic we do for opacity / masks ? > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:112 > + // The nearest target surface that this surface will be drawn into and that contains a what does "nearest" mean? most immediate target?
The idea is when you enter a subtree of a RenderSurface that moves pixels around, you have to drop any occlusion from outside that surface. If you want from a render surface up to the root, the 'nearest filter that moves pixels' is the first render surface you would see with a filter on it that moves pixels around in the surface. The problem case is demonstrated in the blur layout tests, but here's a description of it. Here's an example. We have RenderSurfaces A, Bf, Cf. Bf and Cf have different blur filters on them. A has none. Consider the layer tree something like this, with numbers for layers, in order from back to front: A ----------- 1 A ----------- 2 Bf ------ 3 Bf ------ 4 Cf -- 5 A ----------- 6 6 is the front-most layer and it paints into A. 5 sits below 6 and paints into Cf. Cf paints into Bf which is directly below it. Bf eventually paints into A. Before this code, layer 6 could occlude 5, since it is above it. However since 5 gets blurred, it is possible that which 5 is occluded by 6, it becomes visible in Cf after being blurred. When the occlusion tracking entered Cf, it sees a new blur subtree, and discards occlusion from outside the subtree - occlusion from 6. Now when it enters Bf, which has a different blur filter, the same problem can occur again - 5 may occlude 4, but the blur in Bf makes 4 visible. So we discard occlusion from outside the Bf subtree - where the subtree is bounded from below by other blur filters. Here's a tree view of things in case more clarification is needed. A /|\ / 1 2 / | / Bf | / \ | 3 4 | | | Cf | | | 5 6 The 'nearest filter that moves pixels' breaks this tree into subtrees of unique blur filters: A /|\ / 1 2 / | ========= / Bf | / \ | 3 4 | | ===== | Cf | | | 5 6 When passing one of these ====== boundaries from the front (from below in this tree), we prevent any occlusion collected so far from being used within that blur section of the tree.
Sorry the ===== lines I drew there are wrong. The blur filter causes the tree to be broken as follows: **** marks a blur filter, and the top of a 'nearest filter' subtree. ==== marks the bottom of the 'nearest filter' subtree. Occlusion from outside one of these regions should not affect things inside. A /|\ / 1 2 / | ********* / Bf | / \ | 3 4 | | ======== **** | Cf | | | 5 ==== 6
Comment on attachment 126887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126887&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:247 > + bool useSurfaceForFilters = !layer->filters().isEmpty(); FYI: shawnsingh@ is refactoring this code in https://bugs.webkit.org/show_bug.cgi?id=78539. (Just a heads up in case he lands first.) > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:371 > + bool drawsContent = calculateDrawTransformsAndVisibilityInternal<LayerType, RenderSurfaceType, LayerSorter>(child, rootLayer, sublayerMatrix, nextHierarchyMatrix, nearestFilterThatMovesPixels, renderSurfaceLayerList, descendants, layerSorter, maxTextureSize); Not your fault, but IWBN to move all of these parameters into some kind of context struct. Even by WebKit standards, nine parameters and three template parameters is quite a few. > Source/WebCore/platform/graphics/filters/FilterOperation.h:83 > + // True if the alpha channel of any pixel can change under this operation. > + virtual bool affectsOpacity() const { return false; } > + // True if the the value of one pixel can affect the value of another pixel under this operation, such as blur. > + virtual bool movesPixels() const { return false; } (this comment should be at line 136, but bugzilla won't let me comment there) ReferenceFilter refers to an arbitrary SVG filter network, so it could do either one (affect opacity or move pixels). For now, the safest thing would be for ReferenceFilter return true for both. However, note that it's currently unimplemented, so it won't be testable right now. > Source/WebCore/platform/graphics/filters/FilterOperation.h:208 > + virtual bool affectsOpacity() const { return true; } > + Only opacity() affects opacity AFAICT; invert, brightness and contrast should leave it unchanged. What you have is fine, since it's conservative, but I think you could return m_type == OPACITY here.
(In reply to comment #5) > (From update of attachment 126887 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126887&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:247 > > + bool useSurfaceForFilters = !layer->filters().isEmpty(); > > FYI: shawnsingh@ is refactoring this code in https://bugs.webkit.org/show_bug.cgi?id=78539. (Just a heads up in case he lands first.) Thanks, yep I'm watching it :) > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:371 > > + bool drawsContent = calculateDrawTransformsAndVisibilityInternal<LayerType, RenderSurfaceType, LayerSorter>(child, rootLayer, sublayerMatrix, nextHierarchyMatrix, nearestFilterThatMovesPixels, renderSurfaceLayerList, descendants, layerSorter, maxTextureSize); > > Not your fault, but IWBN to move all of these parameters into some kind of context struct. Even by WebKit standards, nine parameters and three template parameters is quite a few. I do agree, but I think I should leave this to Shawn's larger refactoring. > > Source/WebCore/platform/graphics/filters/FilterOperation.h:83 > > + // True if the alpha channel of any pixel can change under this operation. > > + virtual bool affectsOpacity() const { return false; } > > + // True if the the value of one pixel can affect the value of another pixel under this operation, such as blur. > > + virtual bool movesPixels() const { return false; } > > (this comment should be at line 136, but bugzilla won't let me comment there) > > ReferenceFilter refers to an arbitrary SVG filter network, so it could do either one (affect opacity or move pixels). For now, the safest thing would be for ReferenceFilter return true for both. However, note that it's currently unimplemented, so it won't be testable right now. Ah, I meant to catch that one, thanks! > > Source/WebCore/platform/graphics/filters/FilterOperation.h:208 > > + virtual bool affectsOpacity() const { return true; } > > + > > Only opacity() affects opacity AFAICT; invert, brightness and contrast should leave it unchanged. What you have is fine, since it's conservative, but I think you could return m_type == OPACITY here. Cool, I wasn't sure, will do!
If Stephen or Enne want to double-check the iteration logic here that'd be great and I'd be happy to review the rest. That stuff just hurts my brain.
Comment on attachment 126887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126887&action=review >> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:112 >> + // The nearest target surface that this surface will be drawn into and that contains a > > what does "nearest" mean? most immediate target? I agree that this could use a better name. Would "topmost" make sense? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:512 > + bool enteringFilterThatMovesPixelsSubtree = newTarget->nearestFilterThatMovesPixels() != oldFilterThatMovesPixels; Can it happen that nearestFilterThatMovesPixels() is NULL, but the oldFilter isn't? Is it still valid to say that we're enteringFilterThatMovesPixelsSubtree in that case? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:516 > stack.append(RenderSurfaceRegion()); > stack.last().surface = newTarget; > int lastIndex = stack.size() - 1; It's not new to this patch, but this manual stack manipulation seems a little error-prone to me (and I admit I don't fully understand it). I'm guessing that we can't use the system stack here, since the layers are being traversed in Z rather than in graph order? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:518 > + stack[lastIndex].occludedInScreen = stack[lastIndex - 1].occludedInScreen; Can lastIndex can be zero here (ie., stack size of 1)?
Comment on attachment 126887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126887&action=review >>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:112 >>> + // The nearest target surface that this surface will be drawn into and that contains a >> >> what does "nearest" mean? most immediate target? > > I agree that this could use a better name. Would "topmost" make sense? Topmost isn't right since tree structure and z-order are different.. You could have a sibling "above" you in the tree (below you in the display) with a blur, but you don't care about. What you want is the nearest layer, walking up to the root, that draws pixels belonging to this surface, and that moves pixels. I thought of it as the nearest ancestor of the surface (inclusively), which is where nearest came from. So.. how about "nearest ancestor"? Is that more clear? Maybe "first pixel movement" or something along those lines..? >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:512 >> + bool enteringFilterThatMovesPixelsSubtree = newTarget->nearestFilterThatMovesPixels() != oldFilterThatMovesPixels; > > Can it happen that nearestFilterThatMovesPixels() is NULL, but the oldFilter isn't? Is it still valid to say that we're enteringFilterThatMovesPixelsSubtree in that case? You're right. If nearestAncestorFilterThatMovesPixels() is NULL, then we have no blurring to worry about here, as leaving a blur is handled elsewhere. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:516 >> int lastIndex = stack.size() - 1; > > It's not new to this patch, but this manual stack manipulation seems a little error-prone to me (and I admit I don't fully understand it). I'm guessing that we can't use the system stack here, since the layers are being traversed in Z rather than in graph order? Thanks for this. I'm going to think about if its possible with the interation of LayerIterators to do this, and will consider it for another CL. >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:518 >> + stack[lastIndex].occludedInScreen = stack[lastIndex - 1].occludedInScreen; > > Can lastIndex can be zero here (ie., stack size of 1)? No. We know the stack was not empty entering the function, and we have just appended something onto it, so the size is >= 2.
Comment on attachment 126887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126887&action=review >>>> Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.h:112 >>>> + // The nearest target surface that this surface will be drawn into and that contains a >>> >>> what does "nearest" mean? most immediate target? >> >> I agree that this could use a better name. Would "topmost" make sense? > > Topmost isn't right since tree structure and z-order are different.. You could have a sibling "above" you in the tree (below you in the display) with a blur, but you don't care about. > > What you want is the nearest layer, walking up to the root, that draws pixels belonging to this surface, and that moves pixels. > > I thought of it as the nearest ancestor of the surface (inclusively), which is where nearest came from. So.. how about "nearest ancestor"? Is that more clear? > > Maybe "first pixel movement" or something along those lines..? I like nearestAncestor, since it indicates that it's hierarchy and not z-order that matters. "nearest" could mean either. I would just use nearestAncestorThatMovesPixels(), since it's not really important that it's a filter, just that it moves pixels, although I leave it open for further bikeshedding. :)
Created attachment 127422 [details] Patch Addressed comments. I'm good with nearestAncestorThatMovesPixels(), thanks for saving me from 10 more characters. I moved the 'hasFilterThatMovesPixels()' and 'hasFilterThatAffectsOpacity()' methods from RenderSurfaceChromium and CCRenderSurface into FilterOperations, so the code only needs to exist once. Also, rebased to TOT.
Comment on attachment 126887 [details] Patch I see that this patch is only for the paint culling side of things, but you also have layout tests. How are filters not affecting the draw culling side of things too?
(In reply to comment #12) > (From update of attachment 126887 [details]) > I see that this patch is only for the paint culling side of things, but you also have layout tests. How are filters not affecting the draw culling side of things too? Draw culling only considers layers within a single target. Since filters make a new RenderSurface, it just skips the whole ordeal (for now).
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 126887 [details] [details]) > > I see that this patch is only for the paint culling side of things, but you also have layout tests. How are filters not affecting the draw culling side of things too? > > Draw culling only considers layers within a single target. Since filters make a new RenderSurface, it just skips the whole ordeal (for now). Oh, quite right! Excellent. This looks good to me. I don't really have any other feedback.
Comment on attachment 127422 [details] Patch That works for me!
Comment on attachment 127422 [details] Patch Thanks for R! Going to move the blur expected.png files to platform/chromium as requested by senorblanco.
Created attachment 127466 [details] Patch Moved the blur test result to platform/chromium, and marked the tests as potentially failing for gardeners, so we can rebaseline.
Comment on attachment 127466 [details] Patch Thanks for the fixes (and the tests). r=me
Comment on attachment 127466 [details] Patch Clearing flags on attachment: 127466 Committed r108013: <http://trac.webkit.org/changeset/108013>
All reviewed patches have been landed. Closing bug.