Bug 225088 - [GPU Process] [Filters 7/23] Refactor the FilterEffect result buffers into a new class named 'FilterImage'
Summary: [GPU Process] [Filters 7/23] Refactor the FilterEffect result buffers into a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks: 231253
  Show dependency treegraph
 
Reported: 2021-04-26 22:20 PDT by Said Abou-Hallawa
Modified: 2021-11-29 21:32 PST (History)
25 users (show)

See Also:


Attachments
Patch (52.60 KB, patch)
2021-04-26 22:22 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (52.60 KB, patch)
2021-04-26 22:33 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (52.59 KB, patch)
2021-04-26 23:24 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (58.62 KB, patch)
2021-04-27 13:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (59.35 KB, patch)
2021-04-27 14:00 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (60.45 KB, patch)
2021-04-27 19:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (60.45 KB, patch)
2021-04-27 23:56 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (305.13 KB, patch)
2021-11-01 18:12 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (305.04 KB, patch)
2021-11-02 01:31 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (304.97 KB, patch)
2021-11-02 02:00 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (304.42 KB, patch)
2021-11-02 02:16 PDT, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (305.04 KB, patch)
2021-11-02 02:41 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (78.14 KB, patch)
2021-11-02 02:53 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (77.07 KB, patch)
2021-11-11 11:07 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (85.72 KB, patch)
2021-11-11 13:05 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (85.80 KB, patch)
2021-11-11 13:19 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (84.89 KB, patch)
2021-11-14 22:38 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (91.90 KB, patch)
2021-11-19 17:03 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (90.95 KB, patch)
2021-11-20 03:10 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (91.10 KB, patch)
2021-11-20 03:42 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (91.01 KB, patch)
2021-11-20 11:29 PST, Said Abou-Hallawa
heycam: review+
Details | Formatted Diff | Diff
Patch (90.92 KB, patch)
2021-11-23 00:33 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2021-04-26 22:20:46 PDT
This moves all the conversion from ImageBuffer to the unpremultiplied/premultiplied ImageData and vice versa from FilterEffect to FilterImage. This will help with the following:

1. Simplifying the FilterEffect geometry calculation and give the ability to save memory
2. Integrate CoreImage in FilterImage and give the ability to deal with non CoreImage filters seamlessly through FilterImage.
3. Prepare for applying the FilterEffects in GPUP.
Comment 1 Said Abou-Hallawa 2021-04-26 22:22:59 PDT
Created attachment 427120 [details]
Patch
Comment 2 Said Abou-Hallawa 2021-04-26 22:33:30 PDT
Created attachment 427121 [details]
Patch
Comment 3 Said Abou-Hallawa 2021-04-26 23:24:30 PDT
Created attachment 427123 [details]
Patch
Comment 4 Said Abou-Hallawa 2021-04-27 13:31:23 PDT
Created attachment 427188 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-04-27 14:00:51 PDT
Created attachment 427193 [details]
Patch
Comment 6 Said Abou-Hallawa 2021-04-27 19:56:36 PDT
Created attachment 427229 [details]
Patch
Comment 7 Said Abou-Hallawa 2021-04-27 23:56:06 PDT
Created attachment 427241 [details]
Patch
Comment 8 Sam Weinig 2021-04-28 13:34:28 PDT
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?
Comment 9 Said Abou-Hallawa 2021-04-29 10:53:00 PDT
(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.
Comment 10 Radar WebKit Bug Importer 2021-05-03 22:21:13 PDT
<rdar://problem/77487760>
Comment 11 Sam Weinig 2021-05-06 18:00:05 PDT
(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().
Comment 12 Said Abou-Hallawa 2021-11-01 18:12:16 PDT
Created attachment 443047 [details]
Patch
Comment 13 Said Abou-Hallawa 2021-11-02 01:31:48 PDT
Created attachment 443067 [details]
Patch
Comment 14 Said Abou-Hallawa 2021-11-02 02:00:21 PDT
Created attachment 443072 [details]
Patch
Comment 15 Said Abou-Hallawa 2021-11-02 02:16:16 PDT
Created attachment 443073 [details]
Patch
Comment 16 Said Abou-Hallawa 2021-11-02 02:41:15 PDT
Created attachment 443074 [details]
Patch
Comment 17 Said Abou-Hallawa 2021-11-02 02:53:35 PDT
Created attachment 443075 [details]
Patch for review
Comment 18 Said Abou-Hallawa 2021-11-11 11:07:00 PST
Created attachment 443976 [details]
Patch
Comment 19 Said Abou-Hallawa 2021-11-11 13:05:43 PST
Created attachment 443993 [details]
Patch
Comment 20 Said Abou-Hallawa 2021-11-11 13:19:34 PST
Created attachment 443995 [details]
Patch
Comment 21 Said Abou-Hallawa 2021-11-14 22:38:19 PST
Created attachment 444211 [details]
Patch
Comment 22 Said Abou-Hallawa 2021-11-16 10:35:09 PST
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 23 Kimmo Kinnunen 2021-11-17 06:12:02 PST
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) ?
Comment 24 Said Abou-Hallawa 2021-11-19 17:03:59 PST
Created attachment 444871 [details]
Patch
Comment 25 Said Abou-Hallawa 2021-11-20 03:10:16 PST
Created attachment 444892 [details]
Patch
Comment 26 Said Abou-Hallawa 2021-11-20 03:42:25 PST
Created attachment 444893 [details]
Patch
Comment 27 Said Abou-Hallawa 2021-11-20 11:29:36 PST
Created attachment 444902 [details]
Patch
Comment 28 Said Abou-Hallawa 2021-11-20 20:20:38 PST
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 29 Cameron McCormack (:heycam) 2021-11-22 20:21:07 PST
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?
Comment 30 Said Abou-Hallawa 2021-11-23 00:33:21 PST
Created attachment 445012 [details]
Patch
Comment 31 Said Abou-Hallawa 2021-11-23 00:51:06 PST
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.
Comment 32 EWS 2021-11-23 01:22:24 PST
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].