Bug 233973 - [GPU Process] [Filters] Make Filter::apply() and FilterEffect:apply() take FilterImageVector for the inputs
Summary: [GPU Process] [Filters] Make Filter::apply() and FilterEffect:apply() take Fi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 231253 233989 234027
  Show dependency treegraph
 
Reported: 2021-12-07 22:05 PST by Said Abou-Hallawa
Modified: 2021-12-12 12:09 PST (History)
17 users (show)

See Also:


Attachments
Patch (59.00 KB, patch)
2021-12-07 22:31 PST, Said Abou-Hallawa
heycam: review+
Details | Formatted Diff | Diff
Patch (44.98 KB, patch)
2021-12-08 19:37 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-12-07 22:05:24 PST
This is a step towards removing the dependency on the input effects. This step is required to make encoding/decoding the FilterEffect is just sending or receiving its primitive data.
Comment 1 Said Abou-Hallawa 2021-12-07 22:31:00 PST
Created attachment 446302 [details]
Patch
Comment 2 Cameron McCormack (:heycam) 2021-12-08 16:26:18 PST
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 3 Said Abou-Hallawa 2021-12-08 19:19:46 PST
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.
Comment 4 Radar WebKit Bug Importer 2021-12-08 19:26:14 PST
<rdar://problem/86248999>
Comment 5 Said Abou-Hallawa 2021-12-08 19:37:57 PST
Created attachment 446488 [details]
Patch
Comment 6 EWS 2021-12-08 22:40:38 PST
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].