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 Rendering | Assignee: | 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
Said Abou-Hallawa
2021-11-24 21:50:23 PST
Created attachment 445118 [details]
Patch
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.) Created attachment 445128 [details]
Patch
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(). 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]. |