383184 - Implement SVG lighting filters

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Work in progress.  On hold until we get replies from the WG on specification errata questions:

  http://lists.w3.org/Archives/Public/www-svg/2007Jun/0003.html
  http://lists.w3.org/Archives/Public/www-svg/2007Jun/0002.html
  http://lists.w3.org/Archives/Public/www-svg/2007Jun/0005.html

Comment 1

[tor]

Attachment: work in progress

Comment 2

[Robert Longson [:longsonr]]

I've not really looked too closely yet but...

What about lighting-color="transparent" ?

Some variable names don't match mozilla conventions. You could add a comment that these are taken from the spec to head off explaining that later.

> color = 0; // black

NS_RGB(0, 0, 0);

> +      targetData[index + GFX_ARGB32_OFFSET_B] = PRUint8(tmp * NS_GET_R(color));
> +      targetData[index + GFX_ARGB32_OFFSET_G] = PRUint8(tmp * NS_GET_G(color));
> +      targetData[index + GFX_ARGB32_OFFSET_R] = PRUint8(tmp * NS_GET_B(color));

Not sure why R is assigned to B and vice versa?

Comment 3

[tor]

(In reply to comment #2)

This is nowhere near the point that I'm looking for reviews - just backing up work in bugzilla.

Comment 4

[tor]

Attachment: update to tip

Comment 5

[tor]

Attachment: wip - basic diffuse lighting working

Comment 6

[tor]

Attachment: wip - diffuse and specular lighting (no resampling yet)

Comment 7

[tor]

Attachment: wip - full lighting filters

Comment 8

[tor]

Attachment: same changes, using diff --minimal to get a sane patch

Comment 9

[tor]

Attachment: style section broken out for reviewing

Comment 10

[tor]

Attachment: non-style section

New competitor for slowest filter...

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273025 [details] [diff] [review]
style section broken out for reviewing

Please add an appropriate entry for the property to layout/style/tests/property_database.js, fix anything needed to make all the mochitests in layout/style/test/ pass, and then request review.

(To run the mochitests, run "perl ./runtests.pl" in objdir/_tests/testing/mochitest/, click on the "layout/style/test" subdirectory, and then (on the page for that subdirectory) click "Run All Tests".

Comment 12

[Robert Longson [:longsonr]]

Comment on attachment 273026 [details] [diff] [review]
non-style section

>+static void
>+GenerateNormal(float *N, PRUint8 *data, PRInt32 stride, nsRect rect,
>+               PRInt32 x, PRInt32 y, float surfaceScale)

Make data a const pointer. 

>+  float norm = sqrt(DOT(N, N));

Norm if you are going to follow the Spec naming.

>+  N[0] /= norm;
>+  N[1] /= norm;
>+  N[2] /= norm;

The calculation of Norm followed by division by it occurs in multiple places. Make a function or macro for it perhaps.

>+    L[0] = cos(azimuth * M_PI/180.0) * cos(elevation * M_PI/180.0);
>+    L[1] = sin(azimuth * M_PI/180.0) * cos(elevation * M_PI/180.0);
>+    L[2] = sin(elevation * M_PI/180.0);
>+  }

define a conversion variable

const float radPerDeg = M_PI/180.0;

and use it.

Also could define const PRInt32 x = 0, y = 1, z = 2 then we would have L[x] which might make it very slightly easier to relate to the spec. In fact x, y and z could be used in GenerateNormal too.

>+    if (!spot->HasAttr(kNameSpaceID_None, nsGkAtoms::limitingConeAngle)) {

Easier to comprehend if you take the ! out and reverse order of the lines below.

>+      cosConeAngle = 0;
>+    } else {
>+      cosConeAngle = PR_MAX(cos(limitingConeAngle * M_PI / 180), 0);

Use conversion variable (see above)

>+      if (pointLight || spotLight) {
>+        float Z =
>+          surfaceScale * sourceData[index + GFX_ARGB32_OFFSET_A] / 255;

Is FAST_DIVIDE_BY_255 appropriate?

>+void
>+nsSVGFEDiffuseLightingElement::LightPixel(float *N, float *L,
>+                                          nscolor color, PRUint8 *targetData)

make N and L const. 

>+void
>+nsSVGFESpecularLightingElement::LightPixel(float *N, float *L,
>+                                           nscolor color, PRUint8 *targetData)

N and L should be const.

>+  float specularConstant = mNumberAttributes[SPECULAR_CONSTANT].GetAnimValue();

If you are using names from the spec, this should be k shouldn't it?

>+  if (DOT(N, H) > 0 && specularConstant > 0) {

store DOT(N, H) in a variable.

>+    float specularNH =
>+      specularConstant *
>+      pow(DOT(N, H), mNumberAttributes[SPECULAR_EXPONENT].GetAnimValue());

And then use it here.

>+    targetData[GFX_ARGB32_OFFSET_B] = 255;

Blue? I think you meant to set GFX_ARGB32_OFFSET_A to 255

>+    targetData[GFX_ARGB32_OFFSET_G] = 0;
>+    targetData[GFX_ARGB32_OFFSET_R] = 0;
>+    targetData[ GFX_ARGB32_OFFSET_A] = 0;

Nit: remove extraneous space after [

I need to lie down now. My head hurts.

Comment 13

[tor]

(In reply to comment #12)
> >+      if (pointLight || spotLight) {
> >+        float Z =
> >+          surfaceScale * sourceData[index + GFX_ARGB32_OFFSET_A] / 255;
> 
> Is FAST_DIVIDE_BY_255 appropriate?

No, surfaceScale doesn't have any range restriction, and we need a floating point result without a scale factor for the rest of the calculations.

> I need to lie down now. My head hurts.

Insane parts of the specification will do that.

Comment 14

[tor]

Attachment: style section split out for reviewing - passes mochitest

Comment 15

[tor]

Attachment: non-style section updated per longsonr's comments

Comments applied except for the 0,1,2->x,y,z one, which seems to be a minor gain and slightly less of an issue with a normalization macro.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 273451 [details] [diff] [review]
style section split out for reviewing - passes mochitest

r+sr=dbaron, although you might want to test rgba(255,255,255,1.0) rather than rgba(255,255,255,255).  Probably good to test a big number too, but make it != 255 to avoid confusion.

Comment 17

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

+GenerateNormal(float *N, const PRUint8 *data, PRInt32 stride, nsRect rect,
+               PRInt32 x, PRInt32 y, float surfaceScale)

Don't we already have code for this kind of convolution kernel somewhere around? If not, how about declaring 8 local variables, one for each data element, and initializing them in x/y conditionals, along with the X/Y pixel count you need to generate factorX and factorY. Then the conditionals would not need to be nested and you could write the expressions just once. A lot could be done to speed this up but let's make it as simple as possible at least.

Comment 18

[tor]

(In reply to comment #17)
> +GenerateNormal(float *N, const PRUint8 *data, PRInt32 stride, nsRect rect,
> +               PRInt32 x, PRInt32 y, float surfaceScale)
> 
> Don't we already have code for this kind of convolution kernel somewhere
> around?

We have some more general convolution code for feConvolutionMatrix, but it seems that code specifically for the 3x3 filter would be better suited for this.

> If not, how about declaring 8 local variables, one for each data
> element, and initializing them in x/y conditionals, along with the X/Y pixel
> count you need to generate factorX and factorY. Then the conditionals would not
> need to be nested and you could write the expressions just once.

Spent some time going over this description, the code, and the relevant convolution matrices, but I'm missing what you're aiming at here.

I could simplify the code a fair bit, but haven't figured out the logic behind the factor_x/factor_y normalization specified by SVG:

  http://www.w3.org/TR/SVG11/filters.html#feDiffuseLighting

Comment 19

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

Oops, I made some assumptions that aren't true.

Alright then, how about converting x and y into values 0-2 representing left edge, interior, right edge, and then have all the K values and factors in arrays like so:
  static const PRInt8 Kx[3][3][3][3] = ...;
  static const PRInt8 Ky[3][3][3][3] = ...;
  static const PRInt8 FACTORx[3][3] = ...;
  static const PRInt8 FACTORy[3][3] = ...;
then when you apply the kernel you need to test whether the K value is zero before accessing the corresponding array element, but that's OK. You could have a single function to apply a kernel and call it twice, once for x and once for y.

Comment 20

[tor]

Attachment: non-style section with roc's approach

Comment 21

[Robert Longson [:longsonr]]

Comment on attachment 273797 [details] [diff] [review]
non-style section with roc's approach

This makes more sense if you start at the bottom and work up.

>+static PRInt32
>+Convolve3x3(const PRUint8 *index, PRInt32 stride,
>+            const PRInt8 kernel[3][3])
>+{
>+  PRInt32 sum = 0;
>+  for (PRInt32 y = 0; y < 3; y++) {
>+    for (PRInt32 x = 0; x < 3; x++) {
>+      PRInt8 k = kernel[y][x];
>+      if (k)
>+        sum += k * index[4 * (x - 1) + stride * (y - 1)];

I think we might be out of bounds here when x = y = 0.

>+    }
>+  }
>+  return sum;
>+}
>+
>+static void
>+GenerateNormal(float *N, const PRUint8 *data, PRInt32 stride, nsRect rect,
>+               PRInt32 x, PRInt32 y, float surfaceScale)
>+{

...

>+  const PRUint8 *index = data + y * stride + 4 * x + GFX_ARGB32_OFFSET_A;

So index either points to the start of the image (data) or the start + 3 depending on endianness.

>+
>+  N[0] = -surfaceScale * FACTORx[yflag][xflag] *
>+    Convolve3x3(index, stride, Kx[yflag][xflag]);
>+  N[1] = -surfaceScale * FACTORy[yflag][xflag] *
>+    Convolve3x3(index, stride, Ky[yflag][xflag]);
>+  N[2] = 255;
>+  NORMALIZE(N);
>+}
>+

...

>+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
>+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
>+      PRInt32 index = y * stride + x * 4;
>+
>+      float N[3];
>+      GenerateNormal(N, sourceData, stride, rect, x, y, surfaceScale);
>+

Let's assume x = y = 0 (because the filter covers the whole filter region)

Comment 22

[tor]

(In reply to comment #21)
> (From update of attachment 273797 [details] [diff] [review])
> This makes more sense if you start at the bottom and work up.
> 
> >+static PRInt32
> >+Convolve3x3(const PRUint8 *index, PRInt32 stride,
> >+            const PRInt8 kernel[3][3])
> >+{
> >+  PRInt32 sum = 0;
> >+  for (PRInt32 y = 0; y < 3; y++) {
> >+    for (PRInt32 x = 0; x < 3; x++) {
> >+      PRInt8 k = kernel[y][x];
> >+      if (k)
> >+        sum += k * index[4 * (x - 1) + stride * (y - 1)];
> 
> I think we might be out of bounds here when x = y = 0.

No - that's why the 'k' test is there - the convolution kernels are such that the pixels off the borders have zero weight.

> >+static void
> >+GenerateNormal(float *N, const PRUint8 *data, PRInt32 stride, nsRect rect,
> >+               PRInt32 x, PRInt32 y, float surfaceScale)
> >+{
> 
> ...
> 
> >+  const PRUint8 *index = data + y * stride + 4 * x + GFX_ARGB32_OFFSET_A;
> 
> So index either points to the start of the image (data) or the start + 3
> depending on endianness.

No - it always points to the source pixel Z value (A_in).

> >+  for (PRInt32 y = rect.y; y < rect.YMost(); y++) {
> >+    for (PRInt32 x = rect.x; x < rect.XMost(); x++) {
> >+      PRInt32 index = y * stride + x * 4;
> >+
> >+      float N[3];
> >+      GenerateNormal(N, sourceData, stride, rect, x, y, surfaceScale);
> >+
> 
> Let's assume x = y = 0 (because the filter covers the whole filter region)

Missing something from your comment?

Comment 23

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

Comment on attachment 273797 [details] [diff] [review]
non-style section with roc's approach

Much better!

> I think we might be out of bounds here when x = y = 0.

Shouldn't be, although it's subtle. When x-1 or y-1 are out of bounds, k should be zero.

Comment 24

[Robert Longson [:longsonr]]

OK. I understand now.

Comment 25

[tor]

Checked in.

Comment 26

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

looks like this added 26K of code size. I guess it's mostly DOM glue stuff so not much we can do about it.

Comment 27

[Steffen Wilberg]

Cool, thanks a lot for implementing this.

These two look different, the reference image doesn't have so much light on it:
http://www.w3.org/TR/SVG/images/filters/filters01.png
http://www.w3.org/TR/SVG/images/filters/filters01.svg
That's from http://www.w3.org/TR/SVG/filters.html#AnExample.

Comment 28

[Arpad Borsos [:Swatinem]]

Attachment: filters01.svg zoomed in and out

There seems to be a problem with zooming.
When I zoom in, the lighting increases. And it decreases when I zoom out.
(I used my own SVGControl extension to zoom in/out. The extension is not publicly released, however you can get the code from the hg repo: http://swatinemz.sourceforge.net/hg/svgc/ )

Comment 29

[Arpad Borsos [:Swatinem]]

Attachment: filters-light-01-f zoomed in

This is http://www.w3.org/Graphics/SVG/Test/20061213/htmlEmbedHarness/full-filters-light-01-f.html from the official SVG testsuite zoomed in.
Apart from the ugly pixels (I just read in the code that the testsuite uses a png bumpmap) I can see some artifacts.
Also the light strength seems to change with the zoom factor. It's much lighter when I zoom in.
In the other examples of filters-light-01-f also the light direction changes when I zoom in/out.
Maybe it's a problem with zoom? (My extension uses the currentScale DOM property) Should I open a new bug for that?

Comment 30

[tor]

Please file new bugs for problems encountered with the lighting filters.