Closed Bug 619992 Opened 9 years ago Closed 9 years ago

feSpecularLighting with point light incorrectly determines light position

Categories

(Core :: SVG, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: heycam, Assigned: longsonr)

References

()

Details

Attachments

(7 files, 10 obsolete files)

442 bytes, image/svg+xml
Details
58.90 KB, image/png
Details
1.08 KB, image/svg+xml
Details
1.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
546 bytes, image/svg+xml
Details
1.13 KB, image/svg+xml
Details
6.77 KB, patch
Details | Diff | Splinter Review
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.
Attached image Test (obsolete) —
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.
Attached image 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.
Attachment #498404 - Attachment is obsolete: true
The dark dot in the middle of the light shouldn't be there, either.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → longsonr
Attachment #515476 - Flags: review?(roc)
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.
Attachment #515557 - Flags: review?(longsonr) → review+
Attached patch Fix conversion of light points (obsolete) — Splinter Review
Some refactoring here as well.
Attachment #515573 - Flags: review?(longsonr)
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 :-).
Attachment #515573 - Flags: review?(longsonr) → review+
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.
Attached image another testcase
We still have problems ... this testcase should have the lights at the centers of the circles.
Attached patch Fix delta coordinates (obsolete) — Splinter Review
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.
Attachment #515841 - Flags: review?(longsonr) → review+
Assignee: longsonr → roc
(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
Whiteboard: [not-ready-for-cedar]
Blocks: 647687
Attached patch fix failing reftest (obsolete) — Splinter Review
comment 18 was correct the reftest was broken.
Attachment #524867 - Flags: review?(roc)
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.
But we also need new reftests to test the stuff we fixed here.
Attachment #524868 - Attachment description: fix failing reftest → existing reftest fix checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c)
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.
Attached image test 1 (OK with patches) (obsolete) —
Attachment #531608 - Attachment description: test 1 → test 1 (OK with patches)
Attached image test 2 (ref) better but not same (obsolete) —
Attached image test 2 (obsolete) —
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.
Assignee: roc → longsonr
Depends on: 671892
Attachment #515476 - Attachment is obsolete: true
Attachment #515557 - Attachment is obsolete: true
Attachment #515573 - Attachment is obsolete: true
Attachment #515841 - Attachment is obsolete: true
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
Attachment #531608 - Attachment is obsolete: true
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.
Attachment #548010 - Attachment is obsolete: true
Attachment #559853 - Flags: feedback?(roc)
Attachment #531609 - Attachment is obsolete: true
Attachment #531610 - Attachment is obsolete: true
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/061b6c26a019
Summary: feSpecularLighting with point light incorrectly determines bounding box for light positioning → feSpecularLighting with point light incorrectly determines light position
Whiteboard: [not-ready-for-cedar] → [inbound]
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/061b6c26a019
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.