It seems like <feSpecularLighting><fePointLight> incorrectly determines where to position the light source when primitiveUnits="objectBoundingBox" and the top-left corner of the bounding box of the filter target is negative x and/or y. Compare the attachment's rendering in Opera to Firefox. Both of the rectangles in the test should have the same lighting.

Looks the same to me. I think I'm going to need another screenshot.

To be clear, I see significant differences in the w3c reftests between the SVG and the image but I don't see any differences in your reduced testcase.

Sorry, I think I got myself confused with that first test. I couldn't reproduce the problem with it either! Here is a test with a problem I can reproduce. The centre of the lighting should be in the centre of the rectangle, but we render it somewhere towards the top-left of the rectangle (at 50x50, probably). It seems like the viewBox transform isn't being taken into account when determining the position of the lighting.

The dark dot in the middle of the light shouldn't be there, either.

This fixes the position. I don't know how to write a reftest. Any ideas Cam? It doesn't fix the svg 1.1 test though.

There are a few remaining problems in the SVG 1.1 tests. One is that the test expects the light in the circles to be in the lower right. However, I don't know why this should be the case for the light-primobjbbox filter: <fePointLight x="0.375" y="0.375" z="-0.0625"/> So the light should be the top-left of the leftmost circle, as far as I can tell. That seems like a bug in the test. But maybe I'm misunderstanding something. The lights on the other two circles should be in the bottom right because the coordinates 30,30 should be interpreted as user-space-on-use, and the user-space 0,0 is at the center of the circles. That's our bug. Another bug is that we're not computing the bounds of the SourceImage/SourceAlpha image correctly in nsSVGFilterInstance. For SVG filter targets we're using the SVG bbox to get the bounds, but that doesn't include stroke width.

Some refactoring here as well.

With these three patches we pass the test in the URL field, except for the issue noted in the first paragraph of comment #8, which I remain convinced is a test bug. I will discuss this with Erik tomorrow :-).

We don't want to take these patches for FF4 I guess.

I wouldn't if I was making the decisions ;-)

I guess there is a valid spec question: whether 'x' and 'y' in the light positions should be interpreted as a point in the object-bounding-box coordinate system (the way 'x' and 'y' in the filter primitive subregion are) (and thus be relative to the object-bounding-box top-left) or whether they should just be lengths expressed as fractions of the object-bounding-box dimensions, which are then interpreted in user space. It seems obvious to me that the former interpretation makes the most sense, but I think the spec isn't really clear. The lighting coordinates and the 'x' and 'y' that define the filter primitive subregion are the only "absolute" coordinate attributes defined for filter primitives --- all the other filter primitive lengths are explicitly some kind of distance or offset --- so I think it makes sense for them to behave the same way. I will bring this up at the WG meeting this week.

I think I convinced Erik of comment #14.

We still have problems ... this testcase should have the lights at the centers of the circles.

We were comparing "filter space" coordinates with "data rect" coordinates, but the data rects need mSurfaceRect.x/y added to them to get into filter space. I also think we should be doing the difference in user space coordinates. Doing it in "filter space" makes the magnitude of the difference depend on the resolution we're using, which is bogus.

These patches cause objectBoundingBox-and-fePointLight-01.svg to fail, I think the test needs to be fixed. And I need to write a bunch more tests here.

(In reply to comment #17) > Created attachment 515841 [details] [diff] [review] > I also think we should be doing the difference in user space coordinates. Doing > it in "filter space" makes the magnitude of the difference depend on the > resolution we're using, which is bogus. Sounds like you've fixed bug 411023 too.

(In reply to comment #18) > These patches cause objectBoundingBox-and-fePointLight-01.svg to fail, I think > the test needs to be fixed. That test is just one of the bottom rectangles in the SVG testsuite test we're trying to fix.

Maybe it is totally unrelated, but this good old Adobe SVG demo at http://svg.kvalitne.cz/adobe/feMarble.svg , which is quite complicated, I know, but I'm unable to simplify it quickly, sorry, and have no time, does not render well in Gecko, but is working properly in Opera, Batik and even Chrome (see comparison table http://svg.kvalitne.cz/resume.htm, #99). It would be really great to have it fixed in Firefox 4.1 or 5.0... There is feSpecularLightning in use, among other filters, but what is really causing unexpected rendering is unknown to me...

That's probably due to the fact that we don't support feImage referring to SVG fragments in the same document --- different bug.

(bug 455986 to be specific)

Thx for clarification and sorry for bothering you with this. Marek

comment 18 was correct the reftest was broken.

Is the reftest fix review the last thing that needs doing before landing? It'd be great to get this landed soon, to help with the SVG 1.1 Second Edition implementation report.

Comment on attachment 524868 [details] [diff] [review] existing reftest fix checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c) Review of attachment 524868 [details] [diff] [review]:

But we also need new reftests to test the stuff we fixed here.

I've checked in the fix to the existing reftest issue.

This test fails with the patches applied. I expect it to show a blue 100x100 rectangle. Should the SourceAlpha/SourceGraphic bounds fixup patch have made it pass?

Here's another test. The two rectangles should render identically, I think, and they (apart from pixels right at the edge of the second rectangle) do in current builds, but not with the patches applied. The only difference between the two is different filterRes values.

try server builds with all the patches in for this bug should appear here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/longsonr@gmail.com-c1e1f2104e75

(In reply to comment #31) > Created attachment 529403 [details] > test for SourceGraphic bounds > > This test fails with the patches applied. I expect it to show a blue 100x100 > rectangle. Should the SourceAlpha/SourceGraphic bounds fixup patch have made > it pass? With all the patches applied I see a red 100x100 rectangle with a black centre maybe 20x20. Same on Opera.

(In reply to comment #32) > Created attachment 529409 [details] > test filterRes differences > > Here's another test. The two rectangles should render identically, I think, > and they (apart from pixels right at the edge of the second rectangle) do in > current builds, but not with the patches applied. The only difference between > the two is different filterRes values. They don't render identically on Opera.

longsonr, would you be able to check my tests for correctness? I believe they're right (despite not working in Opera).

Sure, I can look at them.

In the SourceGraphic example you've specified an x, y, width and height for the feComposite filter element. This creates a hard clipping region and since objectBoundingBox units ignore stroke in determining the bounding box you end up clipping the filter into nothingness. Removing the x, y, width and height attributes improves things somewhat. Compare your example with https://bug647687.bugzilla.mozilla.org/attachment.cgi?id=524728 which we could use as the basis of a reftest. The filterRes example also specifies a 0, 0, 1, 1 x, y, width, height on one of the filters though I've only given it a cursory look so far.

Should test 2 and test 2 ref display the same? They look the same (by eye) in Opera. They're pretty close but not the same with the patches here (radically different without).

Robert, Cameron, can one of you guys take ownership of this? I'm swamped.

Comment on attachment 515557 [details] [diff] [review] fix SourceImage bounds I've moved this into bug 647697. This bug can then just be about the lighting filters.

Oops, I've actually moved the SourceImage bounds into bug 647687

I've added a test and I think this can land. The bounding box changes are now in bug 647687 so this isn't a complete fix for the SVG test suite any more. The difference between Opera and Firefox now is down to how the default filterRes is calculated. The SVG Specification says that UAs should choose something reasonable and they both do though in different ways.

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/061b6c26a019