[webkit-reviews] review granted: [Bug 232413] [GPU Process] [Filters 2/17] Introduce FilterFunction and make it the base class of Filter and FilterEffect : [Attachment 443665] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 232413: [GPU Process] [Filters 2/17] Introduce FilterFunction and make it
the base class of Filter and FilterEffect
https://bugs.webkit.org/show_bug.cgi?id=232413

Attachment 443665: Patch

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




--- Comment #12 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 443665
  --> https://bugs.webkit.org/attachment.cgi?id=443665
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.ht
ml

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