Bug 233487

Summary: [GPU Process] [Filters 10/23] Make FilterEffect appliers take FilterImages in their apply() methods
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: Layout and RenderingAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, 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
heycam: review+
Patch none

Said Abou-Hallawa
Reported 2021-11-24 21:50:23 PST
Eventually the FilterImage will be removed from FilterEffect. So this is a step towards making the FilterEffect hold the effect data only. To do this change, FilterImage needs to hold two new members: primitiveSubregion and isAlphaImage.
Attachments
Patch (101.56 KB, patch)
2021-11-24 22:03 PST, Said Abou-Hallawa
heycam: review+
Patch (101.29 KB, patch)
2021-11-25 02:34 PST, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2021-11-24 22:03:23 PST
Cameron McCormack (:heycam)
Comment 2 2021-11-24 22:54:48 PST
Comment on attachment 445118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445118&action=review > Source/WebCore/ChangeLog:10 > + his change, FilterImage needs to hold two new members: primitiveSubregion "this change" > Source/WebCore/ChangeLog:13 > + FilterEffect::setIsAlphaImage() is not needed and isAlphaImage() will be "will be a" > Source/WebCore/platform/graphics/filters/FEBlend.cpp:56 > + FilterImageVector inputs { { *inputEffect(0)->filterImage() }, { *inputEffect(1)->filterImage() } }; Are the inner braces needed? The Ref constructor that takes a T& is not explicit, so I think they shouldn't be needed. (Throughout this patch.) Also: maybe this helps subsequent patches in your queue, but wouldn't it be easier to have a function that returns the FilterImageVector by iterating over m_inputEffects and calling `filterImage()` on them all (called `inputEffectImages()` or `inputImages()`)? Is there an advantage to grabbing out input effect #0 and input effect #1 here explicitly? > Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:96 > +bool FEColorMatrix::isAlphaImage() const Since we're separating the effects from the images, maybe this should be called "producesAlphaImage", something that sounds less like the effect object is an image itself. > Source/WebCore/platform/graphics/filters/FEMerge.cpp:46 > + FilterImageVector inputs; > + > + for (auto& inputEffect : inputEffects()) > + inputs.append({ *inputEffect->filterImage() }); (Something like this, as a member function on effect objects, is what I was thinking.)
Said Abou-Hallawa
Comment 3 2021-11-25 02:34:00 PST
Said Abou-Hallawa
Comment 4 2021-11-25 02:35:42 PST
Comment on attachment 445118 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445118&action=review >> Source/WebCore/platform/graphics/filters/FEBlend.cpp:56 >> + FilterImageVector inputs { { *inputEffect(0)->filterImage() }, { *inputEffect(1)->filterImage() } }; > > Are the inner braces needed? The Ref constructor that takes a T& is not explicit, so I think they shouldn't be needed. (Throughout this patch.) > > Also: maybe this helps subsequent patches in your queue, but wouldn't it be easier to have a function that returns the FilterImageVector by iterating over m_inputEffects and calling `filterImage()` on them all (called `inputEffectImages()` or `inputImages()`)? Is there an advantage to grabbing out input effect #0 and input effect #1 here explicitly? Done. >> Source/WebCore/platform/graphics/filters/FEColorMatrix.cpp:96 >> +bool FEColorMatrix::isAlphaImage() const > > Since we're separating the effects from the images, maybe this should be called "producesAlphaImage", something that sounds less like the effect object is an image itself. Done. But I named resultIsAlphaImage() to be consistent with resultColorSpace().
EWS
Comment 5 2021-11-25 03:44:51 PST
Committed r286164 (244547@main): <https://commits.webkit.org/244547@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445128 [details].
Radar WebKit Bug Importer
Comment 6 2021-11-25 03:45:40 PST
Note You need to log in before you can comment on or make changes to this bug.