Bug 218529 – [GPU Process] Use the Ref counting of ImageBuffer to control its life cycle in Web Process and GPU Process

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

DrawImageBuffer will hold a Variant<RefPtr<WebCore::ImageBuffer>, RenderingResourceIdentifier>. The RefPtr<WebCore::ImageBuffer> will be set in the Web Process when drawing an ImageBuffer to another ImageBuffer. The RenderingResourceIdentifier of ImageBuffer will be encoded when sending the DisplayList to the GPU Process. In the GPU process, a DrawImageBuffer will be decoded with RenderingResourceIdentifier. The RemoteRenderingBackend will search for the ImageBuffer in its RemoteResourceCache given the RenderingResourceIdentifier and will draw it to the GraphicsContext. This will remove the need to lock and unlock the remote resource in RemoteResourceCacheProxy.

[Said Abou-Hallawa]

Created attachment 413092 [details]
Patch

[Said Abou-Hallawa]

Created attachment 413100 [details]
Patch

[Said Abou-Hallawa]

Created attachment 413108 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 413108 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:816
> +    if (m_impl && image.renderingResourceIdentifier()) {
> +        m_impl->drawImageBuffer(image, destination, source, options);

I think it's wrong to rely on the presence of a image.renderingResourceIdentifier to control behavior. What's the real question you're asking?

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191
> +    m_displayList.registerImageBuffer(makeRef(imageBuffer));

This seems like a use for willAppendItemOfType().

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:192
>      appendItemAndUpdateExtent(DrawImageBuffer::create(imageBuffer.renderingResourceIdentifier(), destRect, srcRect, options));

Should probably ASSERT(imageBuffer.renderingResourceIdentifier()) here.

[Said Abou-Hallawa]

Created attachment 413361 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413108 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:816
>> +        m_impl->drawImageBuffer(image, destination, source, options);
> 
> I think it's wrong to rely on the presence of a image.renderingResourceIdentifier to control behavior. What's the real question you're asking?

I changed the patch such that this question means what is written. The question here is "can we inline the DrawImageBuffer item?". The DisplayList will keep a HashSet which will be used to control the life cycle of the source ImageBuffers. And it can be used also to resolve the RenderingResourceIdentifier to an ImageBuffer if the display list is replayed back in WebP. if it is replayed In GPUP, we are going to pass the HashSet which is maintained by RemoteResourceCache.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191
>> +    m_displayList.registerImageBuffer(makeRef(imageBuffer));
> 
> This seems like a use for willAppendItemOfType().

I might be confused by your comment here. But I think willAppendItemOfType() work on the destination ImageBuffer but we want to cache the source ImageBuffer.

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:192
>>      appendItemAndUpdateExtent(DrawImageBuffer::create(imageBuffer.renderingResourceIdentifier(), destRect, srcRect, options));
> 
> Should probably ASSERT(imageBuffer.renderingResourceIdentifier()) here.

Done.

[Simon Fraser (smfr)]

Comment on attachment 413361 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.cpp:815
> +    if (m_impl && image.renderingResourceIdentifier()) {

image.renderingResourceIdentifier should not change behavior.

[Said Abou-Hallawa]

Created attachment 413391 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413361 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext.cpp:815
>> +    if (m_impl && image.renderingResourceIdentifier()) {
> 
> image.renderingResourceIdentifier should not change behavior.

Fixed. The new patch records an inline DrawImageBuffer item for both DL and GPUP cases.

[Simon Fraser (smfr)]

Comment on attachment 413391 [details]
Patch

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

> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191
> +    m_displayList.cacheImageBuffer(makeRef(imageBuffer));

Rather than this, you should the work in willAppendItemOfType(), because there are going to be other drawing commands that use ImageBuffers (e.g. clipToImageBuffer() which you did not fix in this patch).

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:54
> +    if (item.type() == ItemType::DrawImageBuffer) {
> +        auto& drawItem = static_cast<DrawImageBuffer&>(item);
> +        if (auto* imageBuffer = m_imageBuffers.get(drawItem.renderingResourceIdentifier()))
> +            drawItem.apply(m_context, *imageBuffer);
> +        return;
> +    }

Also needs to deal with clipToImageBuffer().

> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:57
> +    if (m_delegate && m_delegate->apply(item, m_context))
> +        return;

It's unclear how someone reading this code would know whether it's correct for this to be after the ImageBuffer special-casing.

[Said Abou-Hallawa]

Created attachment 413406 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 413391 [details]
Patch

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

>> Source/WebCore/platform/graphics/displaylists/DisplayListRecorder.cpp:191
>> +    m_displayList.cacheImageBuffer(makeRef(imageBuffer));
> 
> Rather than this, you should the work in willAppendItemOfType(), because there are going to be other drawing commands that use ImageBuffers (e.g. clipToImageBuffer() which you did not fix in this patch).

I will address fixing clipToImageBuffer in a separate patch.

>> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:57
>> +        return;
> 
> It's unclear how someone reading this code would know whether it's correct for this to be after the ImageBuffer special-casing.

I cleaned this function a little. I moved the call to delegate first, then I added DrawImageBuffer special-casing second.

[EWS]

Committed r269503: <https://trac.webkit.org/changeset/269503>

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

[Radar WebKit Bug Importer]

<rdar://problem/71109948>