Bug 232844 – [GPU Process] [Filters 21/23] Add the encoding/decoding for Filter and FilterEffect

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

This will make it possible to transfer the Filter and FilterEffects data from WebProcess to GPUProcess.

[Radar WebKit Bug Importer]

<rdar://problem/85426351>

[Said Abou-Hallawa]

Created attachment 445978 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445979 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445981 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445982 [details]
Patch

[Said Abou-Hallawa]

Created attachment 445983 [details]
Patch

[Wenson Hsieh]

Comment on attachment 445983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445983&action=review

> Source/WebCore/platform/graphics/filters/FEColorMatrix.h:99
> +template<> struct EnumTraits<WebCore::ColorMatrixType> {

Nit - we should consider making ColorMatrixType an `enum class : uint8_t`, perhaps in a separate patch.

> Source/WebCore/platform/graphics/filters/FEComponentTransfer.h:179
> +        WebCore::ComponentTransferType,

We should consider making ComponentTransferType an enum class as well (in a followup).

> Source/WebCore/platform/graphics/filters/FEComposite.h:137
> +        WebCore::CompositeOperationType,

(Ditto for CompositeOperationType)

> Source/WebCore/platform/graphics/filters/FEMerge.h:43
> +    unsigned m_numberOfEffectInputs;

Nit - let's make this initialize to `{ 0 };`

> Source/WebCore/platform/graphics/filters/Filter.h:79
> +    using FilterFunction::FilterFunction;

Is this necessary? I don't see any nested `FilterFunction` symbol defined in the `FilterFunction` class (or a separate namespace called `FilterFunction`)

> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:229
> +    auto sendResult = sendSync(Messages::RemoteRenderingBackend::GetFilteredImageForImageBuffer(imageBuffer, IPC::FilterReference { filter }), Messages::RemoteRenderingBackend::GetFilteredImageForImageBuffer::Reply(handle), renderingBackendIdentifier(), 1_s);

I'm a little surprised that this actually works, given that RRB only receives stream messages. Did you mean to use sendSyncToStream() instead?

[Said Abou-Hallawa]

Comment on attachment 445983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445983&action=review

>> Source/WebCore/platform/graphics/filters/FEMerge.h:43
>> +    unsigned m_numberOfEffectInputs;
> 
> Nit - let's make this initialize to `{ 0 };`

Fixed.

>> Source/WebCore/platform/graphics/filters/Filter.h:79
>> +    using FilterFunction::FilterFunction;
> 
> Is this necessary? I don't see any nested `FilterFunction` symbol defined in the `FilterFunction` class (or a separate namespace called `FilterFunction`)

This defines a new constructor for the Filter class which will be called from the new CSSFilter constructor:

CSSFilter::CSSFilter(Vector<Ref<FilterFunction>>&& functions)
    : Filter(Type::CSSFilter)
    , m_functions(WTFMove(functions))
{
}

Instead of writing

Filter::Filter(Type filterType)
    : FilterFunction(filterType)
{
}

we can just write 

using FilterFunction::FilterFunction;

>> Source/WebKit/WebProcess/GPU/graphics/RemoteRenderingBackendProxy.cpp:229
>> +    auto sendResult = sendSync(Messages::RemoteRenderingBackend::GetFilteredImageForImageBuffer(imageBuffer, IPC::FilterReference { filter }), Messages::RemoteRenderingBackend::GetFilteredImageForImageBuffer::Reply(handle), renderingBackendIdentifier(), 1_s);
> 
> I'm a little surprised that this actually works, given that RRB only receives stream messages. Did you mean to use sendSyncToStream() instead?

You are right. Fixed.

[Said Abou-Hallawa]

Created attachment 445992 [details]
Patch

[EWS]

Committed r286538 (244870@main): <https://commits.webkit.org/244870@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445992 [details].