Bug 84479 – [chromium] Some background filters require inflating damage on the surface behind them

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Some background filters require inflating damage on the surface behind them

[Dana Jansens]

Created attachment 138145 [details]
Patch

[Shawn Singh]

Comment on attachment 138145 [details]
Patch


Very awesome that you caught this problem before it bites =)  Here are my thoughts... otherwise it LGTM.


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

> Source/WebCore/ChangeLog:3
> +        [chromium] Some background filters require inflating damage on the surface behind them

"on the surface behind them" is probably misleading here... =)   If I understood correctly, should it say "Some background filters require inflating damage that occurs behind it"

> Source/WebCore/ChangeLog:9
> +        surface below it. We extend the damage tracker to expand damage in a

I think same is true for these two sentences, too.  sounds like we're expanding damage on the surface, where the surface is below the layer

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:64
> +static inline bool expandPixelOutsetsWithFilters(IntRect& pixelOutsets, const FilterOperations& filters)
> +{
> +    if (!filters.hasFilterThatMovesPixels())
> +        return false;
> +

Personally I would prefer not to have a function that has two simultaneous purposes like this.  Could we separate this into two helper functions:
  bool needsToExpandDamage()
  void expandPixelOutsetsForFilters()

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:79
> +static inline bool expandDamageRectWithFilters(FloatRect& damageRect, const FilterOperations& filters)

Does this need to be a boolean?  It seems not to be used that way, and making it void we can remove that similar "side-effecty two purposes" feeling from it.

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:206
> +        if (contributingSurface && expandPixelOutsetsWithFilters(pixelOutsets, layer->backgroundFilters())) {

Do we need to check whether this is actually a contributing renderSurface versus a contributing layer?   (for example, if for some wacky wild reason, we allow filters to exist on layers in the future)
  If you think we dont: could we remove this part of the condition?
  If you think we do: can we fold that into the needsToExpandDamage() function that I mentioned above?

[Dana Jansens]

Comment on attachment 138145 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:64
>> +
> 
> Personally I would prefer not to have a function that has two simultaneous purposes like this.  Could we separate this into two helper functions:
>   bool needsToExpandDamage()
>   void expandPixelOutsetsForFilters()

Sure I'll split it up

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:79
>> +static inline bool expandDamageRectWithFilters(FloatRect& damageRect, const FilterOperations& filters)
> 
> Does this need to be a boolean?  It seems not to be used that way, and making it void we can remove that similar "side-effecty two purposes" feeling from it.

Yeh it can be void when we have needsToExpandDamage.

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:206
>> +        if (contributingSurface && expandPixelOutsetsWithFilters(pixelOutsets, layer->backgroundFilters())) {
> 
> Do we need to check whether this is actually a contributing renderSurface versus a contributing layer?   (for example, if for some wacky wild reason, we allow filters to exist on layers in the future)
>   If you think we dont: could we remove this part of the condition?
>   If you think we do: can we fold that into the needsToExpandDamage() function that I mentioned above?

I think it being a surface is important because if it's a layer then the damage stays in the surface below, so the code here would need to do something different. I will try come up with a less generic name than needsToExpandDamage to indicate it's for a surface.

[Dana Jansens]

Comment on attachment 138145 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [chromium] Some background filters require inflating damage on the surface behind them
> 
> "on the surface behind them" is probably misleading here... =)   If I understood correctly, should it say "Some background filters require inflating damage that occurs behind it"

The only thing to consider is that the inflation happens in the space of the surface, not the "blurring" layer's space. So really the damage is inflated in the surface (through the lens of the layer above) kind of.. So I'd prefer to keep the name as is.

>>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:64
>>> +
>> 
>> Personally I would prefer not to have a function that has two simultaneous purposes like this.  Could we separate this into two helper functions:
>>   bool needsToExpandDamage()
>>   void expandPixelOutsetsForFilters()
> 
> Sure I'll split it up

needsToExpandDamage() == filters.hasFilterThatMovesPixels() so I just used that directly.

This let me merge the 3 helper functions into a single expandPixelOutsetsWithFilters() so yay.

[Dana Jansens]

Created attachment 138420 [details]
Patch

[Shawn Singh]

(In reply to comment #4)
> (From update of attachment 138145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138145&action=review
> 
> >> Source/WebCore/ChangeLog:3
> >> +        [chromium] Some background filters require inflating damage on the surface behind them
> > 
> > "on the surface behind them" is probably misleading here... =)   If I understood correctly, should it say "Some background filters require inflating damage that occurs behind it"
> 
> The only thing to consider is that the inflation happens in the space of the surface, not the "blurring" layer's space. So really the damage is inflated in the surface (through the lens of the layer above) kind of.. So I'd prefer to keep the name as is.
> 

OK not a big deal.  To me personally it still sounds like we're talking about a different surface that's behind the damage, which is not what we mean. =)

[Adrienne Walker]

Comment on attachment 138420 [details]
Patch

Can you explain to me why we need to damage the surface behind it? Those pixels aren't written to, just read from.  It seems like their correct values should already be in the backbuffer.  If anything, maybe we just need to unset the scissor before readback?  If you expand the actual damage, then I think we're going to consume more bandwidth writing to the backbuffer for pixels that aren't changed.

[Dana Jansens]

(In reply to comment #7)
> (From update of attachment 138420 [details])
> Can you explain to me why we need to damage the surface behind it? Those pixels aren't written to, just read from.  It seems like their correct values should already be in the backbuffer.  If anything, maybe we just need to unset the scissor before readback?  If you expand the actual damage, then I think we're going to consume more bandwidth writing to the backbuffer for pixels that aren't changed.

Sure, I think we do need to expand damage in the surface and I'll try explain.

Surface S has layer A inside it with a background blur.

S-----------------------+
|                       |
|     A-----+           |
|     | ... |           |
|     | .x. |           |
|     | ... |           |
|     +-----+           |
+-----------------------+

Pixel x gets damaged in S and is visible through A. A is going to read back all the period pixels around x, blur them, and write them back into S. So changed color in x will be reflected in all the period pixels as well. The damaged contents in the *surface* S will larger than the actual pixels that are modified from the perspective of the *layer* S.

[Adrienne Walker]

Ok, I see.  Do we also need to clamp to the size of A or does the filter outsets take care of that? (Also, why does filter outsets not return an IntRect?)

[Dana Jansens]

(In reply to comment #9)
> Ok, I see.  Do we also need to clamp to the size of A or does the filter outsets take care of that? (Also, why does filter outsets not return an IntRect?)

We could, but we don't need to. We are being sloppy here.

Probably because it's not reeeeally a rect. It could though, but things like expanding a bounding box by some smaller rect's edge distance from its center is not such a clear operation. move(-left, -top) expand(left+right, top+bottom) is more clear than a rect for the general case I think.

[Dana Jansens]

(In reply to comment #10)
> (In reply to comment #9)
> > Ok, I see.  Do we also need to clamp to the size of A or does the filter outsets take care of that? (Also, why does filter outsets not return an IntRect?)
> 
> We could, but we don't need to. We are being sloppy here.

Like if a damage rect is half under the layer, we only need to expand the half that is underneath. If it's not under at all we don't need to expand at all. I don't know if it's worth bothering with that though. The occlusion tracker currently takes an even more blunt approach and does the equivalent of damaging the whole surface heh.

[Dana Jansens]

Ever wish for an undo send button on b.w.o like you have in gmail? Occlusion tracker's blunt approach is for foreground filters, so it's a looser analogy..

[Adrienne Walker]

Comment on attachment 138420 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:180
> +        // Visit layers in front-to-back order.

To simplify this, would it be possible to process layers in back-to-front order and then immediately expand the damage rect when we come upon a contributing surface with background filters that move pixels rather than having to call this function again with an offset?

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:196
> +            expandDamageRectWithFilters(damageRectBelowFilters, layer->backgroundFilters());
> +            damageRect.unite(damageRectBelowFilters);

Is it easy to clip the damage here to the layer size? If it's not a big deal, you should to avoid a bigger scissor than needed.  Even if occlusion tracking is more loose, I think there's still a win by being as conservative as possible with our damage rects.

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:416
> +    FloatRect expectedDamageRect = FloatRect(100, 110, 12, 13);
> +    expectedDamageRect.move(-outsetLeft, -outsetTop);
> +    expectedDamageRect.expand(outsetLeft + outsetRight, outsetTop + outsetBottom);

This is kind of circular, in that you're just testing if filter.getOutsets is used.  Do you think it'd be worth baking literal outsets into this test or asserting that the outsets are what you expect they are?

[Dana Jansens]

Comment on attachment 138420 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:180
>> +        // Visit layers in front-to-back order.
> 
> To simplify this, would it be possible to process layers in back-to-front order and then immediately expand the damage rect when we come upon a contributing surface with background filters that move pixels rather than having to call this function again with an offset?

Yes! I think that is a great idea.

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:196
>> +            damageRect.unite(damageRectBelowFilters);
> 
> Is it easy to clip the damage here to the layer size? If it's not a big deal, you should to avoid a bigger scissor than needed.  Even if occlusion tracking is more loose, I think there's still a win by being as conservative as possible with our damage rects.

Hm.... damageRect.unite(union(damageRectBelowFilters, intersection(expandedDamageRectBelowFilters, layerRect))) !

>> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:416
>> +    expectedDamageRect.expand(outsetLeft + outsetRight, outsetTop + outsetBottom);
> 
> This is kind of circular, in that you're just testing if filter.getOutsets is used.  Do you think it'd be worth baking literal outsets into this test or asserting that the outsets are what you expect they are?

Well, I think that is testing FilterOperations, which isn't the goal here. It does seem a bit circular.. but its verifying that the damage matches what the FilterOperations says it should do. It seems scoped correctly here to me. The outsets as they are I think are also larger than what they need to be, and would be testing an implementation details of FilterOperations that isn't important here?

[Shawn Singh]

> >> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:196
> >> +            damageRect.unite(damageRectBelowFilters);
> > 
> > Is it easy to clip the damage here to the layer size? If it's not a big deal, you should to avoid a bigger scissor than needed.  Even if occlusion tracking is more loose, I think there's still a win by being as conservative as possible with our damage rects.
> 
> Hm.... damageRect.unite(union(damageRectBelowFilters, intersection(expandedDamageRectBelowFilters, layerRect))) !

(1) I think the more appropriate place to do this would be where the FIXME is written in updateDamageTrackingState().  

(2) However, last time I tried to do this, it broke things, so I left it as a FIXME.   I would prefer that we create a different bug for this, so that we can carefully test it and get it correct in an isolated change.  (feel free to assign it to me and nag me to do it sooner if needed)

[Shawn Singh]

(In reply to comment #15)
> > >> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:196
> > >> +            damageRect.unite(damageRectBelowFilters);
> > > 
> > > Is it easy to clip the damage here to the layer size? If it's not a big deal, you should to avoid a bigger scissor than needed.  Even if occlusion tracking is more loose, I think there's still a win by being as conservative as possible with our damage rects.
> > 
> > Hm.... damageRect.unite(union(damageRectBelowFilters, intersection(expandedDamageRectBelowFilters, layerRect))) !
> 
> (1) I think the more appropriate place to do this would be where the FIXME is written in updateDamageTrackingState().  
> 
> (2) However, last time I tried to do this, it broke things, so I left it as a FIXME.   I would prefer that we create a different bug for this, so that we can carefully test it and get it correct in an isolated change.  (feel free to assign it to me and nag me to do it sooner if needed)


well, just to correct myself, the FIXME comment is on the top of the few lines of code, and the clamping should occur at the bottom of that same scope...  but maybe that was already clear to you guys

[Shawn Singh]

Comment on attachment 138420 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:196
>>> +            damageRect.unite(damageRectBelowFilters);
>> 
>> Is it easy to clip the damage here to the layer size? If it's not a big deal, you should to avoid a bigger scissor than needed.  Even if occlusion tracking is more loose, I think there's still a win by being as conservative as possible with our damage rects.
> 
> Hm.... damageRect.unite(union(damageRectBelowFilters, intersection(expandedDamageRectBelowFilters, layerRect))) !

Doh - sorry for the noise.  I see now you guys are talking about clamping in a different place than I was thinking.

[Dana Jansens]

Created attachment 138921 [details]
Patch

-back to front!
-only expand damage under the blurring layer!

this is much more awesome, thanks enne. also made unit test more thorough to cover all this stuff.

[Shawn Singh]

Comment on attachment 138921 [details]
Patch


This makes a lot of sense - very awesome.  I had some style nits...


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

> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:65
> +    IntRect pixelOutsets(-left, -top, left + right, top + bottom);

I feel like this is quite misleading.  It encodes and obscures the real meaning of left, right, bottom top.   Those values are actually "deltas" of position, while an IntRect should always represent an actual rect with position and size.

I think it's much clearer to just skip creating the IntRect and use left, right, top, bottom directly in the move/expand operations immediately below.  =)

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:417
> +    // Setting the update rect should cause the corresponding damage to the

I know this is the World's Most Trivial Nit, but would it be OK if we labeled each scenario with something like "CASE 1:"  "CASE 2:"   etc.?
Personally I find those visual cues really help make the code quite a bit more readable.

> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:419
> +    // Since the damage extends to the right/bottom outside of the blured

s/blured/blurred

[Dana Jansens]

Comment on attachment 138921 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:65
>> +    IntRect pixelOutsets(-left, -top, left + right, top + bottom);
> 
> I feel like this is quite misleading.  It encodes and obscures the real meaning of left, right, bottom top.   Those values are actually "deltas" of position, while an IntRect should always represent an actual rect with position and size.
> 
> I think it's much clearer to just skip creating the IntRect and use left, right, top, bottom directly in the move/expand operations immediately below.  =)

Oh sure, this was an artifact of old code where I was trying to merge pixel outsets. Done.

>> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:417
>> +    // Setting the update rect should cause the corresponding damage to the
> 
> I know this is the World's Most Trivial Nit, but would it be OK if we labeled each scenario with something like "CASE 1:"  "CASE 2:"   etc.?
> Personally I find those visual cues really help make the code quite a bit more readable.

done.

>> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:419
>> +    // Since the damage extends to the right/bottom outside of the blured
> 
> s/blured/blurred

done.

[Dana Jansens]

Created attachment 139022 [details]
Patch

[Shawn Singh]

unofficially LGTM =)

[Adrienne Walker]

Comment on attachment 139022 [details]
Patch

Looks great.  R=me.

[WebKit Review Bot]

Comment on attachment 139022 [details]
Patch

Clearing flags on attachment: 139022

Committed r115397: <http://trac.webkit.org/changeset/115397>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.