993934 - lighting filter output is wrong since Firefox 28

Status: NEW

(Core :: SVG, defect)

Description

[Cameron McCormack (:heycam)]

This example:

http://trac.webkit.org/export/167007/trunk/LayoutTests/svg/dynamic-updates/SVGFEPointLightElement-svgdom-y-prop.html

in Firefox 27 shows the same as Blink, IE11, Inkscape.  In Firefox 28 this looks a lot darker.

(There is also the issue of the lighting filter's output region that is different, but that is a separate bug.)

Comment 1

[Dirk Schulze]

Attachment: lighting.svg

This is a bigger issue for us at Adobe now. Do you have a regression window?

The attached example shows the darker color of the lighting filter feDiffuseLighting which is used for bump maps and other things. Compare it to Chrome, Safari or IE. Looks the same everywhere but Firefox.

Comment 2

[Markus Stange [:mstange]]

I can think of two possible causes of this bug: We could be doing something wrong when converting the 3D coordinates into filter space, or we could be using the wrong color space.
If this regressed in Firefox 28 then it's very likely due to the switch to Moz2D filter rendering (bug 924103). Are software filters affected as well, or only D2D 1.1 filters?

The conversion of 3D coordinates into filter space happens here:
http://hg.mozilla.org/mozilla-central/annotate/71497ed2e0db/layout/svg/nsSVGFilterInstance.cpp#l184

And forcing lighting filters to be sRGB happens here (introduced by bug 828526, shipped in Firefox 21):
http://hg.mozilla.org/mozilla-central/annotate/afcb3af79d09/content/svg/content/src/nsSVGFilters.h#l195

Comment 3

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #2)
> I can think of two possible causes of this bug: We could be doing something
> wrong when converting the 3D coordinates into filter space, or we could be
> using the wrong color space.
> If this regressed in Firefox 28 then it's very likely due to the switch to
> Moz2D filter rendering (bug 924103). Are software filters affected as well,
> or only D2D 1.1 filters?

I am on Mac. So I didn't test if D2D has the same issue. Software rendering does though.

Comment 6

[Lenard Szolnoki]

(In reply to Markus Stange [:mstange] from comment #2)
> And forcing lighting filters to be sRGB happens here (introduced by bug
> 828526, shipped in Firefox 21):
> http://hg.mozilla.org/mozilla-central/annotate/afcb3af79d09/content/svg/
> content/src/nsSVGFilters.h#l195

Why would the lighting filters should operate in the sRGB colorspace? I think they should operate in a linear colorspace (like a linear RGB in the sRGB gamut).

Comment 7

[Tom Klein]

Attachment: feSpecularLighting testcase.svg

Specular lighting is also affected: we're darker than chrome on this one.  (IE is yet another color, but my assumption based on the fact that the feSpecularLighting-1.svg reftest is skip-if(d2d) is that it's IE that's off.  (More on that later.))

Comment 8

[Tom Klein]

Attachment: Patch v1.diff

It looks like the lighting procedures do work in linear color space, so I switched to that, and then also switched the color-light from its sRGB CSS color space to linear.  With the patch we match chrome (to within 1 in each color component) on both diffuse and specular testcases.  This also causes the live version at https://developer.mozilla.org/en-US/docs/Web/SVG/Element/feDiffuseLighting to match the reference version (wherever that came from...).

There are multiple Windows/D2D issues with the testcases though (software/linux all work as expected):

1) The existing feDiffuseLighting-1.svg testcase vertical line color changed (as expected), so I changed the reference image.  But there are two other D2D issues with that testcase now:

1a) The bottom edge of the interior rectangle in that testcase (the left edge of which is brightly rendered) should be black but D2D now renders it as #0d0d0d.  It renders as #090909 on IE11, so I feel comfortable saying that's a D2D bug.  I increased the fuzz on that testcase to account for it, but maybe there's a better option?

1b) D2D now renders bright and dark lighting highlights at the edge of both diffuse and specular lit rectangles even when the filter region equals the rectangle dimensions.  This didn't happen before and no other browser does it, including IE11, so I'm not sure if it's us are D2D causing that.  I'll look into it some more if this patch is the route we want to go, but if anybody has any ideas on what might be causing it I'd be happy to hear them :)  For now I've worked around that issue in the testcases by increasing the size of the rectangles so that the edges lie outside the svg viewport.

2) Chrome and firefox software filters with the patch applied (on linux) both render feSpecularLighting-2.svg the same, but firefox on Windows with the patch applied renders a different color.  As I mentioned earlier, feSpecularLighting-1.svg is already skip-if(D2D), so I assume IE is in error here and so made feSpecularLighting-2.svg skip-if(D2D) as well.

How do you feel about the patch Robert?  I'd welcome any thoughts on the testcase issues as well ;)

Comment 9

[Tom Klein]

Attachment: testcases on Windows with patch screenshots.png

Here are some screenshots from Windows with the patch applied of the highlights/shadows appearing when the filter bounds match rectangle bounds, as mentioned in comment 8 1b.  The screenshots are from feDiffuseLighting-1.svg, feDiffuseLighting-2.svg, and feSpecularLighting-2.svg, but all with the rectangle bounds within the svg viewport so that the effect is visible.

The effect is pretty clear in the middle image, but you may need to zoom in to see it on the other two (on the first a brighter line is visible on the right side, on the third highlights are visible on the top, left, and bottom edges).

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 8659053 [details] [diff] [review]
Patch v1.diff

as far as I can see lighting filters should obey color-interpolation-filters so...

> protected:
>   virtual bool OperatesOnSRGB(int32_t aInputIndex,
>-                              bool aInputIsAlreadySRGB) override { return true; }
>+                              bool aInputIsAlreadySRGB) override { return false; }

just remove this

...

>+      // both diffuse and specular light primitives operate in linear color

only by default

>+      // space, so transform our css sRGBLightingColor to linear

test the color space (see feDropShadow code in this file) and convert only if linearRGB
might want to make a function and call it.

>+      Color linearLightingColor(
>+              gsRGBToLinearRGBMap[uint8_t(sRGBLightingColor.r * 255)],
>+              gsRGBToLinearRGBMap[uint8_t(sRGBLightingColor.g * 255)],
>+              gsRGBToLinearRGBMap[uint8_t(sRGBLightingColor.b * 255)],
>+              sRGBLightingColor.a);

Comment 11

[Tom Klein]

(In reply to Robert Longson from comment #10)
> as far as I can see lighting filters should obey color-interpolation-filters
> so...

Yeah, I thought I had tested chrome and found it was ignoring color-interpolation-filters in the lighting case, but it's not.  My other argument for ignoring it for lighting and always using linearRGB is that the Phong lighting formulas that svg uses assume input colors are in linear space, and produce colors that are in linear space (I think??), so applying those formulas to sRGB colors and pretending the result is sRGB just seems kind of bogus...  (I'm no expert though, so I could well be wrong about that.  I guess after all the formulas just produce darker but still recognizable results when applied in sRGB space.)

Still, I guess if people don't want the "bogus" sRGB results then they should just not set color-interpolation-filters to sRGB for lighting filters, so I'll make the suggested changes and resubmit.  Thanks for the feedback!

Comment 12

[Tom Klein]

Attachment: Patch v2 WIP

I think the lighting code changes are ready to go, but the tests aren't going to pass until bug 1214889 gets resolved (at which time I'll take another look at the tests and then request review).

Comment 13

[Robert Longson [:longsonr]]

looks good to me.