Closed Bug 988066 Opened 6 years ago Closed 6 years ago

SVG filter invalidation problem

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jrmuizel, Assigned: mstange)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Add failing reftest (obsolete) — Splinter Review
We seem to screw up invalidation on this test case. You can run reftests with the patch applied to see the problem.
Blocks: can-tiles
I don't think this is the right patch.
Flags: needinfo?(jmuizelaar)
Attached patch The right patchSplinter Review
Flags: needinfo?(jmuizelaar)
Attachment #8396791 - Attachment is obsolete: true
(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).
Assignee: nobody → matt.woodrow
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?
Flags: needinfo?(mstange)
(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.
Depends on: 997735
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.
Assignee: matt.woodrow → mstange
Flags: needinfo?(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.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.