Bug 32197

Created attachment 54924 [details] first draft Use -grapcssystem raster on Qt, or an OS which fully support QPixmaps

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>

Interesting to know: floats, like specularConstant=".75" is not accepted by WebKit, they are converted to 0.0 . I think this is a bug.

Created attachment 54937 [details] first draft again Argh, the new files were missing from the previous patch.

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.

Attachment 54937 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1975052

Attachment 54937 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1963043

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

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.

(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.

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.

> 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 :)

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.

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.

Created attachment 55112 [details] diffuse lighting filter

Created attachment 55113 [details] specular lighting filter Just realized that the color components are reversed :D

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.

Created attachment 55116 [details] fixing the minor issues

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.

Attachment 55116 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2128040

Created attachment 55119 [details] fixing mac warnings Fixing warnings like: "warning: implicit conversion shortens 64-bit value into a 32-bit value"

Attachment 55119 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2128041

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 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.

Created attachment 55696 [details] redesigned patch Hopefully addressed all requests from Nikolas Zimmermann.

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.

Attachment 55696 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2227128

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!

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.

(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!

Created attachment 55820 [details] final patch (hopefully) The patch is big because of the pixel tests.

Created attachment 55824 [details] final patch (hopefully)

Comment on attachment 55824 [details] final patch (hopefully) r=me.

Landed in r59220 http://trac.webkit.org/changeset/59220. A build-fix was needed for Win and Chromium, and landed in r59221. Closing bug.