1065606 - feImage filter primitive paints a smaller region than it used to

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: test.xhtml

STR:
 1. Load attached testcase.

EXPECTED RESULTS:
Wide blue bar.

ACTUAL RESULTS:
Skinny blue bar.

This is a behavior-change with respect to Firefox Release.

Mozregression gives this range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4b8680a3302d&tochange=4ef3fa0ebedf

Looks like regression from https://hg.mozilla.org/integration/mozilla-inbound/rev/a368ab7e93ab :
{
Max Vujovic — Bug 948265 - Clip filter primitives to their filter's filter region. r=mstange
}

I suppose it's conceivable that the old behavior was broken and the new behavior is correct -- not sure.  But it sounds like bug 948265 comment 119 was saying that this change wasn't expected to have a functional effect -- so, assuming it's a regression.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> But it sounds like bug 948265 comment 119
> was saying that this change wasn't expected to have a functional effect --
> so, assuming it's a regression.

(and the subsequent comment expands on that a bit, but it still sounds like a functional change was only expected when there were multiple primitives in play -- <<When there is one SVG filter in the filter chain, the "overall" filter region equals the SVG filter's filter region, so there is no functional change.>>)

Max or Markus, do you know what's going on here?

(FWIW, this was reported by nemo in IRC, with URL http://m8y.org/tmp/testcase239.xhtml , whose filtered "Hello World" text doesn't fully render in Nightly but does render in Release. I reduced the attached testcase from that testcase.)

Comment 2

[Max Vujovic]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Max or Markus, do you know what's going on here?

Thanks for the reduction, Daniel. I'll start digging into it.

I think the bar should be painted wide, since the default filter region is the bounding box of the target element (which is a 100% width div) plus 10% margins, and the feImage is 600px.

Comment 3

[Daniel Holbert [:dholbert]]

Thanks, Max!

Comment 4

[Max Vujovic]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Thanks, Max!

No problem! Thanks again for the great, reduced test case. It made this a lot easier to track down.

The test case shows we can’t clip the primitive subregion to the filter region too early (in nsSVGFilterInstance::ComputeFilterPrimitiveSubregion). SVGFEImageElement::GetPrimitiveDescription relies on the primitive subregion to calculate the image transform. Before the regression, the primitive subregion was set to (0, 0, 600, 600), as defined by the feImage element in the SVG file. After the regression, the primitive subregion is immediately clipped by the filter region (which is 100% x 30px plus 10% margins = 120% x 36px), and the image transform is calculated from that, resulting in a much smaller image.

The image in the test case is a 1x1 blue pixel, and it’s scaled up to a 36x36 square now because of the clipped primitive subregion. It should be scaled up to 600x600 first, and that should be clipped by the 120% x 36px filter region.

I’ll write a patch to get the old behavior back. (Reverting the specified commit won’t quite work, since later changes rely on it. Plus, I think I can factor the new code a little better than before.)

Comment 5

[Max Vujovic]

Attachment: Patch

In this patch, we clip the primitive subregion to the filter region after the filter->GetPrimitiveDescription call instead of before. This makes sure SVGFEImageElement::GetPrimitiveDescription uses the original specified primitive subregion to calculate its image transform.

SVGFEImageElement::GetPrimitiveDescription is the only nsSVGFE::GetPrimitiveDescription implementation that uses the aFilterSubregion argument, which should reduce the chance for other surprises.

Comment 6

[Max Vujovic]

Comment on attachment 8487657 [details] [diff] [review]
Patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/e556497472d3
https://tbpl.mozilla.org/?tree=Try&rev=64d9e3dcd292

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e556497472d3