Bug 207233 - Implement ImageBuffer shareable backends
Summary: Implement ImageBuffer shareable backends
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 207198
Blocks: 204955 207221
  Show dependency treegraph
 
Reported: 2020-02-04 15:04 PST by Said Abou-Hallawa
Modified: 2020-02-27 17:36 PST (History)
31 users (show)

See Also:


Attachments
Patch (501.12 KB, patch)
2020-02-04 15:23 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (39.62 KB, patch)
2020-02-04 16:31 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (40.23 KB, patch)
2020-02-27 00:40 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (40.61 KB, patch)
2020-02-27 07:29 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (41.26 KB, patch)
2020-02-27 08:40 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (42.15 KB, patch)
2020-02-27 08:52 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (41.92 KB, patch)
2020-02-27 09:11 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (43.60 KB, patch)
2020-02-27 15:34 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 2020-02-04 15:04:34 PST
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.
Comment 1 Said Abou-Hallawa 2020-02-04 15:23:24 PST
Created attachment 389723 [details]
Patch
Comment 2 Said Abou-Hallawa 2020-02-04 16:31:32 PST
Created attachment 389737 [details]
Patch  for review
Comment 3 Said Abou-Hallawa 2020-02-13 08:15:58 PST
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)) {
Comment 4 Said Abou-Hallawa 2020-02-27 00:40:13 PST
Created attachment 391841 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-02-27 07:29:36 PST
Created attachment 391863 [details]
Patch
Comment 6 Said Abou-Hallawa 2020-02-27 08:36:36 PST
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.
Comment 7 Said Abou-Hallawa 2020-02-27 08:40:06 PST
Created attachment 391880 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-02-27 08:52:56 PST
Created attachment 391882 [details]
Patch
Comment 9 Said Abou-Hallawa 2020-02-27 09:11:59 PST
Created attachment 391883 [details]
Patch
Comment 10 Simon Fraser (smfr) 2020-02-27 13:27:07 PST
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<>
Comment 11 Said Abou-Hallawa 2020-02-27 15:20:41 PST
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.
Comment 12 Said Abou-Hallawa 2020-02-27 15:34:36 PST
Created attachment 391932 [details]
Patch
Comment 13 Eric Carlson 2020-02-27 16:14:15 PST
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.
Comment 14 WebKit Commit Bot 2020-02-27 17:35:31 PST
Comment on attachment 391932 [details]
Patch

Clearing flags on attachment: 391932

Committed r257606: <https://trac.webkit.org/changeset/257606>
Comment 15 WebKit Commit Bot 2020-02-27 17:35:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2020-02-27 17:36:17 PST
<rdar://problem/59871107>