Bug 240777 – [GPU Process] [Filters] Don't use Uint8ClampedArray in software filter appliers

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

[Said Abou-Hallawa]

The plan is to allocate the FilterImage buffers differently for GPUProcess. So the first step is to hide the underlying memory of PixelBuffer from the software filter appliers.

[Said Abou-Hallawa]

Created attachment 459654 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459655 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459656 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459660 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459662 [details]
Patch

[Said Abou-Hallawa]

Created attachment 459685 [details]
Patch

[Sam Weinig]

(In reply to Said Abou-Hallawa from comment #0)
> The plan is to allocate the FilterImage buffers differently for GPUProcess.
> So the first step is to hide the underlying memory of PixelBuffer from the
> software filter appliers.

This doesn't really seem to be hiding the buffers. It is in fact seems to make getting raw access to the bytes even easier:

  uint8_t* bytes() const;
  size_t byteLength() const;


Can you expand a bit on what the intent of these changes are?

[Sam Weinig]

Comment on attachment 459685 [details]
Patch

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

> Source/WebCore/platform/graphics/PixelBuffer.h:67
> +    bool setRange(const void* data, size_t dataByteLength, size_t byteOffset);

Please don't introduce new functions that take raw void* pointers. If it must be a raw pointer, please use uint8_t* for data buffers. But also, please prefer Span<>.

> Source/WebCore/platform/graphics/filters/software/FEGaussianBlurSoftwareApplier.cpp:389
> +                    // FIXME: No need to create a PixelBuffer if it is RefCounted class.

Can you clarify what this FIXME means? If what is RefCounted?

[Said Abou-Hallawa]

(In reply to Sam Weinig from comment #7)
> (In reply to Said Abou-Hallawa from comment #0)
> > The plan is to allocate the FilterImage buffers differently for GPUProcess.
> > So the first step is to hide the underlying memory of PixelBuffer from the
> > software filter appliers.
> 
> This doesn't really seem to be hiding the buffers. It is in fact seems to
> make getting raw access to the bytes even easier:
> 
>   uint8_t* bytes() const;
>   size_t byteLength() const;
> 
> 
> Can you expand a bit on what the intent of these changes are?

We want to allocate shared memory for PixelBuffer when it is created for Software filter appliers in GPUP. The goal is to attribute them to WebProcess so we can avoid jetsam the GPUP when the filter is a little bit large or complex (has many effects).

The JS Uint8ClampedArray does not work well in this case. In fact I do not think there is any need to back the PixelBuffer with Uint8ClampedArray even if it is created in WebProcess.

The full change is in https://bugs.webkit.org/attachment.cgi?id=459639&action=review. 

I think the final change should be something like this:

class PixelBuffer : public RefCounted<PixelBuffer> {
{
public:
    virtual uint8_t* bytes() const = 0;
    virtual size_t byteLength() const = 0;
};

class JSPixelBuffer : public PixelBuffer {
{
public:
    virtual uint8_t* bytes() const override { return m_data->data(); }
    virtual size_t byteLength() const { return m_data->byteLength(); }
private:
    Ref<JSC::Uint8ClampedArray> m_data;
};

class ShareablePixelBuffer : public WebCore::PixelBuffer {
public:
    virtual uint8_t* bytes() const override { return static_cast<uint8_t*>(m_data->data()); }
    virtual size_t byteLength() const { return m_data->size(); }
private:
    Ref<SharedMemory> m_data;
};

[Said Abou-Hallawa]

Comment on attachment 459685 [details]
Patch

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

>> Source/WebCore/platform/graphics/filters/software/FEGaussianBlurSoftwareApplier.cpp:389
>> +                    // FIXME: No need to create a PixelBuffer if it is RefCounted class.
> 
> Can you clarify what this FIXME means? If what is RefCounted?

Although we start by a PixelBuffer in FEGaussianBlurSoftwareApplier::apply() and we fall back to use Uint8ClampedArray because it is RefCounted. The problem is when if (!job) is true we used to store ioBuffer and tmpPixelArray in FEGaussianBlurSoftwareApplier::ApplyParameters. When  if (!job) is false we used to create new Uint8ClampedArray. Because I need to change everything to be PixelBuffer. I changed FEGaussianBlurSoftwareApplier::ApplyParameters to look like this:

    struct ApplyParameters {
        std::optional<PixelBuffer> ioBuffer;
        std::optional<PixelBuffer> tempBuffer;
    }

So I needed to copy (but not deep copy) the PixelBuffers ioBuffer and tempBuffer. If we make PixelBuffer RefCounted, ApplyParameters will be changed to be like this:

    struct ApplyParameters {
        RefPtr<PixelBuffer> ioBuffer;
        RefPtr<PixelBuffer> tempBuffer;
    }

And the code below will be changed to be like this:

params.ioBuffer = &ioBuffer;
params.tempBuffer = &tempBuffer;

[Said Abou-Hallawa]

Pull request: https://github.com/WebKit/WebKit/pull/967

[Radar WebKit Bug Importer]

<rdar://problem/93916935>

[EWS]

Committed r294832 (250982@main): <https://commits.webkit.org/250982@main>

Reviewed commits have been landed. Closing PR #967 and removing active labels.