Bug 232844 - [GPU Process] [Filters 21/23] Add the encoding/decoding for Filter and FilterEffect
Summary: [GPU Process] [Filters 21/23] Add the encoding/decoding for Filter and Filter...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (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-11-08 13:21 PST by Said Abou-Hallawa
Modified: 2021-12-05 21:50 PST (History)
20 users (show)

See Also:


Attachments
Patch (119.51 KB, patch)
2021-12-04 22:45 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (121.35 KB, patch)
2021-12-04 23:33 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (123.16 KB, patch)
2021-12-05 00:20 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (121.21 KB, patch)
2021-12-05 01:00 PST, Said Abou-Hallawa
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (121.26 KB, patch)
2021-12-05 01:18 PST, Said Abou-Hallawa
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch (121.19 KB, patch)
2021-12-05 20:27 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-11-08 13:21:21 PST
This will make it possible to transfer the Filter and FilterEffects data from WebProcess to GPUProcess.
Comment 1 Radar WebKit Bug Importer 2021-11-15 13:24:38 PST
<rdar://problem/85426351>
Comment 2 Said Abou-Hallawa 2021-12-04 22:45:59 PST
Created attachment 445978 [details]
Patch
Comment 3 Said Abou-Hallawa 2021-12-04 23:33:19 PST
Created attachment 445979 [details]
Patch
Comment 4 Said Abou-Hallawa 2021-12-05 00:20:42 PST
Created attachment 445981 [details]
Patch
Comment 5 Said Abou-Hallawa 2021-12-05 01:00:21 PST
Created attachment 445982 [details]
Patch
Comment 6 Said Abou-Hallawa 2021-12-05 01:18:38 PST
Created attachment 445983 [details]
Patch
Comment 7 Wenson Hsieh 2021-12-05 11:32:08 PST
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?
Comment 8 Said Abou-Hallawa 2021-12-05 20:14:00 PST
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.
Comment 9 Said Abou-Hallawa 2021-12-05 20:27:30 PST
Created attachment 445992 [details]
Patch
Comment 10 EWS 2021-12-05 21:49:59 PST
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].