Created attachment 446302 [details] Patch

Comment on attachment 446302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446302&action=review > Source/WebCore/ChangeLog:3 > + [GPU Process] [Filters] Make Filter::apply() and FilterEffect:apply() takes FilterImageVector for the inputs "take" > Source/WebCore/ChangeLog:9 > + step is required to make encoding/decoding the FilterEffect is just s/is just/be just/ (or "just be") > Source/WebCore/ChangeLog:26 > + FilterEffect. Every FilterEffect is asked to takeInputs() form this "from this" > Source/WebCore/ChangeLog:40 > + premultiplied pixels of the input FilterImageVector. We do not need > + to do this correction if the FilterEffect we apply is arithmetic > + composite filter. Otherwise we need to correct the FilterImage of any > + arithmetic composite filter in the FilterImageVector. This sounds contradictory. > Source/WebCore/platform/graphics/filters/FEComposite.h:70 > + bool resultIsValidPremultiplied() const override { return m_type != FECOMPOSITE_OPERATOR_ARITHMETIC; } (I feel like this has flipped back and forth over the course of the patch queue!) > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:94 > + return apply(filter, FilterImageVector { Ref { const_cast<FilterImage&>(input) } }); Is the const_cast needed? > Source/WebCore/platform/graphics/filters/FilterEffect.h:51 > + virtual FilterImageVector takeInputs(FilterImageVector&) const { return { }; } Idea: another way would be to have a virtual function that returns the number of inputs a given FilterEffect takes, and have takeInputs be a non-virtual function that pops that many items off the stack and returns them. > Source/WebCore/rendering/CSSFilter.cpp:374 > + if (m_functions.isEmpty() || !sourceImage) > + return nullptr; Is m_functions being empty a condition that content can create? Or is this defending against an error case. Just wondering if it needs to be checked here or if we'd be happy with returning sourceImage unchanged in that case. > Source/WebCore/svg/graphics/filters/SVGFilter.cpp:135 > + // Need to remove the inputs here in case the effect has a result. "in case the effect already has a result"

Comment on attachment 446302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446302&action=review >> Source/WebCore/ChangeLog:3 >> + [GPU Process] [Filters] Make Filter::apply() and FilterEffect:apply() takes FilterImageVector for the inputs > > "take" Fixed. >> Source/WebCore/ChangeLog:9 >> + step is required to make encoding/decoding the FilterEffect is just > > s/is just/be just/ (or "just be") Fixed. >> Source/WebCore/ChangeLog:26 >> + FilterEffect. Every FilterEffect is asked to takeInputs() form this > > "from this" Fixed. >> Source/WebCore/ChangeLog:40 >> + arithmetic composite filter in the FilterImageVector. > > This sounds contradictory. I do no think this is contradictory but I may not be able to phrase it in a clear way. The arithmetic composite filter may produce invalid premultiplied PixelBuffer. This happens when the color component is larger than the alpha channel. So the arithmetic composite filter itself does not need its inputs to be corrected since the result may be invalid anyway. And this is why we check RefPtr<FilterImage> FilterEffect::apply(...) { ... if (isValidPremultiplied) correctPremultipliedInputs(inputs); ... } isValidPremultiplied is true for all FilterEffects except the arithmetic composite filter. And in correctPremultipliedInputs() we call FilterImage::correctPremultipliedPixelBuffer() which will only correct the premultiplied PixelBuffer if it was generated by the arithmetic composite filter. void FilterImage::correctPremultipliedPixelBuffer() { if (!m_premultipliedPixelBuffer || m_isValidPremultiplied) return; ... } >> Source/WebCore/platform/graphics/filters/FEComposite.h:70 >> + bool resultIsValidPremultiplied() const override { return m_type != FECOMPOSITE_OPERATOR_ARITHMETIC; } > > (I feel like this has flipped back and forth over the course of the patch queue!) Yes this is true. I changed it from requiresValidPreMultipliedPixels() to mayProduceInvalidPremultipliedPixels(). But when I looked at it recently it took me awhile to understand it what it means. I think resultIsValidPremultiplied() is clearer than the previous two because it is positive (valid) not negative (invalid). And it explicitly says this is related to the result FilterImage of the FilterEffect not the inputs. >> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:94 >> + return apply(filter, FilterImageVector { Ref { const_cast<FilterImage&>(input) } }); > > Is the const_cast needed? No it is not needed. It was left over when I was trying to pass "const FilterImage&". But I realized the input FilterImage may change while applying the FilterEffect. So it can't be const. Removed. >> Source/WebCore/platform/graphics/filters/FilterEffect.h:51 >> + virtual FilterImageVector takeInputs(FilterImageVector&) const { return { }; } > > Idea: another way would be to have a virtual function that returns the number of inputs a given FilterEffect takes, and have takeInputs be a non-virtual function that pops that many items off the stack and returns them. yes this is a better approach. Done. >> Source/WebCore/rendering/CSSFilter.cpp:374 >> + return nullptr; > > Is m_functions being empty a condition that content can create? Or is this defending against an error case. Just wondering if it needs to be checked here or if we'd be happy with returning sourceImage unchanged in that case. You are right. A CSSFilter can't be created with empty m_functions. So this condition is not needed. Removed. >> Source/WebCore/svg/graphics/filters/SVGFilter.cpp:135 >> + // Need to remove the inputs here in case the effect has a result. > > "in case the effect already has a result" Fixed.

<rdar://problem/86248999>

Created attachment 446488 [details] Patch

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