Bug 225508 – Add an experimental alternative display-list-based RemoteLayerBackingStore implementation

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Add an experimental alternative display-list-based RemoteLayerBackingStore implementation

[Tim Horton]

Created attachment 427987 [details]
Patch

[Tim Horton]

Created attachment 427988 [details]
Patch

[Sam Weinig]

Comment on attachment 427988 [details]
Patch

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

Really excellent work.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:416
> +#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
> +        , [&] (IPC::SharedBufferCopy& buffer) {
> +            ASSERT(m_type == Type::CGDisplayList);
> +            ASSERT([layer isKindOfClass:[WKCompositingLayer class]]);
> +            if (![layer isKindOfClass:[WKCompositingLayer class]])
> +                return;
> +            [layer setValue:@1 forKeyPath:WKCGDisplayListEnabledKey];
> +            auto data = buffer.buffer()->createCFData();
> +            [(WKCompositingLayer *)layer _setWKContentsDisplayList:data.get()];
>          }

Perhaps in the future we can make this function take a WKCompositingLayer rather than a vanilla CALayer and just typedef WKCompositingLayer to CALayer on platforms where they can be the same thing?

> Source/WebKit/UIProcess/RemoteLayerTree/cocoa/RemoteLayerTreeLayers.h:26
> +#if PLATFORM(COCOA)

#pragma once?

> Source/WebKit/UIProcess/RemoteLayerTree/cocoa/RemoteLayerTreeLayers.h:34
> +@property (nonatomic, retain, setter=_setWKContentsDisplayList:)__attribute__((NSObject)) CFDataRef _wkContentsDisplayList;

I think you want a space before the __attribute__. Also, this can probably be direct.

> Source/WebKit/WebProcess/GPU/graphics/ImageBufferBackendHandle.h:-40
>  #if PLATFORM(COCOA) // FIXME: This is really about IOSurface.
> -    MachSendRight,
> +    MachSendRight
> +#endif
> +    , ShareableBitmap::Handle
> +#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
> +    , IPC::SharedBufferCopy
>  #endif
> -    ShareableBitmap::Handle

I think this could be a bit neater if we made ShareableBitmap::Handle first here.

using ImageBufferBackendHandle = Variant<
    ShareableBitmap::Handle
#if PLATFORM(COCOA) // really should be HAVE(IOSURFACE) or something like that
    , MachSendRight
#endif
#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
    , IPC::SharedBufferCopy
#endif
>;


Additionally, I think we should probably add struct wrappers for MachSendRight and IPC::SharedBufferCopy in the future to indicate their use a bit better.

struct IOSurfaceImageBufferBackendHandle { MachSendRight handle };
struct CGDisplayListImageBufferBackendHandle { IPC::SharedBufferCopy handle };

using ImageBufferBackendHandle = Variant<
    ShareableBitmap::Handle
#if PLATFORM(COCOA)
    , IOSurfaceImageBufferBackendHandle
#endif
#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
    , CGDisplayListImageBufferBackendHandle
#endif
>;

This would let the type system tell the story a bit nicer. Not needed in this pass though.

[Tim Horton]

Created attachment 428019 [details]
Patch

[EWS]

Committed r277210 (237482@main): <https://commits.webkit.org/237482@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/77678782>