Bug 226425 – Adopt BifurcatedGraphicsContext for backing store + display list RemoteLayerBackingStore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Adopt BifurcatedGraphicsContext for backing store + display list RemoteLayerBackingStore

[Tim Horton]

Created attachment 430113 [details]
Patch

[Tim Horton]

Comment on attachment 430113 [details]
Patch

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

Needs a bunch more ifdefs, too.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:60
> +    void ensureBackingStore(Type, WebCore::FloatSize, float scale, bool deepColor, bool isOpaque, bool includeDisplayList);

Should not be just a bool

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:298
> +    m_frontBuffer.imageBuffer->flushDrawingContextAsync();
> +
> +    m_frontBufferFlusher = m_frontBuffer.imageBuffer->createFlusher();

Need to make sure that it's OK to not flush the DL ImageBuffer.

[Tim Horton]

Created attachment 430413 [details]
Patch

[Tim Horton]

Created attachment 430424 [details]
Patch

[Tim Horton]

Created attachment 430528 [details]
Patch

[Sam Weinig]

Comment on attachment 430528 [details]
Patch

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

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:60
> +    enum class IncludeDisplayList : bool { No, Yes };

This feels a bit confusing, since this includes the CGDisplayList not the WebCore DisplayList. 

I also think it would be useful to keep around the "only send the CGDisplayList" version for a bit if we can. I wonder if we can generalize this a bit to keep that around without adding too much overhead.

[Sam Weinig]

Let's chat about this a bit.

[Tim Horton]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 430528 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430528&action=review
> 
> > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:60
> > +    enum class IncludeDisplayList : bool { No, Yes };
> 
> This feels a bit confusing, since this includes the CGDisplayList not the
> WebCore DisplayList. 

... true.

> I also think it would be useful to keep around the "only send the
> CGDisplayList" version for a bit if we can. I wonder if we can generalize
> this a bit to keep that around without adding too much overhead.

We could, I don't even think it would be particularly high overhead. I will peek.

[Sam Weinig]

Comment on attachment 430528 [details]
Patch

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

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:123
>          RefPtr<WebCore::ImageBuffer> imageBuffer;
> +#if ENABLE(CG_DISPLAY_LIST_BACKED_IMAGE_BUFFER)
> +        RefPtr<WebCore::ImageBuffer> displayListImageBuffer;
> +#endif

Ultimately, I think it would be beneficial if we could avoid this layer being aware of the two image buffers at all, but just having a single bifurcated ImageBuffer class and letting the interface take care of things. That way, if we need a different structure in the future this won't have to change (and all the other benefits that come with good abstractions).

[Radar WebKit Bug Importer]

<rdar://problem/78911168>

[Tim Horton]

Created attachment 433667 [details]
Patch

[EWS]

Committed r279992 (239735@main): <https://commits.webkit.org/239735@main>

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