Bug 232413

Summary: [GPU Process] [Filters 2/23] Introduce FilterFunction and make it the base class of Filter and FilterEffect
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, darin, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, hta, jer.noble, kondapallykalyan, mmaxfield, pdr, philipj, schenney, sergio, simon.fraser, tommyw, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 231253    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
mmaxfield: review+
Patch
none
Patch none

Description Said Abou-Hallawa 2021-10-27 17:30:35 PDT
CSSFilter is composed of a sequence of <filter-function>. It can be defined as:

filter: <filter-function> [<filter-function>]* | none

The <filter-function> can be 'blur', 'brightness', ... etc. Or it can be 'url("filters.svg#filter-id")'. See https://developer.mozilla.org/en-US/docs/Web/CSS/filter.

So it make sense to introduce the same kind of layering in the filters code.
Comment 1 Said Abou-Hallawa 2021-10-27 18:05:04 PDT
Created attachment 442660 [details]
Patch
Comment 2 Darin Adler 2021-10-27 18:17:49 PDT
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 3 Darin Adler 2021-10-27 18:18:07 PDT
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.
Comment 4 Radar WebKit Bug Importer 2021-11-03 17:31:20 PDT
<rdar://problem/84999239>
Comment 5 Said Abou-Hallawa 2021-11-08 20:51:33 PST
Created attachment 443653 [details]
Patch
Comment 6 Said Abou-Hallawa 2021-11-08 20:52:39 PST
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.
Comment 7 Said Abou-Hallawa 2021-11-09 02:21:46 PST
Created attachment 443665 [details]
Patch
Comment 8 Myles C. Maxfield 2021-11-09 13:54:18 PST
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 9 Darin Adler 2021-11-09 14:17:22 PST
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 10 Said Abou-Hallawa 2021-11-09 14:27:58 PST
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 11 Said Abou-Hallawa 2021-11-09 14:31:11 PST
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 12 Myles C. Maxfield 2021-11-09 14:53:39 PST
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.
Comment 13 Said Abou-Hallawa 2021-11-09 15:30:01 PST
Created attachment 443739 [details]
Patch
Comment 14 Said Abou-Hallawa 2021-11-09 15:35:18 PST
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.
Comment 15 Said Abou-Hallawa 2021-11-09 15:39:57 PST
Created attachment 443740 [details]
Patch
Comment 16 EWS 2021-11-09 16:41:01 PST
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].