Created attachment 442660 [details] Patch

Comment on attachment 442660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442660&action=review > Source/WebCore/platform/graphics/filters/FilterFunction.cpp:74 > + static auto filterNames = makeNeverDestroyed([] { > + FilterNameMap filterNames = { > + { FilterFunction::Type::CSSFilter, "CSSFilter"_s }, > + { FilterFunction::Type::SVGFilter, "SVGFilter"_s }, > + > + { FilterFunction::Type::FEBlend, "FEBlend"_s }, > + { FilterFunction::Type::FEColorMatrix, "FEColorMatrix"_s }, > + { FilterFunction::Type::FEComponentTransfer, "FEComponentTransfer"_s }, > + { FilterFunction::Type::FEComposite, "FEComposite"_s }, > + { FilterFunction::Type::FEConvolveMatrix, "FEConvolveMatrix"_s }, > + { FilterFunction::Type::FEDiffuseLighting, "FEDiffuseLighting"_s }, > + { FilterFunction::Type::FEDisplacementMap, "FEDisplacementMap"_s }, > + { FilterFunction::Type::FEDropShadow, "FEDropShadow"_s }, > + { FilterFunction::Type::FEFlood, "FEFlood"_s }, > + { FilterFunction::Type::FEGaussianBlur, "FEGaussianBlur"_s }, > + { FilterFunction::Type::FEImage, "FEImage"_s }, > + { FilterFunction::Type::FEMerge, "FEMerge"_s }, > + { FilterFunction::Type::FEMorphology, "FEMorphology"_s }, > + { FilterFunction::Type::FEOffset, "FEOffset"_s }, > + { FilterFunction::Type::FESpecularLighting, "FESpecularLighting"_s }, > + { FilterFunction::Type::FETile, "FETile"_s }, > + { FilterFunction::Type::FETurbulence, "FETurbulence"_s }, > + > + { FilterFunction::Type::SourceAlpha, "SourceAlpha"_s }, > + { FilterFunction::Type::SourceGraphic, "SourceGraphic"_s } > + > + }; > + return filterNames; > + }()); > + > + return filterNames.get(); I suggest we use a SortedArrayMap here rather than a HashMap.

Comment on attachment 442660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442660&action=review >> Source/WebCore/platform/graphics/filters/FilterFunction.cpp:74 >> + return filterNames.get(); > > I suggest we use a SortedArrayMap here rather than a HashMap. Or a switch statement.

<rdar://problem/84999239>

Created attachment 443653 [details] Patch

Comment on attachment 442660 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442660&action=review >>> Source/WebCore/platform/graphics/filters/FilterFunction.cpp:74 >>> + return filterNames.get(); >> >> I suggest we use a SortedArrayMap here rather than a HashMap. > > Or a switch statement. I used SortedArrayMap instead of HashMap.

Created attachment 443665 [details] Patch

Comment on attachment 443665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443665&action=review > Source/WebCore/platform/graphics/filters/FilterFunction.h:41 > + CSSFilter, > + SVGFilter, I don't quite understand this. Below, there's a list of specific filters ("FEDropShadow") but CSSFilter/SVGFilter seems to refer to a collection of filters, rather than any specific one. > Source/WebCore/platform/graphics/filters/FilterFunction.h:65 > + FEMaximum = SourceGraphic This seems error-prone. Suppose someone does for (uint8_t i = 0; i < FilterFunction::Type::FEMaximum; ++i) { ... } They wouldn't hit SourceGraphic. > Source/WebCore/platform/graphics/filters/FilterFunction.h:74 > + bool isSVGFilter() const { return m_filterType == Type::SVGFilter; } FESpecularLighting isn't an SVG filter? > Source/WebCore/platform/graphics/filters/FilterFunction.h:82 > + It seems a bit unfortunate that there's nothing common here that does any real work. If there's no shared functionality, why not just say "using FilterFunction = std::variant<Filter, FilterEffect>"?

Comment on attachment 443665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443665&action=review > Source/WebCore/platform/graphics/filters/FilterFunction.cpp:68 > + ASSERT(namesMap.tryGet(filterType)); Seems like to make such call sites more elegant we should add a contains function to SortedArrayMap that calls tryGet and checks for null. > Source/WebCore/platform/graphics/filters/FilterFunction.cpp:69 > + return *namesMap.tryGet(filterType); Why not just use get here?

Comment on attachment 443665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443665&action=review >> Source/WebCore/platform/graphics/filters/FilterFunction.h:41 >> + SVGFilter, > > I don't quite understand this. Below, there's a list of specific filters ("FEDropShadow") but CSSFilter/SVGFilter seems to refer to a collection of filters, rather than any specific one. The whole point of this layering is to allow applying the filter functions from first to last. Currently we do the opposite. We start from the last and we go backward till we reach the first. This new layering will simplify the calculation much and we do not need to store any calculation while applying. Look at this CSS filter: contrast(175%) brightness(103%) url("#gray-scale"); And this SVG <svg style="position: absolute; top: -999999px" xmlns="http://www.w3.org/2000/svg"> <filter id="svgGrayscale"> <feColorMatrix type="saturate" values="0.10"/> </filter> </svg> When applying this filter we should apply them in this order: SourceGraphic, FEComponentTransfer, FEComponentTransfer, SVGFilter. And the SVGFilter should be applied in the following order { SourceGraphic, EFColorMatrix }. Currently we do the opposite. We start form EFColorMatrix and go back till the SourceGraphic of the CSS filter. You can look at Filter as a composite pattern of FilterEffect with the following restriction: 1. CSSFilter has to be a root element it can not be nested in any FilterFunction. 2. SVGFiler can be a root element or a child of CSSFilter. And its children can only be FilterEffects. This is not the ideal design but having two different hierarchies for Filter and FilterEffect will cause a Ref counting problem. >> Source/WebCore/platform/graphics/filters/FilterFunction.h:65 >> + FEMaximum = SourceGraphic > > This seems error-prone. Suppose someone does > > for (uint8_t i = 0; i < FilterFunction::Type::FEMaximum; ++i) { > ... > } > > They wouldn't hit SourceGraphic. I meant FEMaximum will the type of the "last" FilterEffect. I can change FEMinimum to FEFirst to FEMaximum FELast. Will that make it clear? >> Source/WebCore/platform/graphics/filters/FilterFunction.h:74 >> + bool isSVGFilter() const { return m_filterType == Type::SVGFilter; } > > FESpecularLighting isn't an SVG filter? No FESpecularLighting is a FilterEffect. SVGFilter in this context is a collection of FilterEffect and which you define in <svg > <filte> ... </filter> </svg> >> Source/WebCore/platform/graphics/filters/FilterFunction.h:82 >> + > > It seems a bit unfortunate that there's nothing common here that does any real work. If there's no shared functionality, why not just say "using FilterFunction = std::variant<Filter, FilterEffect>"? At least these functions will be added in the future patches. virtual IntOutsets outsets() const { return { }; } virtual bool apply(const Filter&) { return false; } See https://bugs.webkit.org/attachment.cgi?id=443517&action=review

Comment on attachment 443665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443665&action=review >> Source/WebCore/platform/graphics/filters/FilterFunction.cpp:68 >> + ASSERT(namesMap.tryGet(filterType)); > > Seems like to make such call sites more elegant we should add a contains function to SortedArrayMap that calls tryGet and checks for null. Yes and this will match the HashMap interface. >> Source/WebCore/platform/graphics/filters/FilterFunction.cpp:69 >> + return *namesMap.tryGet(filterType); > > Why not just use get here? You are right. I will use get().

Comment on attachment 443665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443665&action=review >>> Source/WebCore/platform/graphics/filters/FilterFunction.h:41 >>> + SVGFilter, >> >> I don't quite understand this. Below, there's a list of specific filters ("FEDropShadow") but CSSFilter/SVGFilter seems to refer to a collection of filters, rather than any specific one. > > The whole point of this layering is to allow applying the filter functions from first to last. Currently we do the opposite. We start from the last and we go backward till we reach the first. This new layering will simplify the calculation much and we do not need to store any calculation while applying. > > Look at this CSS > > filter: contrast(175%) brightness(103%) url("#gray-scale"); > > And this SVG > > <svg style="position: absolute; top: -999999px" xmlns="http://www.w3.org/2000/svg"> > <filter id="svgGrayscale"> > <feColorMatrix type="saturate" values="0.10"/> > </filter> > </svg> > > When applying this filter we should apply them in this order: SourceGraphic, FEComponentTransfer, FEComponentTransfer, SVGFilter. And the SVGFilter should be applied in the following order { SourceGraphic, EFColorMatrix }. Currently we do the opposite. We start form EFColorMatrix and go back till the SourceGraphic of the CSS filter. > > You can look at Filter as a composite pattern of FilterEffect with the following restriction: > > 1. CSSFilter has to be a root element it can not be nested in any FilterFunction. > 2. SVGFiler can be a root element or a child of CSSFilter. And its children can only be FilterEffects. > > This is not the ideal design but having two different hierarchies for Filter and FilterEffect will cause a Ref counting problem. Going backwards makes sense to me - it's a way to guarantee you're only computing what is necessary. Applying the graph forwards means you don't know which nodes/edges will ultimately contribute to the final image, vs which are unnecessary and can be avoided altogether. >>> Source/WebCore/platform/graphics/filters/FilterFunction.h:65 >>> + FEMaximum = SourceGraphic >> >> This seems error-prone. Suppose someone does >> >> for (uint8_t i = 0; i < FilterFunction::Type::FEMaximum; ++i) { >> ... >> } >> >> They wouldn't hit SourceGraphic. > > I meant FEMaximum will the type of the "last" FilterEffect. I can change FEMinimum to FEFirst to FEMaximum FELast. Will that make it clear? Usually this is done by just doing SourceAlpha, SourceGraphic, FEMaximum } that way, looping until < FEMaximum will do the right thing. For an example in another project, see lines 775 - 854 in https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/uchar_8h_source.html >>> Source/WebCore/platform/graphics/filters/FilterFunction.h:82 >>> + >> >> It seems a bit unfortunate that there's nothing common here that does any real work. If there's no shared functionality, why not just say "using FilterFunction = std::variant<Filter, FilterEffect>"? > > At least these functions will be added in the future patches. > > virtual IntOutsets outsets() const { return { }; } > virtual bool apply(const Filter&) { return false; } > > See https://bugs.webkit.org/attachment.cgi?id=443517&action=review Ah, okay, cool. I misunderstood that these were coming later.

Created attachment 443739 [details] Patch

Comment on attachment 443665 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443665&action=review >>>> Source/WebCore/platform/graphics/filters/FilterFunction.h:65 >>>> + FEMaximum = SourceGraphic >>> >>> This seems error-prone. Suppose someone does >>> >>> for (uint8_t i = 0; i < FilterFunction::Type::FEMaximum; ++i) { >>> ... >>> } >>> >>> They wouldn't hit SourceGraphic. >> >> I meant FEMaximum will the type of the "last" FilterEffect. I can change FEMinimum to FEFirst to FEMaximum FELast. Will that make it clear? > > Usually this is done by just doing > > SourceAlpha, > SourceGraphic, > > FEMaximum > } > > that way, looping until < FEMaximum will do the right thing. > > For an example in another project, see lines 775 - 854 in https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/uchar_8h_source.html Adding FEMaximum as a separate enum value will make using FilterFunction::Type in a switch statement a little bit complicated. So I am using FEFirst and FELast since this will not require adding unused enum value.

Created attachment 443740 [details] Patch

Committed r285543 (244057@main): <https://commits.webkit.org/244057@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 443740 [details].