Bug 233516 – [GPU Process] [Filters 12/23] Make FilterEffects create FilterEffectAppliers which will be used by FilterEffect::apply()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

This is a step towards integrating CoreImage appliers in FilterEffect::apply().

[Said Abou-Hallawa]

Created attachment 445185 [details]
Patch

[Cameron McCormack (:heycam)]

Comment on attachment 445185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445185&action=review

> Source/WebCore/platform/graphics/filters/FilterEffectApplier.h:37
> +    template<typename FilterEffectApplierType, typename FilterEffectType>
> +    static std::unique_ptr<FilterEffectApplierType> create(const FilterEffectType& effect)

Is there a need to allocate FilterEffectAppliers on the heap? Currently we don't let the applier object last longer than the function we create it in.

> Source/WebCore/platform/graphics/filters/FilterEffectApplier.h:42
>      FilterEffectApplier() = default;

If we do keep creating unique_ptrs for filterEffectAppliers, should this be private?

[Said Abou-Hallawa]

Comment on attachment 445185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445185&action=review

>> Source/WebCore/platform/graphics/filters/FilterEffectApplier.h:37
>> +    static std::unique_ptr<FilterEffectApplierType> create(const FilterEffectType& effect)
> 
> Is there a need to allocate FilterEffectAppliers on the heap? Currently we don't let the applier object last longer than the function we create it in.

Yes this is correct. But the apply() method is virtual and we need to call it from FilterEffect::apply()

    auto applier = createApplier(filter);
    applier->apply(filter, inputFilterImages(), *m_filterImage);

I think this can't be achieved unless the applier is a pointer to the base class.

>> Source/WebCore/platform/graphics/filters/FilterEffectApplier.h:42
>>      FilterEffectApplier() = default;
> 
> If we do keep creating unique_ptrs for filterEffectAppliers, should this be private?

makeUnique() requires the constructor to be public. But std::make_unique() does not. But you are right FilterEffectApplier::FilterEffectApplier() should be protected at least.

[Cameron McCormack (:heycam)]

(In reply to Said Abou-Hallawa from comment #3)
> Yes this is correct. But the apply() method is virtual and we need to call
> it from FilterEffect::apply()
> 
>     auto applier = createApplier(filter);
>     applier->apply(filter, inputFilterImages(), *m_filterImage);
> 
> I think this can't be achieved unless the applier is a pointer to the base
> class.

Understood.

[EWS]

Committed r286186 (244568@main): <https://commits.webkit.org/244568@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445185 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85774904>