Bug 233516

Summary: [GPU Process] [Filters 12/23] Make FilterEffects create FilterEffectAppliers which will be used by FilterEffect::apply()
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, dino, ews-watchlist, fmalita, gyuyoung.kim, heycam, kondapallykalyan, pdr, schenney, sergio, simon.fraser, 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

Said Abou-Hallawa
Reported 2021-11-26 02:58:59 PST
This is a step towards integrating CoreImage appliers in FilterEffect::apply().
Attachments
Patch (77.42 KB, patch)
2021-11-26 03:11 PST, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2021-11-26 03:11:44 PST
Created attachment 445185 [details] Patch
Cameron McCormack (:heycam)
Comment 2 2021-11-26 13:06:58 PST
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 3 2021-11-26 13:15:17 PST
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)
Comment 4 2021-11-26 13:24:27 PST
(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
Comment 5 2021-11-26 15:51:23 PST
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
Comment 6 2021-11-26 15:52:21 PST
<rdar://problem/85774904>
Note You need to log in before you can comment on or make changes to this bug.