988066 - SVG filter invalidation problem

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add failing reftest

We seem to screw up invalidation on this test case. You can run reftests with the patch applied to see the problem.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I don't think this is the right patch.

Comment 2

[Daniel Holbert [:dholbert]]

Shot in the dark: if the actual reftest happens to have a filtered <use> element, this could be related to bug 987590 (also filed today).

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: The right patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(bug 984226 probably fixes overflow area calculation for some dynamic changes to filters, but I don't see how that would be related to this testcase.)

I'd also note that the directory you added this reftest to is:
> # invalidation - only run on B2G
> skip-if(!B2G) include invalidation/reftest.list
(though maybe we should consider changing that).

Comment 5

[Matt Woodrow (:mattwoodrow)]

This looks like a bug in nsSVGIntegrationUtils::AdjustInvalidAreaForSVGEffects, or further down the callstack from there.

The testcase is moving the inner content to the right so that it's outside of the filter area and should no longer be drawn.

Given that, I'd expect the translation from filter space to frame space to have a negative x value, so that 0,0 in the filter space maps to around -200,0 in the frame space.

Instead, mFilterSpaceToFrameSpaceInCSSPxTransform (in nsFilterInstance) is a positive transform (of 180px).

So when we take the dirty rect computed by DLBI (-100,0,300,200 in frame space), and transform it by the inverse of the above to get into 'filter space' we end up with (-280, 20, 300, 200).

The actual filtering is insignificant, so doesn't change the region, but FilterSupport::ComputeResultChangeRegion And's it with the filter space bounds (which are 0, 0, 240, 240). Obviously that removes almost all of our invalid region, and we fail to invalidate.

Removing this intersection fixes the bug, but doesn't seem like the right thing to do, because it should be valid.

Inverting mFilterSpaceToFrameSpaceInCSSPxTransform so that it matches what I'd expect completely breaks rendering, so I guess it's correct for some of the code at least.

Markus, any ideas here?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #4)
> I'd also note that the directory you added this reftest to is:
> > # invalidation - only run on B2G
> > skip-if(!B2G) include invalidation/reftest.list
> (though maybe we should consider changing that).

Filed bug 989574 for this.

Comment 7

[Markus Stange [:mstange]]

The patches in bug 997735 fix this bug.

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> The actual filtering is insignificant, so doesn't change the region, but
> FilterSupport::ComputeResultChangeRegion And's it with the filter space
> bounds (which are 0, 0, 240, 240). Obviously that removes almost all of our
> invalid region, and we fail to invalidate.

Even if all the offsets were correct, this intersection would still remove those parts of the invalid region that fall outside the new filter space bounds. That's not a bug in nsSVGIntegrationUtils::AdjustInvalidAreaForSVGEffects. Instead, the invalidation coming from the inactive layer tree of the filtered children isn't enough to fix this testcase - we also need to invalidate from the outside. See bug 997735 comment 7.

Comment 8

[Markus Stange [:mstange]]

This was fixed by bug 997735. I didn't land attachment 8397060 [details] [diff] [review] because the test from bug 997735 covers this bug as well.