Bug 77498 - [Chromium] Occlusion tracking with CSS filters
Summary: [Chromium] Occlusion tracking with CSS filters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-31 17:45 PST by Dana Jansens
Modified: 2012-02-16 18:28 PST (History)
9 users (show)

See Also:


Attachments
Patch (43.50 KB, patch)
2012-02-13 19:08 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (43.42 KB, patch)
2012-02-16 11:46 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (44.66 KB, patch)
2012-02-16 16:47 PST, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-01-31 17:45:38 PST
- 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.
Comment 1 Dana Jansens 2012-02-13 19:08:36 PST
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 2 James Robinson 2012-02-13 22:37:48 PST
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?
Comment 3 Dana Jansens 2012-02-14 07:47:02 PST
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.
Comment 4 Dana Jansens 2012-02-14 07:51:36 PST
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 5 Stephen White 2012-02-14 11:05:16 PST
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.
Comment 6 Dana Jansens 2012-02-14 11:55:21 PST
(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!
Comment 7 James Robinson 2012-02-15 17:12:47 PST
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 8 Stephen White 2012-02-16 07:05:41 PST
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 9 Dana Jansens 2012-02-16 10:54:42 PST
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 10 Stephen White 2012-02-16 11:10:00 PST
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.  :)
Comment 11 Dana Jansens 2012-02-16 11:46:31 PST
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 12 Adrienne Walker 2012-02-16 13:58:08 PST
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?
Comment 13 Dana Jansens 2012-02-16 14:24:05 PST
(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).
Comment 14 Adrienne Walker 2012-02-16 16:17:11 PST
(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 15 James Robinson 2012-02-16 16:35:37 PST
Comment on attachment 127422 [details]
Patch

That works for me!
Comment 16 Dana Jansens 2012-02-16 16:42:34 PST
Comment on attachment 127422 [details]
Patch

Thanks for R!

Going to move the blur expected.png files to platform/chromium as requested by senorblanco.
Comment 17 Dana Jansens 2012-02-16 16:47:35 PST
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 18 Stephen White 2012-02-16 17:01:48 PST
Comment on attachment 127466 [details]
Patch

Thanks for the fixes (and the tests).  r=me
Comment 19 WebKit Review Bot 2012-02-16 18:28:14 PST
Comment on attachment 127466 [details]
Patch

Clearing flags on attachment: 127466

Committed r108013: <http://trac.webkit.org/changeset/108013>
Comment 20 WebKit Review Bot 2012-02-16 18:28:19 PST
All reviewed patches have been landed.  Closing bug.