[webkit-reviews] review granted: [Bug 233973] [GPU Process] [Filters] Make Filter::apply() and FilterEffect:apply() takes FilterImageVector for the inputs : [Attachment 446302] Patch

[bugzilla-daemon at webkit.org]

Cameron McCormack (:heycam) <heycam at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 233973: [GPU Process] [Filters] Make Filter::apply() and
FilterEffect:apply() takes FilterImageVector for the inputs
https://bugs.webkit.org/show_bug.cgi?id=233973

Attachment 446302: Patch

https://bugs.webkit.org/attachment.cgi?id=446302&action=review




--- Comment #2 from Cameron McCormack (:heycam) <heycam at apple.com> ---
Comment on attachment 446302
  --> https://bugs.webkit.org/attachment.cgi?id=446302
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"