Bug 113059

Created attachment 195114 [details] Patch

Attachment 195114 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-limitingConeAngle-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-pointsAtX-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-pointsAtY-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-pointsAtZ-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-specularExponent-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-x-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-y-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-dom-z-attr-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-limitingConeAngle-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-pointsAtX-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-pointsAtY-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-pointsAtZ-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-specularExponent-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-x-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-y-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/dynamic-updates/SVGFESpotLightElement-svgdom-z-prop-expected.png', u'LayoutTests/platform/chromium-linux/svg/filters/light-source-positioning-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/gtk/TestExpectations', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/qt/TestExpectations', u'LayoutTests/svg/dynamic-updates/SVGFESpotLightElement-dom-pointsAtX-attr-expected.txt', u'LayoutTests/svg/dynamic-updates/SVGFESpotLightElement-dom-x-attr-expected.txt', u'LayoutTests/svg/dynamic-updates/SVGFESpotLightElement-svgdom-pointsAtX-prop-expected.txt', u'LayoutTests/svg/dynamic-updates/SVGFESpotLightElement-svgdom-x-prop-expected.txt', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-limitingConeAngle-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-pointsAtX-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-pointsAtY-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-pointsAtZ-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-specularExponent-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-x-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-y-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-dom-z-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-limitingConeAngle-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-pointsAtX-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-pointsAtY-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-pointsAtZ-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-specularExponent-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-x-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-y-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFESpotLightElement-svgdom-z-prop.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-dom-primitiveUnits-attr.js', u'LayoutTests/svg/dynamic-updates/script-tests/SVGFilterElement-svgdom-primitiveUnits-prop.js', u'LayoutTests/svg/filters/light-source-positioning-expected.txt', u'LayoutTests/svg/filters/light-source-positioning.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h', u'Source/WebCore/platform/graphics/filters/DistantLightSource.cpp', u'Source/WebCore/platform/graphics/filters/DistantLightSource.h', u'Source/WebCore/platform/graphics/filters/FELighting.cpp', u'Source/WebCore/platform/graphics/filters/Filter.h', u'Source/WebCore/platform/graphics/filters/LightSource.h', u'Source/WebCore/platform/graphics/filters/PointLightSource.cpp', u'Source/WebCore/platform/graphics/filters/PointLightSource.h', u'Source/WebCore/platform/graphics/filters/SpotLightSource.cpp', u'Source/WebCore/platform/graphics/filters/SpotLightSource.h', u'Source/WebCore/platform/graphics/filters/skia/FELightingSkia.cpp', u'Source/WebCore/svg/graphics/filters/SVGFilter.cpp', u'Source/WebCore/svg/graphics/filters/SVGFilter.h']" exit_code: 1 Source/WebCore/platform/graphics/filters/LightSource.h:92: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/platform/graphics/filters/LightSource.h:93: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/platform/graphics/filters/LightSource.h:94: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 195324 [details] Patch check-webkit-style is crimping my style :)

Comment on attachment 195324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195324&action=review > Source/WebCore/platform/graphics/filters/LightSource.h:96 > + FloatPoint3D mapPointToFilter(const FloatPoint3D& point, const Filter* filter) const > + { I am not sure if I understand the idea correctly. What you want is the point of origin of either the obb or the userspace of the target element transformed to our filter space, right? This is not necessarily identical with the filter region origin. > Source/WebCore/platform/graphics/filters/PointLightSource.h:44 > + virtual void initPaintingData(PaintingData&, const Filter*); I wonder why we not just pass the necessary values here instead of passing Filter. It just looks like we should calculate all these values before creating the point source. > Source/WebCore/platform/graphics/filters/PointLightSource.h:63 > + mutable FloatPoint3D m_filterPosition; > + mutable bool m_filterDataIsDirty : 1; Especially having this mutable FloatPoint doesn't look really nice and that they are in all light sources instead of the global class.

Thanks Dirk, let me do another pass on this. (In reply to comment #4) > (From update of attachment 195324 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195324&action=review > > > Source/WebCore/platform/graphics/filters/LightSource.h:96 > > + FloatPoint3D mapPointToFilter(const FloatPoint3D& point, const Filter* filter) const > > + { > > I am not sure if I understand the idea correctly. What you want is the point of origin of either the obb or the userspace of the target element transformed to our filter space, right? Yes, that is the intent but the implementation is somewhat obfuscated by the overridden behavior of apply{Horizontal,Vertical}Scale and filterOffset. > This is not necessarily identical with the filter region origin. Good point, applying the scale upfront is probably wrong. I guess the tests only cover the scale == 1.0 case otherwise this should have been obvious... Will need add a fiterRes test too. > > Source/WebCore/platform/graphics/filters/PointLightSource.h:44 > > + virtual void initPaintingData(PaintingData&, const Filter*); > > I wonder why we not just pass the necessary values here instead of passing Filter. It just looks like we should calculate all these values before creating the point source. The problem is that the light sources are long lived and their attributes can be updated at a later time via JS wrappers/setters. On that path we do not have immediate access to the corresponding Filter, and even if we did, encapsulating the logic here seems like a good idea (instead of having all setter callers plus platform impls duplicate the same calculations). > > Source/WebCore/platform/graphics/filters/PointLightSource.h:63 > > + mutable FloatPoint3D m_filterPosition; > > + mutable bool m_filterDataIsDirty : 1; > > Especially having this mutable FloatPoint doesn't look really nice and that they are in all light sources instead of the global class. I'm not sure I understand this: the filter space "shadowing" fields are tied to their corresponding attributes, so they do need to live in the same class as the actual fields they're based on (PointLightSource::m_position, SpotLightSource::m_position, SpotLightSource::m_direction). The reason I've marked them mutable is that they're not really part of the logical state of the object: they're derived on demand (think cached values) - hence they should not affect method const-ness.

A very closely related bug: light source positions and sizes also aren't being scaled relative to the viewBox. Look at this: http://codepen.io/anon/pen/FEIhL Stretch the browser viewport wide and narrow, and notice that the position of the spotlight isn't scaling. The "viewbox" 1.1 spec says "If specified, an additional transformation is applied to all descendants of the given element to achieve the specified effect." Since the filter in this CodePen is a descendant of the element to which the viewBox is applied, it's squarely within the viewBox's "scope", and the units should be scaled. I realize this is distinct from the primitiveUnits="objectBoundingBox" use case. I can file this separately if necessary, but I thought it might make sense to bring up here.

Florin, does it still make sense to review the patch? It might not apply anymore, I am more interested if the fix is still valid.

(In reply to comment #7) > Florin, does it still make sense to review the patch? It might not apply anymore, I am more interested if the fix is still valid. I think so, yes. I may have to review it myself to recall the details, but from a quick glance I think it still makes sense.

(In reply to comment #8) > (In reply to comment #7) > > Florin, does it still make sense to review the patch? It might not apply anymore, I am more interested if the fix is still valid. > > I think so, yes. I may have to review it myself to recall the details, but from a quick glance I think it still makes sense. sugoi@ is working on a fix for this (in a different way) over in Blink: https://codereview.chromium.org/181943003/

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Florin, does it still make sense to review the patch? It might not apply anymore, I am more interested if the fix is still valid. > > > > I think so, yes. I may have to review it myself to recall the details, but from a quick glance I think it still makes sense. > > sugoi@ is working on a fix for this (in a different way) over in Blink: https://codereview.chromium.org/181943003/ Thanks Stephen and Florin!

I'm taking this one. First I started to check what was the current state of other user agents concerning the bug: all are ok (FF, Chrome Canary, even IE) besides WebKit. Initially I started by trying to backport the patch that landed in Blink (it would be nice to share the same implementation for this case): https://gist.github.com/Adenilson/10330991 But later on I noticed that it depends on some new properties (absoluteTransform/inverse, etc) on Filter that are not present on WebKit (plus it would require some changes in RenderLayer to backport it). So, as a fallback, decided to give it a try in Florin's original patch: it required basically no significant changes.

Created attachment 229008 [details] Screenshots of user agents rendering the test case

Created attachment 229009 [details] Patch

Attachment 229009 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/filters/SpotLightSource.h:67: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 229009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229009&action=review Please take a look at the style bot output as well. > Source/WebCore/ChangeLog:20 > + direction for feSpotLight, position for fePointlight. Could you add more descriptions what exactly you are doing to fix this please? > Source/WebCore/ChangeLog:31 > + * platform/graphics/cpu/arm/filters/FELightingNEON.h: > + (WebCore::FELighting::platformApplyNeon): > + * platform/graphics/filters/DistantLightSource.cpp: > + (WebCore::DistantLightSource::initPaintingData): > + * platform/graphics/filters/DistantLightSource.h: > + * platform/graphics/filters/FELighting.cpp: > + (WebCore::FELighting::drawLighting): > + * platform/graphics/filters/Filter.h: Please do more detailed descriptions behind the function names here. It really would help to understand. > Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h:120 > + FloatPoint3D position = pointLightSource->position(filter()); I wonder why you pass a filter() here. Need to check.I guess I can imagine why. > Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h:124 > + floatArguments.lightX = position.x(); > + floatArguments.lightY = position.y(); > + floatArguments.lightZ = position.z(); > floatArguments.padding2 = 0; What is float arguments and why doesn't it simply take a FloatPoint3D? > Source/WebCore/platform/graphics/filters/DistantLightSource.cpp:40 > +void DistantLightSource::initPaintingData(PaintingData& paintingData, const Filter*) There should always be a Filter. therefore, you should be able to pass a reference. Why do you pass it at all? Can't see an instance of the virtual function that is using it? > Source/WebCore/platform/graphics/filters/LightSource.h:101 > + FloatPoint3D mapPointToFilter(const FloatPoint3D& point, const Filter* filter) const > + { > + ASSERT(filter); > + float horizontalUnit = filter->applyHorizontalScale(1); > + float verticalUnit = filter->applyVerticalScale(1); > + > + float adjustedX = point.x() * horizontalUnit - filter->filterOffset().width(); > + float adjustedY = point.y() * verticalUnit - filter->filterOffset().height(); > + float adjustedZ = point.z() * sqrtf((horizontalUnit * horizontalUnit + verticalUnit * verticalUnit) / 2); > + > + return FloatPoint3D(adjustedX, adjustedY, adjustedZ); > + } We usually do scaling operations in Filter itself. Examples are applyHorizontalScale and applyVerticalScale actually. Can't you move this function to Filter as well? Also, don't initialize 3 floats but then create a FloatPoint3D. Create the FloatPoint3D from the beginning: return FloatPoint3D( adjX, adjY, adjZ); Also, can you pass point.x() directly to applyHorizontalScale()? Ditto for point.y() > Source/WebCore/platform/graphics/filters/PointLightSource.cpp:40 > +void PointLightSource::initPaintingData(PaintingData&, const Filter* filter) Ok, missed this one. > Source/WebCore/platform/graphics/filters/PointLightSource.cpp:43 > + if (m_filterDataIsDirty) > + updateFilterData(filter); Is that some kind of optimization? Can you elaborate more? > Source/WebCore/platform/graphics/filters/PointLightSource.h:38 > + const FloatPoint3D& position(const Filter*) const; reference, no pointer. > Source/WebCore/platform/graphics/filters/PointLightSource.h:56 > + void updateFilterData(const Filter*) const; Ditto. > Source/WebCore/platform/graphics/filters/PointLightSource.h:61 > + mutable FloatPoint3D m_filterPosition; > + mutable bool m_filterDataIsDirty : 1; : 1 doesn't make much sense if it is the only argument you want to put in a word. Why is m_filterPosition a member variable? And why does it need to be mutable? > Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:200 > + m_filterPosition = mapPointToFilter(m_position, filter); > + m_filterDirection = mapPointToFilter(m_direction, filter); > + m_filterDataIsDirty = false; Even more new member variables. Why do they need to be mem variables? > Source/WebCore/platform/graphics/filters/SpotLightSource.cpp:215 > +const FloatPoint3D& SpotLightSource::position(const Filter* filter) const > +{ > + if (m_filterDataIsDirty) > + updateFilterData(filter); > + return m_filterPosition; > +} > + > +const FloatPoint3D& SpotLightSource::direction(const Filter* filter) const > +{ > + if (m_filterDataIsDirty) > + updateFilterData(filter); > + return m_filterDirection; > +} Is that the optimization why you need to variables? How often do we compute that per run? Is it really that costly? > Source/WebCore/platform/graphics/filters/SpotLightSource.h:82 > + mutable bool m_filterDataIsDirty : 1; Ditto. See above.

Dirk Thanks for reviewing, I will be submitting a new patch soon.

(In reply to comment #16) > Dirk > > Thanks for reviewing, I will be submitting a new patch soon. Is this bug still being worked on?

(In reply to comment #17) > (In reply to comment #16) > > Dirk > > > > Thanks for reviewing, I will be submitting a new patch soon. > > Is this bug still being worked on? Nope.

(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > Dirk > > > > > > Thanks for reviewing, I will be submitting a new patch soon. > > > > Is this bug still being worked on? > > Nope. It is just currently not worked on. Not not at all.

(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (In reply to comment #16) > > > > Dirk > > > > > > > > Thanks for reviewing, I will be submitting a new patch soon. > > > > > > Is this bug still being worked on? > > > > Nope. > > It is just currently not worked on. Not not at all. Sorry, didn't mean to imply that: I was the bug assignee and since I'm no longer working on it (or planning to), I unassigned myself. But yes, what I meant is "I am no longer working on this bug, feel free to re-assign" :)

Comment on attachment 195324 [details] Patch This patch doesn't apply, and seems to be largely abandoned. Marking r- to get it off of the review queue.

Said, I just CC'd you on this bug in case you can find some use for Florin's work. The test cases seem like they would be useful to add.

Fixed via bug 181147 and bug 181197