Bug 232551

Created attachment 442947 [details] Patch

Created attachment 442955 [details] Patch

Created attachment 442957 [details] Patch for review

<rdar://problem/85136447>

Created attachment 443914 [details] Patch

Created attachment 443917 [details] Patch

Comment on attachment 443917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443917&action=review Refactoring looks OK. A couple of questions below. > Source/WebCore/ChangeLog:17 > + Instead of having a reference to the Filter in the FilterEffect, we are > + going to pass a reference to the Filter to the FilterEffect functions > + only when needed. FilterEffect may need access to the Filter only in two > + methods: determineAbsolutePaintRect() and platformApplySoftware(). > + > + We need to change all the FilterEffect create methods to not take a Filter > + as input. This requires the SVG filter effect elements to change their > + build() method. And it requires also CSSFilter to not pass itself to the > + FilterEffects its create. Please mention why we want to do this refactoring. (Mention something about how it's removing an unnecessary dependency of individual FilterEffects on the Filter object, which makes it easier to send the filter description over IPC.) > Source/WebCore/platform/graphics/filters/FETurbulence.h:96 > + const Filter* filter; > + FETurbulence* effect; I guess these can't be references because the object is created uninitialized by ParallelJobs? > Source/WebCore/platform/graphics/filters/FilterEffect.cpp:424 > + auto buffer = ImageBuffer::create(clampedSize, RenderingMode::Unaccelerated, 1, operatingColorSpace(), PixelFormat::BGRA8); I suppose this is an intermediate stage, and by the end of the patch queue the page "use accelerated buffers for filters" setting will be honored again? What will be the behavior for ports that don't use the GPU process? Will it be a regression that they will use an unaccelerated buffer for filters? > Source/WebCore/rendering/CSSFilter.cpp:240 > + RefPtr<FilterEffect> previousEffect = SourceGraphic::create(); auto > Source/WebCore/rendering/CSSFilter.cpp:415 > - effect->apply(); > + > + for (auto& function : m_functions) { Is this a behavior change, or does it not matter which order you call apply() on the filter functions? > Source/WebCore/rendering/CSSFilter.cpp:417 > + downcast<SVGFilter>(function.ptr())->setSourceImage({ sourceImage() }); Maybe downcasting on the reference is better: ``` downcast<SVGFilter>(function.get()).setSourceImage(...); ``` (and the following two downcasts) > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:239 > - lastEffect->apply(); > + filterData.filter->apply(); Same question here: what's the effect of this change? > Source/WebCore/svg/SVGFEImageElement.cpp:214 > + return FEImage::create(Ref<Image> { *m_cachedImage->imageForRenderer(renderer()) }, preserveAspectRatio()); These days you can leave the `<Image>` off and just write `Ref { *m_cachedImage ... }`.

Comment on attachment 443917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443917&action=review >> Source/WebCore/ChangeLog:17 >> + FilterEffects its create. > > Please mention why we want to do this refactoring. (Mention something about how it's removing an unnecessary dependency of individual FilterEffects on the Filter object, which makes it easier to send the filter description over IPC.) Done. >> Source/WebCore/platform/graphics/filters/FETurbulence.h:96 >> + FETurbulence* effect; > > I guess these can't be references because the object is created uninitialized by ParallelJobs? Yes this is correct. This code should be cleaned up when the software filter code is moved to separate files. >> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:424 >> + auto buffer = ImageBuffer::create(clampedSize, RenderingMode::Unaccelerated, 1, operatingColorSpace(), PixelFormat::BGRA8); > > I suppose this is an intermediate stage, and by the end of the patch queue the page "use accelerated buffers for filters" setting will be honored again? > > What will be the behavior for ports that don't use the GPU process? Will it be a regression that they will use an unaccelerated buffer for filters? Yes this is correct. I mentioned in the ChangeLog above that this is going to change in the next patch. But I will add a FIXME comment also saying this is intermediate stage and should be fixed soon. But this should not affect the software filter since we always create the result ImageBuffer with RenderingMode::Unaccelerated. The CoreImage filters have to create the intermediate ImageBuffers with RenderingMode::Accelerated. But currently CoreImage filters have different code paths from this one. >> Source/WebCore/rendering/CSSFilter.cpp:240 >> + RefPtr<FilterEffect> previousEffect = SourceGraphic::create(); > > auto SourceGraphic::create() returns a Ref<SourceGraphic>. Using auto will set the type of previousEffect to Ref<SourceGraphic>. This will make it difficult to assign it to any other FilterEffect. And more importantly to need to nullify it with the reference filter case. >> Source/WebCore/rendering/CSSFilter.cpp:415 >> + for (auto& function : m_functions) { > > Is this a behavior change, or does it not matter which order you call apply() on the filter functions? No this is not behavior change. Applying the lastEffect will keep applying its inputs recursively before applying itself till we reach to the SourceGraphic which is the first entry in m_functions. >> Source/WebCore/rendering/CSSFilter.cpp:417 >> + downcast<SVGFilter>(function.ptr())->setSourceImage({ sourceImage() }); > > Maybe downcasting on the reference is better: > > ``` > downcast<SVGFilter>(function.get()).setSourceImage(...); > ``` > > (and the following two downcasts) Done. >> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:239 >> + filterData.filter->apply(); > > Same question here: what's the effect of this change? No it is not a behavior change. This is what SVGFilter::apply will do: void SVGFilter::apply() { m_lastEffect->apply(*this); } The plan is to change this function by applying the first FilterEffect till we reach the lastEffect. The goal is we do not need to store FilterEffect geometry. We always apply the FilterEffect forward. We will do the calculation of the input FilterEffects first and send these calculation to FilterEffect::apply(). >> Source/WebCore/svg/SVGFEImageElement.cpp:214 >> + return FEImage::create(Ref<Image> { *m_cachedImage->imageForRenderer(renderer()) }, preserveAspectRatio()); > > These days you can leave the `<Image>` off and just write `Ref { *m_cachedImage ... }`. Done.

Created attachment 444172 [details] Patch

Committed r285796 (244240@main): <https://commits.webkit.org/244240@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444172 [details].