Bug 207233 – Implement ImageBuffer shareable backends

• Component: Canvas
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

The remote ImageBuffer proxy will create a shareable backend in the GPUProcess. Then it will give access to the backend to the remote ImageBuffer in the WebProcess by sending an ImageBufferBackendHandle. Only one shareable accelerated backend is implemented so for which is the IOSurface shareable backend. Other ports will have to implement their own shareable accelerated backends.

[Said Abou-Hallawa]

Created attachment 389723 [details]
Patch

[Said Abou-Hallawa]

Created attachment 389737 [details]
Patch  for review

[Said Abou-Hallawa]

Comment on attachment 389737 [details]
Patch  for review

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

> Source/WebKit/WebProcess/GPU/RenderingBackend/ImageBufferShareableBitmapBackend.cpp:67
> +    if (WTF::holds_alternative<ShareableBitmap::Handle>(handle)) {

This should be
    if (!WTF::holds_alternative<ShareableBitmap::Handle>(handle)) {

[Said Abou-Hallawa]

Created attachment 391841 [details]
Patch

[Said Abou-Hallawa]

Created attachment 391863 [details]
Patch

[Said Abou-Hallawa]

The GKT port fails while compiling WebKit/WebProcess/WebPage/WebFrame.cpp because the following definition 

    Display* m_display { nullptr };

in the header file WebKit/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.h is ambiguous. 

The candidates are the global definition (not inside any namespace)

    typedef struct _XDisplay Display;

which is defined in WebKit/WebProcess/WebPage/gtk/AcceleratedSurfaceX11.h and the WebCore namespace 

    namespace WebCore::Display { }

which is defined in WebCore/layout/displaytree/DisplayLineBox.h

It looks like there is a global "using namespace WebCore" (not inside any namespace) in one of the WebKit sources or the headers which compile with WebKit/WebProcess/WebPage/WebFrame.cpp in a unified source.

[Said Abou-Hallawa]

Created attachment 391880 [details]
Patch

[Said Abou-Hallawa]

Created attachment 391882 [details]
Patch

[Said Abou-Hallawa]

Created attachment 391883 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 391883 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:62
> +    return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context)));

make_unique<>

Do you need to pass both bitmap and context in? The context can be created by the bitmap.

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80
> +    return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context)));

make_unique<>

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:64
> +    RefPtr<ShareableBitmap> m_bitmap;

Can this be a Ref<>...

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:65
> +    std::unique_ptr<WebCore::GraphicsContext> m_context;

And this a GraphicsContext&?

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:49
> +    RetainPtr<CGContextRef> cgContext = surface->ensurePlatformContext(nullptr);

ensurePlatformContext has a nullptr default argument.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:53
> +    CGContextClearRect(cgContext.get(), FloatRect(FloatPoint::zero(), backendSize));

Shame that we have eagerly clear here.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:55
> +    return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(surface)));

make_unique<>

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:69
> +    return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(logicalSize, internalSize, resolutionScale, colorSpace, WTFMove(surface)));

make_unique<>

[Said Abou-Hallawa]

Comment on attachment 391883 [details]
Patch

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

>> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:62
>> +    return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context)));
> 
> make_unique<>
> 
> Do you need to pass both bitmap and context in? The context can be created by the bitmap.

If createGraphicsContext() returns nullptr for any reason, the ImageBuffer will be in a weird state. It is allocated but it can't function probably. I wanted to have two states for the returned value: 

1. nullptr: Something went wrong when allocating the data members needed for the ImageBuffer.
2. valid pointer: Nothing went wrong when allocating all the data member needed for the ImageBuffer. So the ImageBuffer can function properly.

>> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80
>> +    return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context)));
> 
> make_unique<>

Fixed. But I have to make the constructor public.

>> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.h:65
>> +    std::unique_ptr<WebCore::GraphicsContext> m_context;
> 
> And this a GraphicsContext&?

No this can't be because it is created by the create() method by calling ShareableBitmap::createGraphicsContext(). The ownership of this pointer is transferred to this class in the constructor by WTFMove(context). The destructor of this class is responsible for releasing the std::unique_ptr.

>> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:49
>> +    RetainPtr<CGContextRef> cgContext = surface->ensurePlatformContext(nullptr);
> 
> ensurePlatformContext has a nullptr default argument.

default argument was removed.

>> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:53
>> +    CGContextClearRect(cgContext.get(), FloatRect(FloatPoint::zero(), backendSize));
> 
> Shame that we have eagerly clear here.

This what we used to do with IOSurface ImageBuffer before the refactoring.

>> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:55
>> +    return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(size, backendSize, resolutionScale, colorSpace, WTFMove(surface)));
> 
> make_unique<>

Fixed.

>> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableIOSurfaceBackend.cpp:69
>> +    return std::unique_ptr<ImageBufferShareableIOSurfaceBackend>(new ImageBufferShareableIOSurfaceBackend(logicalSize, internalSize, resolutionScale, colorSpace, WTFMove(surface)));
> 
> make_unique<>

Fixed.

[Said Abou-Hallawa]

Created attachment 391932 [details]
Patch

[Eric Carlson]

Comment on attachment 391883 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/graphics/ImageBufferShareableBitmapBackend.cpp:80
>>> +    return std::unique_ptr<ImageBufferShareableBitmapBackend>(new ImageBufferShareableBitmapBackend(logicalSize, backendSize, resolutionScale, colorSpace, WTFMove(bitmap), WTFMove(context)));
>> 
>> make_unique<>
> 
> Fixed. But I have to make the constructor public.

You could keep it private and use a friend declaration. See ContentFilter.h for an example.

[WebKit Commit Bot]

Comment on attachment 391932 [details]
Patch

Clearing flags on attachment: 391932

Committed r257606: <https://trac.webkit.org/changeset/257606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59871107>