Last Comment Bug 619992 - feSpecularLighting with point light incorrectly determines light position
: feSpecularLighting with point light incorrectly determines light position
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Robert Longson
:
Mentors:
http://dev.w3.org/SVG/profiles/1.1F2/...
Depends on: post2.0 671892
Blocks: svg11tests 647687 647809
  Show dependency treegraph
 
Reported: 2010-12-17 13:22 PST by Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
Modified: 2011-09-15 07:28 PDT (History)
6 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test (587 bytes, image/svg+xml)
2010-12-17 13:23 PST, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
no flags Details
Revised test (442 bytes, image/svg+xml)
2010-12-28 19:51 PST, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
no flags Details
Screenshot of revised test (58.90 KB, image/png)
2010-12-28 19:54 PST, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
no flags Details
patch (11.15 KB, patch)
2011-02-27 08:10 PST, Robert Longson
roc: review+
Details | Diff | Review
fix SourceImage bounds (4.66 KB, patch)
2011-02-27 20:24 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
longsonr: review+
Details | Diff | Review
Fix conversion of light points (6.27 KB, patch)
2011-02-28 00:47 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
longsonr: review+
Details | Diff | Review
another testcase (1.08 KB, image/svg+xml)
2011-02-28 21:04 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
Fix delta coordinates (4.00 KB, patch)
2011-02-28 21:09 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
longsonr: review+
Details | Diff | Review
fix failing reftest (875 bytes, patch)
2011-04-09 11:02 PDT, Robert Longson
no flags Details | Diff | Review
existing reftest fix checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c) (1.69 KB, patch)
2011-04-09 11:04 PDT, Robert Longson
roc: review+
Details | Diff | Review
test for SourceGraphic bounds (546 bytes, image/svg+xml)
2011-05-01 18:00 PDT, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
no flags Details
test filterRes differences (1.13 KB, image/svg+xml)
2011-05-01 20:34 PDT, Cameron McCormack (:heycam) (away Jun 25 – Jul 10)
no flags Details
test 1 (OK with patches) (637 bytes, image/svg+xml)
2011-05-11 06:17 PDT, Robert Longson
no flags Details
test 2 (ref) better but not same (415 bytes, image/svg+xml)
2011-05-11 06:20 PDT, Robert Longson
no flags Details
test 2 (414 bytes, image/svg+xml)
2011-05-11 06:21 PDT, Robert Longson
no flags Details
update to tip (no issues addressed yet) (9.57 KB, patch)
2011-07-24 08:24 PDT, Robert Longson
no flags Details | Diff | Review
update to tip and add tests (6.77 KB, patch)
2011-09-12 06:16 PDT, Robert Longson
roc: feedback+
Details | Diff | Review

Description Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2010-12-17 13:22:54 PST
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) (away Jun 25 – Jul 10) 2010-12-17 13:23:28 PST
Created attachment 498404 [details]
Test
Comment 2 Robert Longson 2010-12-18 05:27:04 PST
Looks the same to me. I think I'm going to need another screenshot.
Comment 3 Robert Longson 2010-12-18 05:30:40 PST
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) (away Jun 25 – Jul 10) 2010-12-28 19:51:15 PST
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.
Comment 5 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2010-12-28 19:54:58 PST
Created attachment 500144 [details]
Screenshot of revised test
Comment 6 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2010-12-28 19:55:37 PST
The dark dot in the middle of the light shouldn't be there, either.
Comment 7 Robert Longson 2011-02-27 08:10:50 PST
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.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-27 20:21:29 PST
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) (Exited; email my personal email if necessary) 2011-02-27 20:24:37 PST
Created attachment 515557 [details] [diff] [review]
fix SourceImage bounds
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-28 00:47:47 PST
Created attachment 515573 [details] [diff] [review]
Fix conversion of light points

Some refactoring here as well.
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-28 00:49:09 PST
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) (Exited; email my personal email if necessary) 2011-02-28 02:05:07 PST
We don't want to take these patches for FF4 I guess.
Comment 13 Robert Longson 2011-02-28 02:06:51 PST
I wouldn't if I was making the decisions ;-)
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-28 02:32:42 PST
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) (Exited; email my personal email if necessary) 2011-02-28 21:03:53 PST
I think I convinced Erik of comment #14.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-28 21:04:57 PST
Created attachment 515840 [details]
another testcase

We still have problems ... this testcase should have the lights at the centers of the circles.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-28 21:09:37 PST
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.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-02-28 21:12:22 PST
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 2011-03-01 01:36:05 PST
(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 2011-03-01 01:38:23 PST
(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 2011-03-01 14:29:40 PST
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) (Exited; email my personal email if necessary) 2011-03-01 14:42:38 PST
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] (largely AFK until June 28) 2011-03-01 15:05:12 PST
(bug 455986 to be specific)
Comment 24 Marek Raida 2011-03-01 21:38:38 PST
Thx for clarification and sorry for bothering you with this. 
Marek
Comment 25 Robert Longson 2011-04-09 11:02:35 PDT
Created attachment 524867 [details] [diff] [review]
fix failing reftest

comment 18 was correct the reftest was broken.
Comment 26 Robert Longson 2011-04-09 11:04:08 PDT
Created attachment 524868 [details] [diff] [review]
existing reftest fix
checked in (http://hg.mozilla.org/mozilla-central/rev/74af720b024c)
Comment 27 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2011-04-27 20:04:13 PDT
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) (Exited; email my personal email if necessary) 2011-04-27 20:25:54 PDT
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) (Exited; email my personal email if necessary) 2011-04-27 20:26:31 PDT
But we also need new reftests to test the stuff we fixed here.
Comment 30 Robert Longson 2011-04-30 08:18:31 PDT
I've checked in the fix to the existing reftest issue.
Comment 31 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2011-05-01 18:00:42 PDT
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?
Comment 32 Cameron McCormack (:heycam) (away Jun 25 – Jul 10) 2011-05-01 20:34:33 PDT
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.
Comment 33 Robert Longson 2011-05-02 06:17:48 PDT
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
Comment 34 Robert Longson 2011-05-02 08:08:15 PDT
(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 2011-05-02 08:09:45 PDT
(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) (away Jun 25 – Jul 10) 2011-05-02 14:10:50 PDT
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 2011-05-04 01:21:50 PDT
Sure, I can look at them.
Comment 38 Robert Longson 2011-05-10 01:10:35 PDT
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 2011-05-11 06:17:46 PDT
Created attachment 531608 [details]
test 1 (OK with patches)
Comment 40 Robert Longson 2011-05-11 06:20:34 PDT
Created attachment 531609 [details]
test 2 (ref) better but not same
Comment 41 Robert Longson 2011-05-11 06:21:12 PDT
Created attachment 531610 [details]
test 2
Comment 42 Robert Longson 2011-05-11 06:27:25 PDT
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) (Exited; email my personal email if necessary) 2011-06-30 04:59:03 PDT
Robert, Cameron, can one of you guys take ownership of this? I'm swamped.
Comment 44 Robert Longson 2011-07-24 08:24:17 PDT
Created attachment 548010 [details] [diff] [review]
update to tip (no issues addressed yet)
Comment 45 Robert Longson 2011-08-21 10:24:27 PDT
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 2011-08-21 10:25:28 PDT
Oops, I've actually moved the SourceImage bounds into bug 647687
Comment 47 Robert Longson 2011-09-12 06:16:47 PDT
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.
Comment 49 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-15 07:28:44 PDT
https://hg.mozilla.org/mozilla-central/rev/061b6c26a019

Note You need to log in before you can comment on or make changes to this bug.