Bug 32197 - feDiffuseLighting filter is not implemented
Summary: feDiffuseLighting filter is not implemented
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 68469 26389 32199
  Show dependency treegraph
 
Reported: 2009-12-06 12:14 PST by Dirk Schulze
Modified: 2014-05-12 05:54 PDT (History)
7 users (show)

See Also:


Attachments
first draft (15.23 KB, patch)
2010-05-03 07:02 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
example screenshot (20.65 KB, image/png)
2010-05-03 07:04 PDT, Zoltan Herczeg
no flags Details
first draft again (26.35 KB, patch)
2010-05-03 10:25 PDT, Zoltan Herczeg
krit: review-
Details | Formatted Diff | Diff
next draft patch (32.67 KB, patch)
2010-05-05 05:32 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
diffuse lighting filter (43.42 KB, image/png)
2010-05-05 05:42 PDT, Zoltan Herczeg
no flags Details
specular lighting filter (58.05 KB, image/png)
2010-05-05 05:44 PDT, Zoltan Herczeg
no flags Details
light types (67.09 KB, image/png)
2010-05-05 05:46 PDT, Zoltan Herczeg
no flags Details
fixing the minor issues (36.87 KB, patch)
2010-05-05 06:26 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
fixing mac warnings (36.93 KB, patch)
2010-05-05 06:47 PDT, Zoltan Herczeg
zimmermann: review-
Details | Formatted Diff | Diff
redesigned patch (44.59 KB, patch)
2010-05-11 06:22 PDT, Zoltan Herczeg
zimmermann: review-
Details | Formatted Diff | Diff
final patch (hopefully) (279.19 KB, patch)
2010-05-12 02:31 PDT, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
final patch (hopefully) (279.30 KB, patch)
2010-05-12 03:11 PDT, Zoltan Herczeg
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2009-12-06 12:14:43 PST
feDiffuseLighting filter is not implemented
Comment 1 Zoltan Herczeg 2010-05-03 07:02:26 PDT
Created attachment 54924 [details]
first draft

Use -grapcssystem raster on Qt, or an OS which fully support QPixmaps
Comment 2 Zoltan Herczeg 2010-05-03 07:04:14 PDT
Created attachment 54925 [details]
example screenshot

Proof of concept in QtLauncher:

<defs>
<filter id="MyFilter" filterUnits="userSpaceOnUse" x="0" y="0" width="200" height="120">
<feGaussianBlur in="SourceAlpha" stdDeviation="4" result="blur"/>

<feDiffuseLighting in="blur" surfaceScale="5" lighting-color="#00bb00" result="specOut">
<------><fePointLight x="-5000" y="-10000" z="20000"/>
</feDiffuseLighting>

</filter>
</defs>
Comment 3 Zoltan Herczeg 2010-05-03 07:05:18 PDT
Interesting to know: floats, like specularConstant=".75" is not accepted by WebKit, they are converted to 0.0 . I think this is a bug.
Comment 4 Zoltan Herczeg 2010-05-03 10:25:25 PDT
Created attachment 54937 [details]
first draft again

Argh, the new files were missing from the previous patch.
Comment 5 WebKit Review Bot 2010-05-03 10:30:17 PDT
Attachment 54937 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGLightSource.cpp:25:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Early Warning System Bot 2010-05-03 10:33:59 PDT
Attachment 54937 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1975052
Comment 7 Eric Seidel (no email) 2010-05-03 10:35:16 PDT
Attachment 54937 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1963043
Comment 8 Dirk Schulze 2010-05-03 10:43:56 PDT
Comment on attachment 54937 [details]
first draft again

At first, the patch misses ChangeLogs. You did not add any new LayoutTests, nor
did you update any current LayoutTests.
Also, what does "Use -grapcssystem raster on Qt, or an OS which fully support
QPixmaps
" mean? Only Qt will display this effect? The current SVG filters
implementation is meant to be platform aware.

WebCore/WebCore.pro:1813
 +      svg/graphics/filters/SVGFEMerge.h \
Other build sytems like GNUMakefile, Gyp, Mac, Win, Android... need an update
too.


WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:32
 +  FEDiffuseLighting::FEDiffuseLighting(FilterEffect* in, const Color&
lightingColor, const float& surfaceScale,
There is nothing left of FEDiffuseLighting, maybe you should think about
getting rid of FEDiffuseLighting and FESpecularLighting and directly create
FELighting with an enum on the beloonging SVGFE*Element's.

WebCore/svg/graphics/filters/SVGFELighting.cpp:106
 +      float surfaceScale = m_surfaceScale / 255.0;
255.0f

WebCore/svg/graphics/filters/SVGLightSource.cpp:67
 +      if (ls > 0) {
if (ls)

WebCore/svg/graphics/filters/SVGLightSource.cpp:83
 +      float azimuth = m_azimuth * M_PI / 180;
Don't we have constants for M_PI / 180? Otherwise you should create one:
kDegreeToRad or so.

Is it a complete implementation of all
lighting effects? Both of the two effects call FELighting. Are more tests affected by this
patch? How do the SVG tests of the official SVG test suite look like?
r- so far
Comment 9 Zoltan Herczeg 2010-05-03 11:08:53 PDT
This is just a draft patch, not a finished one, so ChangeLog and build systems can be wait. What I need at this stage is the guidance of a reviewer :) )As Darin said before)

The question here is do you like my appoach, or you would prefer something else. In my patch, I combind the diffuse and specular lighting interfaces to a common base class. This base class contains all common, and own arguments. It makes the implementation much easier, but has a space overhead. I think this overhead is acceptable because of the low number of such objects. Do you agree? I mean you said "I should think about...", but you are the reviewer not me :) Would you prefer a patch doing that? Is that fit for the current implementation style or not?

The other thing is how the lighting source interface is changed. I have added members to the interface which is used when the painting happens. This is not a thread safe approach, but faster than passing the same arguments again and again. This is a balance question, do you prefer speed or thread safe operation?

I put this patch here, because only the diffuse lighting part is finished, except one thing: I don't understand the purpose of kernel units. The official definition is fuzzy, and firefox results for different kernel units are strange. It makes the image "dirty", I can't describe it better. Lots of random pixels appear. I thought it is kind of a scaling argument, but I am not sure anymore.

Also interesting, that the cut-off argument of spot light is not used by the spot light apply formulas. I could create rules for that, but not sure that is the best way, since I should follow the rules in the standard.


> Don't we have constants for M_PI / 180?
Where? Or where to put DegreeToRad?

First I plan to finish the code, the pixel tests can wait.
Comment 10 Dirk Schulze 2010-05-03 11:40:01 PDT
(In reply to comment #9)
> This is just a draft patch, not a finished one, so ChangeLog and build systems
> can be wait. What I need at this stage is the guidance of a reviewer :) )As
> Darin said before)
> 
> The question here is do you like my appoach, or you would prefer something
> else. In my patch, I combind the diffuse and specular lighting interfaces to a
> common base class. This base class contains all common, and own arguments. It
> makes the implementation much easier, but has a space overhead. I think this
> overhead is acceptable because of the low number of such objects. Do you agree?
> I mean you said "I should think about...", but you are the reviewer not me :)
> Would you prefer a patch doing that? Is that fit for the current implementation
> style or not?
> 
> The other thing is how the lighting source interface is changed. I have added
> members to the interface which is used when the painting happens. This is not a
> thread safe approach, but faster than passing the same arguments again and
> again. This is a balance question, do you prefer speed or thread safe
> operation?
> 
> I put this patch here, because only the diffuse lighting part is finished,
> except one thing: I don't understand the purpose of kernel units. The official
> definition is fuzzy, and firefox results for different kernel units are
> strange. It makes the image "dirty", I can't describe it better. Lots of random
> pixels appear. I thought it is kind of a scaling argument, but I am not sure
> anymore.
> 
> Also interesting, that the cut-off argument of spot light is not used by the
> spot light apply formulas. I could create rules for that, but not sure that is
> the best way, since I should follow the rules in the standard.
> 
> 
> > Don't we have constants for M_PI / 180?
> Where? Or where to put DegreeToRad?
> 
> First I plan to finish the code, the pixel tests can wait.

The problem on combining the both effects is on changing the effects (by script or animation). We'll recreate the complete filter on changing one effect, but this should change in the future and only effects that need updates should be rerendered. So we might leave the FEDiffuseLighting.cpp.
For the graphic side of implementation, you should ask a graphic guy. Oliver Hunt and others have experience here.
Comment 11 Zoltan Herczeg 2010-05-03 11:53:43 PDT
About the "-graphcssystem raster" option. Today, I printed out the depth() meber of a QPixmap (X11 version), and it was 24. For an 8 bit RGB implementation, that means ther is no alpha channel. In practice, you have a 1 bit alpha cahnnel (the pixel is transparent or not), which is probably implemented by a special constant, or a bit map (1 bit for each pixel), I don't really know. 1 bit alpha channel is not enough for SVG effects. I.e: the lighting effects use the alpha channel as height vector.

One more interesting thing. Here is a code snippet:

w, h -> width height, anything > 0 will do

QPixmap pixmap(w, h);
pixmap.fill(Qt::transparent);
QPainter p(&pixmap);
p.fillRect(0, 0, w, h, Qt::red)
p.end();
pixmap.save("save.png");

On X11, you will see an empty image
If you comment out the .fill line, you will see a red box.
Comment 12 Zoltan Herczeg 2010-05-03 11:58:24 PDT
> The problem on combining the both effects is on changing the effects (by script
> or animation). We'll recreate the complete filter on changing one effect, but
> this should change in the future and only effects that need updates should be
> rerendered. So we might leave the FEDiffuseLighting.cpp.

That was exactly my thought. Ok, so I will keep the current classes. This apprach is more rbust for changes.

> For the graphic side of implementation, you should ask a graphic guy. Oliver
> Hunt and others have experience here.

CC'ing him. No matter where I am heading in WebKit, we always meet :)
Comment 13 Zoltan Herczeg 2010-05-05 05:32:27 PDT
Created attachment 55111 [details]
next draft patch

This patch is still a draft!

The good news is that the light tests (http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-index.html) are quite nice. I will post some screenshot soon. Well, even better than Firefox, so I am quite happy with the results.
Comment 14 WebKit Review Bot 2010-05-05 05:38:53 PDT
Attachment 55111 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGLightSource.cpp:26:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/svg/graphics/filters/SVGLightSource.cpp:60:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/graphics/filters/SVGLightSource.cpp:77:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Zoltan Herczeg 2010-05-05 05:42:47 PDT
Created attachment 55112 [details]
diffuse lighting filter
Comment 16 Zoltan Herczeg 2010-05-05 05:44:19 PDT
Created attachment 55113 [details]
specular lighting filter

Just realized that the color components are reversed :D
Comment 17 Zoltan Herczeg 2010-05-05 05:46:16 PDT
Created attachment 55114 [details]
light types

I am proud of the anti-aliasing of the spot light cone. Much better than Firefox, and matches better to the image.
Comment 18 Zoltan Herczeg 2010-05-05 06:26:52 PDT
Created attachment 55116 [details]
fixing the minor issues
Comment 19 WebKit Review Bot 2010-05-05 06:31:09 PDT
Attachment 55116 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Eric Seidel (no email) 2010-05-05 06:34:20 PDT
Attachment 55116 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2128040
Comment 21 Zoltan Herczeg 2010-05-05 06:47:44 PDT
Created attachment 55119 [details]
fixing mac warnings

Fixing warnings like: "warning: implicit conversion shortens 64-bit value into a 32-bit value"
Comment 22 Eric Seidel (no email) 2010-05-05 06:54:15 PDT
Attachment 55119 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2128041
Comment 23 WebKit Review Bot 2010-05-05 06:56:24 PDT
Attachment 55119 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Nikolas Zimmermann 2010-05-07 07:11:01 PDT
Comment on attachment 55119 [details]
fixing mac warnings

WebCore/svg/graphics/filters/SVGDistantLightSource.h:41
 +          virtual void initVectors(PaintVectors*);
I'd name it initLightVectors, the name is too general for my taste. PaintVectors doesn't sound too great as well, though it's a subclass of LightSource so it's fine I guess.

WebCore/svg/graphics/filters/SVGDistantLightSource.h:42
 +          virtual void updateVectors(PaintVectors*, int, int, float);
Can you please add the parameter names, int, int, float is too confusing.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:32
 +  FEDiffuseLighting::FEDiffuseLighting(FilterEffect* in, const Color& lightingColor, const float& surfaceScale,
Not introduced by you, but I think we should really use "float" here, not "const float&", there's almost no gain, and WebKit in general does not pass POD types as const-references.

WebCore/svg/graphics/filters/SVGFELighting.cpp:40
 +  FELighting::FELighting(LightingType lightingType, FilterEffect* in, const Color& lightingColor, const float& surfaceScale,
Ditto.

WebCore/svg/graphics/filters/SVGFELighting.cpp:57-58
 + #define UPLEFT \
 +      static_cast<int>(pixels->get(offs - widthM4 - 4 + 3))
I dislike these macros. I see the idea, but how about adding static inlines like this:
static inline int upLeftPixelValue(LightingData& data)
{
    return static_cast<int>(pixels->get(data.offset - data.widthMultipledFourTimes - 4 + 3);
}

LightingData could be a simple struct, constructed in the drawLighing function, just holding a pointer to the CanvasPixelArray, and the variables you need in the process.
This would make the code more readable I think, and it's easier to spot compilation errors etc.
Do you think it would be too much overhead? I'd think the compiler would generate the same code, if you'd define the functions as static inlines, instead of using macros.

Same for all the other macros.

WebCore/svg/graphics/filters/SVGFELighting.cpp:76
 +  #define SETPIXEL(lightX, lightY, factorX, normalX, factorY, normalY) \
SETPIXEL should also be a static inline helper function, taking the LightingData structure.
(LightingData also needs to hold the normalVectors variable, of course, forgot to mention above).

WebCore/svg/graphics/filters/SVGFELighting.cpp:78
 +      normalVector.setX((factorX) * static_cast<float>(normalX) * surfaceScale); \
Unneeded parenthesis around factorX.

WebCore/svg/graphics/filters/SVGFELighting.cpp:79
 +      normalVector.setY((factorY) * static_cast<float>(normalY) * surfaceScale); \
Ditto.

WebCore/svg/graphics/filters/SVGFELighting.cpp:107
 +      // TODO: do something if width or height is 1 pixel. Now the filter does nothing
Name it FIXME, this is prefereed WebKit style.

WebCore/svg/graphics/filters/SVGFELighting.cpp:114
 +      int widthM4 = width << 2;
Needs a better name, maybe the explicit "withMultipliedByFour".

WebCore/svg/graphics/filters/SVGFELighting.cpp:115
 +      int widthS1 = width - 1;
Same here.

WebCore/svg/graphics/filters/SVGFELighting.cpp:116
 +      int heightS1 = height - 1;
Same here.

WebCore/svg/graphics/filters/SVGFELighting.cpp:117
 +      int offs;
Please name it "offset", no abbrevations in WebKit code.

WebCore/svg/graphics/filters/SVGFELighting.cpp:193
 +               pixels->set(i, (unsigned char)0xff);
Please use  c style cast, or maybe even better a "static unsigned char s_fooColorValue = 0xff" outside the loop to save the cast every time in the for loop.

WebCore/svg/graphics/filters/SVGFELighting.cpp:194
 +      else
A parenthesis should be added around the block below. else { ... }

WebCore/svg/graphics/filters/SVGFELighting.cpp:199
 +               pixels->set(i + 3, a1 >= a2 ? (a1 >= a3 ? a1 : a3) : (a2 >= a3 ? a2 : a3));
I'd love a comment here, because it's not clear for me on first sight what's happening :-) Please use more explicit variable names (aka. longer & descriptive).

WebCore/svg/graphics/filters/SVGFELighting.cpp:211
 +      if (!getEffectContext())
Ewww, getEffectContext() - who named it this way? Dirk? ;-)

WebCore/svg/graphics/filters/SVGFELighting.h:59
 +          LightingType m_lightingType;
I'd reorder the variables here, sorted by type, to possibly allow the compiler to pad better.

WebCore/svg/graphics/filters/SVGLightSource.cpp:35
 +  #define M_PI 3.14159265358979323846
No need to define M_PI and include math.h. Just include wtf/MathExtras.h, it contains a piFloat gobal variable.

WebCore/svg/graphics/filters/SVGLightSource.cpp:38
 +  static const double degToRad = M_PI / 180.0f;
Also available in MathExtras.h, as inlined helper function deg2Rad.

WebCore/svg/graphics/filters/SVGLightSource.cpp:94
 +      float ls = paintVectors->lightVector * paintVectors->directionVector;
Please use a more descriptive variable name.

WebCore/svg/graphics/filters/SVGLightSource.cpp:104
 +      float p;
Same here.

Other than those simple comments, an _excellent_ patch, good job Zoltan!
Looking forward to the revised version.
Comment 25 Zoltan Herczeg 2010-05-11 06:22:11 PDT
Created attachment 55696 [details]
redesigned patch

Hopefully addressed all requests from Nikolas Zimmermann.
Comment 26 WebKit Review Bot 2010-05-11 06:28:00 PDT
Attachment 55696 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/svg/graphics/filters/SVGFESpecularLighting.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/svg/graphics/filters/SVGFELighting.h:41:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Eric Seidel (no email) 2010-05-11 06:36:45 PDT
Attachment 55696 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2227128
Comment 28 Nikolas Zimmermann 2010-05-11 06:49:38 PDT
Comment on attachment 55696 [details]
redesigned patch

WebCore/platform/graphics/FloatPoint3D.h:30
 +  class FloatPoint;
If you're including FloatPoint.h, the class forward can go away. I assume you need the include because you've inlined the FloatPoint copy-construtor.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:33
 +      const float diffuseConstant, const float kernelUnitLengthX, const float kernelUnitLengthY, PassRefPtr<LightSource> lightSource)
Please replace "const float" by "float". You've only changed it from const-references to const objects, there's no gain in using const here.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:39
 +      const float surfaceScale, const float diffuseConstant, const float kernelUnitLengthX,
Ditto.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.h:34
 +          static PassRefPtr<FEDiffuseLighting> create(FilterEffect*, const Color&, const float, const float,
Ditto. NOTE: Please leave "const Color&" - non-POD types should still be passed as const-refererences, only POD types like float/int/etc.. not.

WebCore/svg/graphics/filters/SVGFELighting.cpp:39
 +  FELighting::FELighting(LightingType lightingType, FilterEffect* in, const Color& lightingColor, const float surfaceScale,
Ditto.

WebCore/svg/graphics/filters/SVGFELighting.cpp:56
 +  const static int pixelSize = 4;
I'd prefer another naming scheme to easily spot statics while reading the code, something like "cPixelSize" or "gPixelSize". The coding style guideline doesn't say anything explicit here, several places in the code use it like you've done, others use a "c" prefix. 

WebCore/svg/graphics/filters/SVGFELighting.cpp:75
 +  static inline int upLeftPixelValue(LightingData& data)
 +  {
 +      return static_cast<int>(data.pixels->get(data.offset - data.widthMultipliedByPixelSize - pixelSize + alphaChannelOffset));
Excellent!

WebCore/svg/graphics/filters/SVGFELighting.cpp:135
 +          if (data.specularExponent == 1.0)
Should be "1.0f".

WebCore/svg/graphics/filters/SVGFELighting.cpp:164
 +      data.lightingType = m_lightingType;
Hmm, isn't it possible to just store a pointer to the current FELighting object in LightingData? Otherwhise you're duplicating the storage of lightingType/lightSsource/diffuseConstant/etc..


WebCore/svg/graphics/filters/SVGFELighting.cpp:250
 +              data.pixels->set(i, static_cast<unsigned char>(0xff));
Please move the magic 0xff value to a constant out-of-the-loop "cMyColorValue" or something. To avoid doing this cast on every iteration of the loop.

WebCore/svg/graphics/filters/SVGFELighting.h:41
 +      class CanvasPixelArray;
Indention is wrong in this file, code inside namespace should not be indented.

WebCore/svg/graphics/filters/SVGFESpecularLighting.cpp:32
 +  FESpecularLighting::FESpecularLighting(FilterEffect* in, const Color& lightingColor, const float surfaceScale,
The "const float" issue again, please use "float".

WebCore/svg/graphics/filters/SVGLightSource.h:72
 +          virtual void updatePaintingData(PaintingData&, int x, int y, float z) = 0;
Hm, could you explain again why x/y are int and float is z? I may have overlooked the obvious reason.

Only very minor issues remaining, almost there!
Comment 29 Zoltan Herczeg 2010-05-11 07:40:25 PDT
Thank you. I only have a few questions, where the answer is not obvious (for me at :) )

> WebCore/svg/graphics/filters/SVGFEDiffuseLighting.h:34
>  +          static PassRefPtr<FEDiffuseLighting> create(FilterEffect*, const Color&, const float, const float,
> Ditto. NOTE: Please leave "const Color&" - non-POD types should still be passed as const-refererences, only POD types like float/int/etc.. not.

There is no change here, isn't it? It has already been defined as "const Color&". Passing pointers is faster, of course.

> WebCore/svg/graphics/filters/SVGFELighting.cpp:164
>  +      data.lightingType = m_lightingType;
> Hmm, isn't it possible to just store a pointer to the current FELighting object in LightingData? Otherwhise you're duplicating the storage of lightingType/lightSsource/diffuseConstant/etc..

The inline functions are not friend functions of FELighting, thus the protected members are not accessible. There are several possible workaround for that, and I chose the simplest one (without header modification). Shall I make them a member function? Or friend function (moving LightingData inside FELighting)? Or something else?

> WebCore/svg/graphics/filters/SVGFELighting.cpp:250
>  +              data.pixels->set(i, static_cast<unsigned char>(0xff));
> Please move the magic 0xff value to a constant out-of-the-loop "cMyColorValue" or something. To avoid doing this cast on every iteration of the loop.

You mentioned this before, but "static_cast<unsigned char>(0xff)" is a compile time constant, and there is no runtime casting. But I could move it out anyway.

> 
> WebCore/svg/graphics/filters/SVGFELighting.h:41
>  +      class CanvasPixelArray;
> Indention is wrong in this file, code inside namespace should not be indented.

The style checker mention this as well, but most classes in WebKit is indented in the Header file, including other headers in the same directory.

> Hm, could you explain again why x/y are int and float is z? I may have overlooked the obvious reason.

I will put a comment there. z = alpha value (opaque treated as 1.0) scaled by a user specified constant. That constant is a float.


Next time I will add a ChangeLog and update mac pixel tests as well.
Comment 30 Nikolas Zimmermann 2010-05-11 08:05:36 PDT
(In reply to comment #29)
> Thank you. I only have a few questions, where the answer is not obvious (for me at :) )
> 
> > WebCore/svg/graphics/filters/SVGFEDiffuseLighting.h:34
> >  +          static PassRefPtr<FEDiffuseLighting> create(FilterEffect*, const Color&, const float, const float,
> > Ditto. NOTE: Please leave "const Color&" - non-POD types should still be passed as const-refererences, only POD types like float/int/etc.. not.
> 
> There is no change here, isn't it? It has already been defined as "const Color&". Passing pointers is faster, of course.
Right, just wanted to mention that you shouldn't change that one :-)

 
> > WebCore/svg/graphics/filters/SVGFELighting.cpp:164
> >  +      data.lightingType = m_lightingType;
> > Hmm, isn't it possible to just store a pointer to the current FELighting object in LightingData? Otherwhise you're duplicating the storage of lightingType/lightSsource/diffuseConstant/etc..
> 
> The inline functions are not friend functions of FELighting, thus the protected members are not accessible. There are several possible workaround for that, and I chose the simplest one (without header modification). Shall I make them a member function? Or friend function (moving LightingData inside FELighting)? Or something else?
Friendship sounds the fastest solution to me, choose whatever you like, if it avoids duplicating memory.

> 
> > WebCore/svg/graphics/filters/SVGFELighting.cpp:250
> >  +              data.pixels->set(i, static_cast<unsigned char>(0xff));
> > Please move the magic 0xff value to a constant out-of-the-loop "cMyColorValue" or something. To avoid doing this cast on every iteration of the loop.
> 
> You mentioned this before, but "static_cast<unsigned char>(0xff)" is a compile time constant, and there is no runtime casting. But I could move it out anyway.
Oh that's true of course. Hm, I still find it easier to read.

> 
> > 
> > WebCore/svg/graphics/filters/SVGFELighting.h:41
> >  +      class CanvasPixelArray;
> > Indention is wrong in this file, code inside namespace should not be indented.
> 
> The style checker mention this as well, but most classes in WebKit is indented in the Header file, including other headers in the same directory.
Yeah it's a "new rule" compared to when this code appeared in trunk. We're fixing them over the time whenver we see the style bot complain.

> 
> > Hm, could you explain again why x/y are int and float is z? I may have overlooked the obvious reason.
> 
> I will put a comment there. z = alpha value (opaque treated as 1.0) scaled by a user specified constant. That constant is a float.
Good.

> 
> 
> Next time I will add a ChangeLog and update mac pixel tests as well.
Excellent!
Comment 31 Zoltan Herczeg 2010-05-12 02:31:46 PDT
Created attachment 55820 [details]
final patch (hopefully)

The patch is big because of the pixel tests.
Comment 32 Zoltan Herczeg 2010-05-12 03:11:35 PDT
Created attachment 55824 [details]
final patch (hopefully)
Comment 33 Nikolas Zimmermann 2010-05-12 03:15:19 PDT
Comment on attachment 55824 [details]
final patch (hopefully)

r=me.
Comment 34 Zoltan Herczeg 2010-05-12 06:06:09 PDT
Landed in r59220 http://trac.webkit.org/changeset/59220. A build-fix was needed for Win and Chromium, and landed in r59221.

Closing bug.