Last Comment Bug 383184 - Implement SVG lighting filters
: Implement SVG lighting filters
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 316908
Blocks: 311029 411023
  Show dependency treegraph
 
Reported: 2007-06-04 12:38 PDT by tor
Modified: 2008-01-06 06:59 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress (49.31 KB, patch)
2007-06-04 12:39 PDT, tor
no flags Details | Diff | Review
update to tip (49.34 KB, patch)
2007-06-27 15:02 PDT, tor
no flags Details | Diff | Review
wip - basic diffuse lighting working (54.31 KB, patch)
2007-07-06 12:33 PDT, tor
no flags Details | Diff | Review
wip - diffuse and specular lighting (no resampling yet) (75.21 KB, patch)
2007-07-11 14:59 PDT, tor
no flags Details | Diff | Review
wip - full lighting filters (187.61 KB, patch)
2007-07-17 15:56 PDT, tor
no flags Details | Diff | Review
same changes, using diff --minimal to get a sane patch (97.22 KB, patch)
2007-07-17 16:03 PDT, tor
no flags Details | Diff | Review
style section broken out for reviewing (9.92 KB, patch)
2007-07-19 13:59 PDT, tor
dbaron: review-
Details | Diff | Review
non-style section (87.30 KB, patch)
2007-07-19 14:01 PDT, tor
no flags Details | Diff | Review
style section split out for reviewing - passes mochitest (15.69 KB, patch)
2007-07-23 13:32 PDT, tor
dbaron: review+
dbaron: superreview+
Details | Diff | Review
non-style section updated per longsonr's comments (87.22 KB, patch)
2007-07-23 13:34 PDT, tor
longsonr: review+
Details | Diff | Review
non-style section with roc's approach (85.47 KB, patch)
2007-07-25 09:36 PDT, tor
roc: superreview+
Details | Diff | Review
filters01.svg zoomed in and out (93.44 KB, image/png)
2007-07-26 13:08 PDT, Arpad Borsos [:Swatinem]
no flags Details
filters-light-01-f zoomed in (22.95 KB, image/png)
2007-07-26 13:29 PDT, Arpad Borsos [:Swatinem]
no flags Details

Comment 1 tor 2007-06-04 12:39:23 PDT
Created attachment 267185 [details] [diff] [review]
work in progress
Comment 2 Robert Longson 2007-06-04 13:22:27 PDT
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 2007-06-04 13:41:18 PDT
(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 2007-06-27 15:02:20 PDT
Created attachment 270072 [details] [diff] [review]
update to tip
Comment 5 tor 2007-07-06 12:33:30 PDT
Created attachment 271253 [details] [diff] [review]
wip - basic diffuse lighting working
Comment 6 tor 2007-07-11 14:59:09 PDT
Created attachment 271910 [details] [diff] [review]
wip - diffuse and specular lighting (no resampling yet)
Comment 7 tor 2007-07-17 15:56:26 PDT
Created attachment 272712 [details] [diff] [review]
wip - full lighting filters
Comment 8 tor 2007-07-17 16:03:58 PDT
Created attachment 272713 [details] [diff] [review]
same changes, using diff --minimal to get a sane patch
Comment 9 tor 2007-07-19 13:59:57 PDT
Created attachment 273025 [details] [diff] [review]
style section broken out for reviewing
Comment 10 tor 2007-07-19 14:01:32 PDT
Created attachment 273026 [details] [diff] [review]
non-style section

New competitor for slowest filter...
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2007-07-19 14:06:43 PDT
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 2007-07-23 07:08:24 PDT
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 2007-07-23 09:57:07 PDT
(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 2007-07-23 13:32:13 PDT
Created attachment 273451 [details] [diff] [review]
style section split out for reviewing - passes mochitest
Comment 15 tor 2007-07-23 13:34:20 PDT
Created attachment 273452 [details] [diff] [review]
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-7 (review requests must explain patch) 2007-07-23 13:43:28 PDT
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) (Exited; email my personal email if necessary) 2007-07-23 17:23:03 PDT
+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 2007-07-24 12:53:09 PDT
(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) (Exited; email my personal email if necessary) 2007-07-24 17:54:24 PDT
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 2007-07-25 09:36:27 PDT
Created attachment 273797 [details] [diff] [review]
non-style section with roc's approach
Comment 21 Robert Longson 2007-07-25 09:54:18 PDT
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 2007-07-25 11:26:11 PDT
(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) (Exited; email my personal email if necessary) 2007-07-25 11:32:03 PDT
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 2007-07-25 12:35:01 PDT
OK. I understand now.
Comment 25 tor 2007-07-26 00:18:35 PDT
Checked in.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-07-26 01:50:57 PDT
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 2007-07-26 07:49:15 PDT
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] 2007-07-26 13:08:08 PDT
Created attachment 274032 [details]
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] 2007-07-26 13:29:01 PDT
Created attachment 274040 [details]
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 2007-07-27 10:48:22 PDT
Please file new bugs for problems encountered with the lighting filters.

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