Closed Bug 383184 Opened 15 years ago Closed 15 years ago

Implement SVG lighting filters

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tor, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 9 obsolete files)

Attached patch work in progress (obsolete) — Splinter Review
Blocks: 311029
Depends on: 381596
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?

(In reply to comment #2)

This is nowhere near the point that I'm looking for reviews - just backing up work in bugzilla.
Attached patch update to tip (obsolete) — Splinter Review
Attachment #267185 - Attachment is obsolete: true
Attachment #270072 - Attachment is obsolete: true
Attachment #271253 - Attachment is obsolete: true
Attachment #271910 - Attachment is patch: true
Attachment #271910 - Attachment mime type: application/octet-stream → text/plain
Attached patch wip - full lighting filters (obsolete) — Splinter Review
Attachment #271910 - Attachment is obsolete: true
Attachment #272712 - Attachment is obsolete: true
Attachment #272713 - Attachment is obsolete: true
Attachment #273025 - Flags: review?
Attachment #273025 - Flags: review? → review?(dbaron)
Attached patch non-style section (obsolete) — Splinter Review
New competitor for slowest filter...
Attachment #273026 - Flags: review?(longsonr)
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".
Attachment #273025 - Flags: review?(dbaron) → review-
Depends on: 316908
No longer depends on: 381596
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.
(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.
Attachment #273025 - Attachment is obsolete: true
Attachment #273451 - Flags: review?(dbaron)
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.
Attachment #273026 - Attachment is obsolete: true
Attachment #273026 - Flags: review?(longsonr)
Attachment #273452 - Flags: review?(longsonr)
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.
Attachment #273451 - Flags: superreview+
Attachment #273451 - Flags: review?(dbaron)
Attachment #273451 - Flags: review+
Attachment #273452 - Flags: review?(longsonr) → review+
Attachment #273452 - Flags: superreview?(roc)
+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.
(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

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.
Attachment #273452 - Attachment is obsolete: true
Attachment #273797 - Flags: superreview?(roc)
Attachment #273452 - Flags: superreview?(roc)
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)
(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 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.
Attachment #273797 - Flags: superreview?(roc) → superreview+
OK. I understand now.
Checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
looks like this added 26K of code size. I guess it's mostly DOM glue stuff so not much we can do about it.
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.
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/ )
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?
Please file new bugs for problems encountered with the lighting filters.
Blocks: 411023
You need to log in before you can comment on or make changes to this bug.