The default bug view has changed. See this FAQ.

feSpecularLighting with point light incorrectly determines light position

RESOLVED FIXED in mozilla9

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: heycam, Assigned: Robert Longson)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 10 obsolete attachments)

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
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 498404 [details]
Test
(Assignee)

Comment 2

6 years ago
Looks the same to me. I think I'm going to need another screenshot.
(Assignee)

Updated

6 years ago
(Assignee)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Created attachment 500143 [details]
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
(Reporter)

Comment 5

6 years ago
Created attachment 500144 [details]
Screenshot of revised test
(Reporter)

Comment 6

6 years ago
The dark dot in the middle of the light shouldn't be there, either.
(Assignee)

Comment 7

6 years ago
Created attachment 515476 [details] [diff] [review]
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.
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.
Created attachment 515557 [details] [diff] [review]
fix SourceImage bounds
Attachment #515557 - Flags: review?(longsonr)
(Assignee)

Updated

6 years ago
Attachment #515557 - Flags: review?(longsonr) → review+
Created attachment 515573 [details] [diff] [review]
Fix conversion of light points

Some refactoring here as well.
Attachment #515573 - Flags: review?(longsonr)
Attachment #515476 - Flags: review?(roc) → review+
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 :-).
(Assignee)

Updated

6 years ago
Attachment #515573 - Flags: review?(longsonr) → review+
We don't want to take these patches for FF4 I guess.
(Assignee)

Comment 13

6 years ago
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.
Depends on: 610267
I think I convinced Erik of comment #14.
Created attachment 515840 [details]
another testcase

We still have problems ... this testcase should have the lights at the centers of the circles.
Created attachment 515841 [details] [diff] [review]
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.
Attachment #515841 - Flags: review?(longsonr)
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.
(Assignee)

Updated

6 years ago
Attachment #515841 - Flags: review?(longsonr) → review+
(Assignee)

Updated

6 years ago
Assignee: longsonr → roc
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
(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

6 years ago
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)

Comment 24

6 years ago
Thx for clarification and sorry for bothering you with this. 
Marek
Whiteboard: [not-ready-for-cedar]
Blocks: 647809
(Assignee)

Updated

6 years ago
Blocks: 647687
(Assignee)

Comment 25

6 years ago
Created attachment 524867 [details] [diff] [review]
fix failing reftest

comment 18 was correct the reftest was broken.
Attachment #524867 - Flags: review?(roc)
(Assignee)

Comment 26

6 years ago
Created attachment 524868 [details] [diff] [review]
existing reftest fix
checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c)
Attachment #524867 - Attachment is obsolete: true
Attachment #524868 - Flags: review?(roc)
Attachment #524867 - Flags: review?(roc)
(Reporter)

Comment 27

6 years ago
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]:
Attachment #524868 - Flags: review?(roc) → review+
But we also need new reftests to test the stuff we fixed here.
(Assignee)

Updated

6 years ago
Attachment #524868 - Attachment description: fix failing reftest → existing reftest fix checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c)
(Assignee)

Comment 30

6 years ago
I've checked in the fix to the existing reftest issue.
(Reporter)

Comment 31

6 years ago
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?
(Reporter)

Comment 32

6 years ago
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.
(Assignee)

Comment 33

6 years ago
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
(Assignee)

Comment 34

6 years ago
(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.
(Assignee)

Comment 35

6 years ago
(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.
(Reporter)

Comment 36

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

Comment 37

6 years ago
Sure, I can look at them.
(Assignee)

Comment 38

6 years ago
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.
(Assignee)

Comment 39

6 years ago
Created attachment 531608 [details]
test 1 (OK with patches)
(Assignee)

Updated

6 years ago
Attachment #531608 - Attachment description: test 1 → test 1 (OK with patches)
(Assignee)

Comment 40

6 years ago
Created attachment 531609 [details]
test 2 (ref) better but not same
(Assignee)

Comment 41

6 years ago
Created attachment 531610 [details]
test 2
(Assignee)

Comment 42

6 years ago
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)

Updated

6 years ago
Assignee: roc → longsonr
(Assignee)

Updated

6 years ago
Depends on: 671892
(Assignee)

Comment 44

6 years ago
Created attachment 548010 [details] [diff] [review]
update to tip (no issues addressed yet)
Attachment #515476 - Attachment is obsolete: true
Attachment #515557 - Attachment is obsolete: true
Attachment #515573 - Attachment is obsolete: true
Attachment #515841 - Attachment is obsolete: true
(Assignee)

Comment 45

6 years ago
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.
(Assignee)

Comment 46

6 years ago
Oops, I've actually moved the SourceImage bounds into bug 647687
(Assignee)

Updated

6 years ago
Attachment #531608 - Attachment is obsolete: true
(Assignee)

Comment 47

6 years ago
Created attachment 559853 [details] [diff] [review]
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.
Attachment #548010 - Attachment is obsolete: true
Attachment #559853 - Flags: feedback?(roc)
(Assignee)

Updated

6 years ago
Attachment #531609 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #531610 - Attachment is obsolete: true
Attachment #559853 - Flags: feedback?(roc) → feedback+
(Assignee)

Comment 48

6 years ago
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]
(Assignee)

Updated

6 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/061b6c26a019
Status: NEW → RESOLVED
Last Resolved: 6 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.