Created attachment 427120 [details] Patch

Created attachment 427121 [details] Patch

Created attachment 427123 [details] Patch

Created attachment 427188 [details] Patch

Created attachment 427193 [details] Patch

Created attachment 427229 [details] Patch

Created attachment 427241 [details] Patch

I can't quite tell what a FilterResult is supposed to represent? From the name, it sounds like it is the result of a filter effect graph. Is that correct? Is FilterEffect the only class that creates these?

(In reply to Sam Weinig from comment #8) > I can't quite tell what a FilterResult is supposed to represent? From the > name, it sounds like it is the result of a filter effect graph. Is that > correct? Yes it is the result of applying a FilterEffect to its input effects. It caches different formats for the result: image buffer, premultiplied image data and unpremultiplied image data. It facilitates the conversion from a format to another. > Is FilterEffect the only class that creates these? Yes currently FilterEffect is the only class that creates it. But my plan is to make FilterEffectRendererCoreImage use it also by including a fourth format of type CIImage. FilterEffect calculates the geometry and handles the result. This patch is trying to simplify this class by moving the result into an new class. So it will be responsible for calculating the geometry only.

<rdar://problem/77487760>

(In reply to Said Abou-Hallawa from comment #9) > (In reply to Sam Weinig from comment #8) > > Is FilterEffect the only class that creates these? > > Yes currently FilterEffect is the only class that creates it. Ok. I think we should then make it only constructible by FilterEffect by making the create functions private and friending them to FilterEffect. It might also be reasonable to make this a nested class inside FilterEffect, something like FilterEffect::Result, but that is more a style opinion than anything too critical. It seems a bit odd that a result type has functions that can mutate it like correctPremultipliedImageData() and transformToColorSpace().

Created attachment 443047 [details] Patch

Created attachment 443067 [details] Patch

Created attachment 443072 [details] Patch

Created attachment 443073 [details] Patch

Created attachment 443074 [details] Patch

Created attachment 443075 [details] Patch for review

Created attachment 443976 [details] Patch

Created attachment 443993 [details] Patch

Created attachment 443995 [details] Patch

Created attachment 444211 [details] Patch

Please notice this renaming in the latest patch Old New --------------------------------------------------------------------------------------------- FilterEffect::createImageBufferResult() -> FilterEffect::imageBufferResult() FilterEffect::createPremultipliedImageResult -> FilterEffect::premultipliedResult() FilterEffect::premultipliedResult() -> FilterEffect::getPremultipliedResult() FilterEffect::copyPremultipliedResult() -> FilterEffect::copyPremultipliedResult() // No change FilterEffect::createUnmultipliedImageResult -> FilterEffect::unpremultipliedResult() FilterEffect::unmultipliedResult() -> FilterEffect::getUnpremultipliedResult() FilterEffect::copyUnmultipliedResult() -> FilterEffect::copyUnpremultipliedResult()

Comment on attachment 444211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444211&action=review > Source/WebCore/platform/graphics/filters/FilterEffect.h:50 > + PixelBuffer* unpremultipliedResult(); It's not entirely clear to me why in every other place we pass AlphaPremultiplication, but then in this file, we have specific method per alpha premultiplication strategy. So to untrained eye it'd be more consistent and less entry points to have : PixelBuffer* FilterEffect::pixelBufferResult(AlphaPremultiplication) Similar observation for the other functions below > Source/WebCore/platform/graphics/filters/FilterImage.cpp:77 > +#if USE(ACCELERATE) I think these functions might already be available in PixelBufferConversion.h > Source/WebCore/platform/graphics/filters/FilterImage.cpp:113 > + size_t rowBytes = inputSize.width() * 4; Maybe in PixelBufferConversion.h > Source/WebCore/platform/graphics/filters/FilterImage.cpp:249 > + yEnd = logicalSize.height(); Could the above complexity be handled with abstraction of: - copiedRect = logicalRect intersect rect - if copiedRect is empty, return - copy copiedRect > Source/WebCore/platform/graphics/filters/FilterImage.cpp:265 > +std::optional<PixelBuffer> FilterImage::getConvertedPixelBuffer(ImageBuffer& imageBuffer, AlphaPremultiplication alphaFormat, const IntRect& rect, DestinationColorSpace colorSpace) const this could be a .cpp local static function > Source/WebCore/platform/graphics/filters/FilterImage.cpp:268 > + // Create an ImageBuffer with the correct color space and utilize CG to handle color space conversion This comment is probably a bit redunant.. > Source/WebCore/platform/graphics/filters/FilterImage.cpp:269 > + auto convertedImageBuffer = ImageBuffer::create(clampedSize, RenderingMode::Unaccelerated, 1, colorSpace, PixelFormat::BGRA8); I think the drawImageBuffer should still be accelerated, so here you probably could use "createCompatibleBuffer" ? > Source/WebCore/platform/graphics/filters/FilterImage.cpp:283 > + // Create an ImageBuffer to store incoming PixelBuffer This comment is probably a bit redunant.. > Source/WebCore/platform/graphics/filters/FilterImage.cpp:284 > + auto imageBuffer = ImageBuffer::create(clampedSize, RenderingMode::Unaccelerated, 1, m_colorSpace, PixelFormat::BGRA8); here use of m_colorspace is a bit strange, it's unclear if it's the same as the incoming pixel buffer color space. Should the color space come from the incoming PixelBufferConversion? If so, this function can be .cpp local static function. > Source/WebCore/platform/graphics/filters/FilterImage.h:48 > + RefPtr<ImageBuffer>& imageBufferIfExists() { return m_imageBuffer; } This is not used as a slot, so this could be just RefPtr<ImageBuffer>, no reference. (Giving out references to member variables is same as making the members public.) This function could be private? > Source/WebCore/platform/graphics/filters/FilterImage.h:51 > + std::optional<PixelBuffer>& pixelBufferIfExists(AlphaPremultiplication); This function gives out member variable as a public. This should be a private function. The use of this function is primarily not to get the pixel buffer if it exists. It is to get the slot to which to write the pixel buffer being copied. So maybe it would be more obvious if it was renamed to something else. pixelBufferSlot(AlphaPremultiplication) ?

Created attachment 444871 [details] Patch

Created attachment 444892 [details] Patch

Created attachment 444893 [details] Patch

Created attachment 444902 [details] Patch

Comment on attachment 444211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444211&action=review >> Source/WebCore/platform/graphics/filters/FilterEffect.h:50 >> + PixelBuffer* unpremultipliedResult(); > > It's not entirely clear to me why in every other place we pass AlphaPremultiplication, but then in this file, we have specific method per alpha premultiplication strategy. > > So to untrained eye it'd be more consistent and less entry points to have : > PixelBuffer* FilterEffect::pixelBufferResult(AlphaPremultiplication) > > Similar observation for the other functions below Fixed. Three functions were added for PixelBuffer queries: PixelBuffer* pixelBufferResult(AlphaPremultiplication); std::optional<PixelBuffer> getPixelBufferResult(AlphaPremultiplication, const IntRect& sourceRect, std::optional<DestinationColorSpace> = std::nullopt); void copyPixelBufferResult(PixelBuffer& destinationPixelBuffer, const IntRect& sourceRect) const; >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:77 >> +#if USE(ACCELERATE) > > I think these functions might already be available in PixelBufferConversion.h Yes they are. Fixed. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:113 >> + size_t rowBytes = inputSize.width() * 4; > > Maybe in PixelBufferConversion.h Yes it is. Fixed. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:249 >> + yEnd = logicalSize.height(); > > Could the above complexity be handled with abstraction of: > - copiedRect = logicalRect intersect rect > - if copiedRect is empty, return > - copy copiedRect Yes the clipped source rectangle can be calculated this way. But we need we need also to calculate the destination rectangle. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:265 >> +std::optional<PixelBuffer> FilterImage::getConvertedPixelBuffer(ImageBuffer& imageBuffer, AlphaPremultiplication alphaFormat, const IntRect& rect, DestinationColorSpace colorSpace) const > > this could be a .cpp local static function Yes it can be static. Fixed. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:268 >> + // Create an ImageBuffer with the correct color space and utilize CG to handle color space conversion > > This comment is probably a bit redunant.. The comment was removed. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:269 >> + auto convertedImageBuffer = ImageBuffer::create(clampedSize, RenderingMode::Unaccelerated, 1, colorSpace, PixelFormat::BGRA8); > > I think the drawImageBuffer should still be accelerated, so here you probably could use "createCompatibleBuffer" ? We have never used accelerated ImageBuffers for software filters. CoreImages have to use IOSurfaces. So FilterImage will be extended in future patches to handle the CoreImage case as well. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:283 >> + // Create an ImageBuffer to store incoming PixelBuffer > > This comment is probably a bit redunant.. This comment was removed. >> Source/WebCore/platform/graphics/filters/FilterImage.cpp:284 >> + auto imageBuffer = ImageBuffer::create(clampedSize, RenderingMode::Unaccelerated, 1, m_colorSpace, PixelFormat::BGRA8); > > here use of m_colorspace is a bit strange, it's unclear if it's the same as the incoming pixel buffer color space. > Should the color space come from the incoming PixelBufferConversion? > If so, this function can be .cpp local static function. The function was made static and the colorSpace of the sourcePixelBuffer is now used to create the ImageBuffer. >> Source/WebCore/platform/graphics/filters/FilterImage.h:48 >> + RefPtr<ImageBuffer>& imageBufferIfExists() { return m_imageBuffer; } > > This is not used as a slot, so this could be just RefPtr<ImageBuffer>, no reference. > (Giving out references to member variables is same as making the members public.) > > This function could be private? Yes it can be private. Fixed. >> Source/WebCore/platform/graphics/filters/FilterImage.h:51 >> + std::optional<PixelBuffer>& pixelBufferIfExists(AlphaPremultiplication); > > This function gives out member variable as a public. > This should be a private function. > > The use of this function is primarily not to get the pixel buffer if it exists. It is to get the slot to which to write the pixel buffer being copied. So maybe it would be more obvious if it was renamed to something else. > pixelBufferSlot(AlphaPremultiplication) ? The function is renamed to pixelBufferSlot() and it was made private.

Comment on attachment 444902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444902&action=review > Source/WebCore/ChangeLog:17 > + from FilterImage will make this creation happens. "make this creation happen" > Source/WebCore/platform/graphics/filters/FilterImage.h:55 > + void transformToColorSpace(DestinationColorSpace); The patch has a few functions that take a DestinationColorSpace and some take a const reference and other pass by value. We should be consistent. > Source/WebCore/platform/graphics/filters/FilterImage.h:60 > + RefPtr<ImageBuffer>& imageBufferSlot() { return m_imageBuffer; } Is it necessary to have a function for this, rather than having the callers just refer to m_imageBuffer?

Created attachment 445012 [details] Patch

Comment on attachment 444902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444902&action=review >> Source/WebCore/platform/graphics/filters/FilterImage.h:55 >> + void transformToColorSpace(DestinationColorSpace); > > The patch has a few functions that take a DestinationColorSpace and some take a const reference and other pass by value. We should be consistent. This function will take a const reference to DestinationColorSpace. >> Source/WebCore/platform/graphics/filters/FilterImage.h:60 >> + RefPtr<ImageBuffer>& imageBufferSlot() { return m_imageBuffer; } > > Is it necessary to have a function for this, rather than having the callers just refer to m_imageBuffer? This function is deleted.

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