619992 - feSpecularLighting with point light incorrectly determines light position

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Test

Comment 2

[Robert Longson [:longsonr]]

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

Comment 3

[Robert Longson [:longsonr]]

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.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Revised test

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.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Screenshot of revised test

Comment 6

[Cameron McCormack (:heycam)]

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

Comment 7

[Robert Longson [:longsonr]]

Attachment: patch

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix SourceImage bounds

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Fix conversion of light points

Some refactoring here as well.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-).

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 13

[Robert Longson [:longsonr]]

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think I convinced Erik of comment #14.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: another testcase

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

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Fix delta coordinates

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 19

[Robert Longson [:longsonr]]

(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.

Comment 20

[Robert Longson [:longsonr]]

(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.

Comment 21

[Marek Raida]

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...

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[Daniel Holbert [:dholbert]]

(bug 455986 to be specific)

Comment 24

[Marek Raida]

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

Comment 25

[Robert Longson [:longsonr]]

Attachment: fix failing reftest

comment 18 was correct the reftest was broken.

Comment 26

[Robert Longson [:longsonr]]

Attachment: existing reftest fix checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c)

Comment 27

[Cameron McCormack (:heycam)]

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 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]:

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 30

[Robert Longson [:longsonr]]

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

Comment 31

[Cameron McCormack (:heycam)]

Attachment: 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?

Comment 32

[Cameron McCormack (:heycam)]

Attachment: 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.

Comment 33

[Robert Longson [:longsonr]]

try server builds with all the patches in for this bug should appear here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c1e1f2104e75

Comment 34

[Robert Longson [:longsonr]]

(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.

Comment 35

[Robert Longson [:longsonr]]

(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.

Comment 36

[Cameron McCormack (:heycam)]

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

Comment 37

[Robert Longson [:longsonr]]

Sure, I can look at them.

Comment 38

[Robert Longson [:longsonr]]

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.

Comment 39

[Robert Longson [:longsonr]]

Attachment: test 1 (OK with patches)

Comment 40

[Robert Longson [:longsonr]]

Attachment: test 2 (ref) better but not same

Comment 41

[Robert Longson [:longsonr]]

Attachment: test 2

Comment 42

[Robert Longson [:longsonr]]

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).

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 44

[Robert Longson [:longsonr]]

Attachment: update to tip (no issues addressed yet)

Comment 45

[Robert Longson [:longsonr]]

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.

Comment 46

[Robert Longson [:longsonr]]

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

Comment 47

[Robert Longson [:longsonr]]

Attachment: update to tip and add tests

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.

Comment 48

[Robert Longson [:longsonr]]

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

Comment 49

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/061b6c26a019