Bug 77498 – [Chromium] Occlusion tracking with CSS filters

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

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

[Dana Jansens]

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!

[James Robinson]

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?

[Dana Jansens]

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.

[Dana Jansens]

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

[Stephen White]

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.

[Dana Jansens]

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

[James Robinson]

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.

[Stephen White]

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)?

[Dana Jansens]

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.

[Stephen White]

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.  :)

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

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

[Adrienne Walker]

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

[James Robinson]

Comment on attachment 127422 [details]
Patch

That works for me!

[Dana Jansens]

Comment on attachment 127422 [details]
Patch

Thanks for R!

Going to move the blur expected.png files to platform/chromium as requested by senorblanco.

[Dana Jansens]

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.

[Stephen White]

Comment on attachment 127466 [details]
Patch

Thanks for the fixes (and the tests).  r=me

[WebKit Review Bot]

Comment on attachment 127466 [details]
Patch

Clearing flags on attachment: 127466

Committed r108013: <http://trac.webkit.org/changeset/108013>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.