Bug 202798 – Implement asynchronous OffscreenCanvas placeholder updates for TextureMapperGL-based compositor

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Chris Lord]

This bug tracks implementing the 'short-circuit' path for OffscreenCanvas rendering, as mentioned in the spec: https://html.spec.whatwg.org/multipage/canvas.html#offscreencontext2d-commit on platforms that use the GL texture-mapper backend.

[Chris Lord]

Created attachment 380646 [details]
Patch

[Chris Lord]

Created attachment 406454 [details]
Patch

[Darin Adler]

Comment on attachment 406454 [details]
Patch

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

The reason I haven’t reviewed this myself yet is that I want a reviewer who is confident about the threading aspects of this: Which objects have and need thread-safe reference counts, how we move things between threads, etc.

> Source/WebCore/html/OffscreenCanvas.cpp:50
> +#endif // USE(TEXTURE_MAPPER_GL)

Seems short enough that we don’t need the #endif comment.

> Source/WebCore/html/OffscreenCanvas.cpp:112
> +#if USE(TEXTURE_MAPPER_GL)
> +#if USE(NICOSIA)

See comment below about nested if statements.

> Source/WebCore/html/OffscreenCanvas.cpp:115
> +    , m_platformLayerProxy(adoptRef(new TextureMapperPlatformLayerProxy()))

I suggest a * before the "new".

> Source/WebCore/html/OffscreenCanvas.cpp:539
> +    return m_platformLayerProxy.copyRef();

I suggest writing .ptr() instead of .copyRef() here. We are going to get rid of copyRef.

> Source/WebCore/html/OffscreenCanvas.h:190
> +#if USE(TEXTURE_MAPPER_GL)
> +    void pushPendingCommitDataToCompositor() const;
> +    void swapBuffersIfNeeded() override;
> +#if !USE(NICOSIA)
> +    RefPtr<TextureMapperPlatformLayerProxy> proxy() const override;
> +#endif
> +#endif

Since #if doesn’t nest all that well, I’ve been pushing people towards boolean expressions and separate paragraphs.

#If USE(TEXTURE_MAPPER_GL)
...
#endif

#If USE(TEXTURE_MAPPER_GL) && !USE(NICOSIA)
...
#endif

Why does the proxy function return a RefPtr? Can it return null?

> Source/WebCore/html/OffscreenCanvas.h:210
> +#if USE(TEXTURE_MAPPER_GL)
> +#if USE(NICOSIA)
> +    Ref<Nicosia::ContentLayer> m_nicosiaLayer;
> +#else
> +    RefPtr<TextureMapperPlatformLayerProxy> m_platformLayerProxy;
> +#endif
> +#endif

#If USE(TEXTURE_MAPPER_GL) && USE(NICOSIA)
...
#endif

#If USE(TEXTURE_MAPPER_GL) && !USE(NICOSIA)
...
#endif

Seems that m_platformLayerProxy should be Ref, not RefPtr.

[Chris Lord]

(In reply to Darin Adler from comment #3)
> Comment on attachment 406454 [details]
> Patch
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:115
> > +    , m_platformLayerProxy(adoptRef(new TextureMapperPlatformLayerProxy()))
> 
> I suggest a * before the "new".
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:539
> > +    return m_platformLayerProxy.copyRef();
> 
> I suggest writing .ptr() instead of .copyRef() here. We are going to get rid
> of copyRef.

I'm loathe to change this as every other user of this in this way has this exact pattern (see platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp, platform/graphics/holepunch/MediaPlayerPrivateHolePunch.cpp and platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp). I assume eventually the non-nicosia path will disappear anyway?

> Why does the proxy function return a RefPtr? Can it return null?
> 
> > Source/WebCore/html/OffscreenCanvas.h:210
> > +#if USE(TEXTURE_MAPPER_GL)
> > +#if USE(NICOSIA)
> > +    Ref<Nicosia::ContentLayer> m_nicosiaLayer;
> > +#else
> > +    RefPtr<TextureMapperPlatformLayerProxy> m_platformLayerProxy;
> > +#endif
> > +#endif
> 
> #If USE(TEXTURE_MAPPER_GL) && USE(NICOSIA)
> ...
> #endif
> 
> #If USE(TEXTURE_MAPPER_GL) && !USE(NICOSIA)
> ...
> #endif
> 
> Seems that m_platformLayerProxy should be Ref, not RefPtr.

Same comment for this, with the same example. I'd rather not change it unless someone with better knowledge of why it is how it is in these other files can confirm that's an ok change to make.

[Chris Lord]

Created attachment 406704 [details]
Patch

[Kenneth Russell]

Comment on attachment 406704 [details]
Patch

Have you found any other domain experts to review this? If not, I can try to. A quick scan looks okay.

[Noam Rosenthal]

I have to say I wish this use of #ifdefs was done differently, like with something like abstract functions in TextureMapperPlatformLayer. The situation where the different implementations details of TextureMapper are scattered along common WebCore files is not ideal.

[Chris Lord]

(In reply to Kenneth Russell from comment #6)
> Comment on attachment 406704 [details]
> Patch
> 
> Have you found any other domain experts to review this? If not, I can try
> to. A quick scan looks okay.

I haven't, a review would be much appreciated!

[Chris Lord]

(In reply to Noam Rosenthal from comment #7)
> I have to say I wish this use of #ifdefs was done differently, like with
> something like abstract functions in TextureMapperPlatformLayer. The
> situation where the different implementations details of TextureMapper are
> scattered along common WebCore files is not ideal.

I'm also not a fan... I guess the non-nicosia path will eventually be deprecated(?), but I don't know what the plans are.

[Noam Rosenthal]

(In reply to Chris Lord from comment #9)
> (In reply to Noam Rosenthal from comment #7)
> > I have to say I wish this use of #ifdefs was done differently, like with
> > something like abstract functions in TextureMapperPlatformLayer. The
> > situation where the different implementations details of TextureMapper are
> > scattered along common WebCore files is not ideal.
> 
> I'm also not a fan... I guess the non-nicosia path will eventually be
> deprecated(?), but I don't know what the plans are.

Isn’t Igalia currently maintaining TextureMapper? 
As the original contributor I would be happy to help review/rethink the design and code structure, trying to encapsulate better.

but I feel that until then the details of this patch should be reviewed  by current TextureMapper maintainers.

[Chris Lord]

(In reply to Noam Rosenthal from comment #10)
> (In reply to Chris Lord from comment #9)
> > (In reply to Noam Rosenthal from comment #7)
> > > I have to say I wish this use of #ifdefs was done differently, like with
> > > something like abstract functions in TextureMapperPlatformLayer. The
> > > situation where the different implementations details of TextureMapper are
> > > scattered along common WebCore files is not ideal.
> > 
> > I'm also not a fan... I guess the non-nicosia path will eventually be
> > deprecated(?), but I don't know what the plans are.
> 
> Isn’t Igalia currently maintaining TextureMapper? 
> As the original contributor I would be happy to help review/rethink the
> design and code structure, trying to encapsulate better.
> 
> but I feel that until then the details of this patch should be reviewed  by
> current TextureMapper maintainers.

I spoke with Zan and his feedback was that this platform-specific behaviour needs to be abstracted in a separate class.

My current thoughts are that there would be an OffscreenCanvasBase, which would be instantiable and provide the behaviour that we currently have, and that there would be an OffscreenCanvasTextureMapperGL.

I would still appreciate any comment (either on the current patch, or on this plan), just to make sure we're all on the same page.

[Noam Rosenthal]

(In reply to Chris Lord from comment #11)
> (In reply to Noam Rosenthal from comment #10)
> > (In reply to Chris Lord from comment #9)
> > > (In reply to Noam Rosenthal from comment #7)
> > > > I have to say I wish this use of #ifdefs was done differently, like with
> > > > something like abstract functions in TextureMapperPlatformLayer. The
> > > > situation where the different implementations details of TextureMapper are
> > > > scattered along common WebCore files is not ideal.
> > > 
> > > I'm also not a fan... I guess the non-nicosia path will eventually be
> > > deprecated(?), but I don't know what the plans are.
> > 
> > Isn’t Igalia currently maintaining TextureMapper? 
> > As the original contributor I would be happy to help review/rethink the
> > design and code structure, trying to encapsulate better.
> > 
> > but I feel that until then the details of this patch should be reviewed  by
> > current TextureMapper maintainers.
> 
> I spoke with Zan and his feedback was that this platform-specific behaviour
> needs to be abstracted in a separate class.
> 
> My current thoughts are that there would be an OffscreenCanvasBase, which
> would be instantiable and provide the behaviour that we currently have, and
> that there would be an OffscreenCanvasTextureMapperGL.
I wouldn't abstract OffscreenCanvas. I would suggest to add more abstract stuff to TextureMapperPlatformLayer which is already abstract, and implement things differently for nicosia/non-nicosia. Once that's done, it would be easier to read what the patch actually does in terms of canvas rather than in terms of texmap internals.

[Chris Lord]

(In reply to Noam Rosenthal from comment #12)
> (In reply to Chris Lord from comment #11)
> > (In reply to Noam Rosenthal from comment #10)
> > > (In reply to Chris Lord from comment #9)
> > > > (In reply to Noam Rosenthal from comment #7)
> > > > > I have to say I wish this use of #ifdefs was done differently, like with
> > > > > something like abstract functions in TextureMapperPlatformLayer. The
> > > > > situation where the different implementations details of TextureMapper are
> > > > > scattered along common WebCore files is not ideal.
> > > > 
> > > > I'm also not a fan... I guess the non-nicosia path will eventually be
> > > > deprecated(?), but I don't know what the plans are.
> > > 
> > > Isn’t Igalia currently maintaining TextureMapper? 
> > > As the original contributor I would be happy to help review/rethink the
> > > design and code structure, trying to encapsulate better.
> > > 
> > > but I feel that until then the details of this patch should be reviewed  by
> > > current TextureMapper maintainers.
> > 
> > I spoke with Zan and his feedback was that this platform-specific behaviour
> > needs to be abstracted in a separate class.
> > 
> > My current thoughts are that there would be an OffscreenCanvasBase, which
> > would be instantiable and provide the behaviour that we currently have, and
> > that there would be an OffscreenCanvasTextureMapperGL.
> I wouldn't abstract OffscreenCanvas. I would suggest to add more abstract
> stuff to TextureMapperPlatformLayer which is already abstract, and implement
> things differently for nicosia/non-nicosia. Once that's done, it would be
> easier to read what the patch actually does in terms of canvas rather than
> in terms of texmap internals.

So I was actually just going to remove the non-Nicosia path, I do believe it'll eventually be deprecated anyway. What further abstraction do you imagine being on TextureMapperPlatformLayer beyond what's already there?

[Noam Rosenthal]

(In reply to Chris Lord from comment #13)
> (In reply to Noam Rosenthal from comment #12)
> > (In reply to Chris Lord from comment #11)
> > > (In reply to Noam Rosenthal from comment #10)
> > > > (In reply to Chris Lord from comment #9)
> > > > > (In reply to Noam Rosenthal from comment #7)
> > > > > > I have to say I wish this use of #ifdefs was done differently, like with
> > > > > > something like abstract functions in TextureMapperPlatformLayer. The
> > > > > > situation where the different implementations details of TextureMapper are
> > > > > > scattered along common WebCore files is not ideal.
> > > > > 
> > > > > I'm also not a fan... I guess the non-nicosia path will eventually be
> > > > > deprecated(?), but I don't know what the plans are.
> > > > 
> > > > Isn’t Igalia currently maintaining TextureMapper? 
> > > > As the original contributor I would be happy to help review/rethink the
> > > > design and code structure, trying to encapsulate better.
> > > > 
> > > > but I feel that until then the details of this patch should be reviewed  by
> > > > current TextureMapper maintainers.
> > > 
> > > I spoke with Zan and his feedback was that this platform-specific behaviour
> > > needs to be abstracted in a separate class.
> > > 
> > > My current thoughts are that there would be an OffscreenCanvasBase, which
> > > would be instantiable and provide the behaviour that we currently have, and
> > > that there would be an OffscreenCanvasTextureMapperGL.
> > I wouldn't abstract OffscreenCanvas. I would suggest to add more abstract
> > stuff to TextureMapperPlatformLayer which is already abstract, and implement
> > things differently for nicosia/non-nicosia. Once that's done, it would be
> > easier to read what the patch actually does in terms of canvas rather than
> > in terms of texmap internals.
> 
> So I was actually just going to remove the non-Nicosia path, I do believe
> it'll eventually be deprecated anyway. What further abstraction do you
> imagine being on TextureMapperPlatformLayer beyond what's already there?

I think that once that's gone I'd be able to help figure that out... right now the #ifdef inflation is making it hard to read

[Noam Rosenthal]

Comment on attachment 406704 [details]
Patch

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

> Source/WebCore/html/OffscreenCanvas.cpp:115
> +#if USE(TEXTURE_MAPPER_GL) && USE(NICOSIA)
> +    , m_nicosiaLayer(Nicosia::ContentLayer::create(Nicosia::ContentLayerTextureMapperImpl::createFactory(*this)))
> +#elif USE(TEXTURE_MAPPER_GL)
> +    , m_platformLayerProxy(adoptRef(new TextureMapperPlatformLayerProxy()))
> +#endif

I think the issue starts here. If you had one m_textureMapperPlatformLayer that abstracted away nicosia/non-nicosia it would be much more readable.

> Source/WebCore/html/OffscreenCanvas.cpp:389
> +void OffscreenCanvas::pushPendingCommitDataToCompositor() const

I think most of this belongs in platform/*** code

> Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:65
> +                canvasRenderer.contentChanged(CanvasChanged);

This doesn't seem right as a platform specific thing... How is this done in OSX/IOS?

[Chris Lord]

Created attachment 407389 [details]
Patch

[Chris Lord]

I've just updated the patch and it ought to be more readable at least.

(In reply to Noam Rosenthal from comment #15)
> Comment on attachment 406704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406704&action=review
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:115
> > +#if USE(TEXTURE_MAPPER_GL) && USE(NICOSIA)
> > +    , m_nicosiaLayer(Nicosia::ContentLayer::create(Nicosia::ContentLayerTextureMapperImpl::createFactory(*this)))
> > +#elif USE(TEXTURE_MAPPER_GL)
> > +    , m_platformLayerProxy(adoptRef(new TextureMapperPlatformLayerProxy()))
> > +#endif
> 
> I think the issue starts here. If you had one m_textureMapperPlatformLayer
> that abstracted away nicosia/non-nicosia it would be much more readable.

I've removed the non-nicosia path so there's just one set of #ifdefs now.

> > Source/WebCore/html/OffscreenCanvas.cpp:389
> > +void OffscreenCanvas::pushPendingCommitDataToCompositor() const
> 
> I think most of this belongs in platform/*** code

Sounds reasonable to me, how do you envisage this working in that case? I suppose TextureMapperPlatformLayer should have some utility functions to push a bitmap like this? Note that this patch doesn't implement a possible fast-path of just pushing a texture handle, I wonder how that would work too?

> > Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:65
> > +                canvasRenderer.contentChanged(CanvasChanged);
> 
> This doesn't seem right as a platform specific thing... How is this done in
> OSX/IOS?

This part isn't platform specific, but I don't know if it's the correct way to go about it or not. The platform layer is the OffscreenCanvas, which is created asynchronously to the Placeholder context.

[Chris Lord]

Noam and I have discussed this, and I'm going to take some time to more fully understand layer composition here. There's a possibility that a faster, simpler path exists, rather than creating a new platform layer and copying the data like this patch does (certainly a faster path exists, but I expected it to involve more work, rather than less, thus this initial patch).

An initial aim is that the context will own the platform layer (WebGL and accelerated 2d canvases already have a platform layer) and we can use that as the platform layer rather than the OffscreenCanvas sub-classing the nicosia layer.

Once I have a better picture of how it could/will work, I'll update this bug.

[Noam Rosenthal]

Comment on attachment 407389 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +

Is it really covered by existing tests? If it is, I would expect this patch to unskip them for WPE/GTK platforms.

> Source/WebCore/html/OffscreenCanvas.cpp:374
> +                m_placeholderCanvas->setImageBufferAndMarkDirty(WTFMove(buffer));

Doesn't this become a redundant operation when you have a platform layer?
Seems like in this case you would be copying this data twice

> Source/WebCore/html/OffscreenCanvas.cpp:385
> +void OffscreenCanvas::pushPendingCommitDataToCompositor() const

Isn't this actually the proxy implementation of pushBufferToPlaceholder()? Seems like all of this stuff should be in there instead of this new function

> Source/WebCore/html/OffscreenCanvas.cpp:387
> +    auto proxyOperation =

You have a proxyOperation inside a proxyOperation here. Is this how it's supposed to work? Seems complex

> Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:57
> +void PlaceholderRenderingContext::setPlatformLayer(PlatformLayer* platformLayer)
> +{
> +    if (m_platformLayer == platformLayer)
> +        return;
> +

Since this is only called twice (once with a platform layer, one with null), I would do it differently:

- PlaceholderRenderingContext::attachOffscreenPlatformLayer(PlatformLayer*) with proper asserts that it happens once
- PlaceholderRenderingContext::releaseOffscreenPlatformLayer()

> Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:65
> +                canvasRenderer.contentChanged(CanvasChanged);

Something about this doesn't seem correct... I would expect the compositor to understand this using something like setPlatformLayerNeedsDisplay,
 RenderHTMLCanvas shouldn't play a part in this.

[Noam Rosenthal]

(In reply to Noam Rosenthal from comment #19)
> Comment on attachment 407389 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407389&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +
> 
> Is it really covered by existing tests? If it is, I would expect this patch
> to unskip them for WPE/GTK platforms.

Oops, I've already posted this comment before :)

[Chris Lord]

(In reply to Noam Rosenthal from comment #19)
> Comment on attachment 407389 [details]
> > Source/WebCore/html/OffscreenCanvas.cpp:374
> > +                m_placeholderCanvas->setImageBufferAndMarkDirty(WTFMove(buffer));
> 
> Doesn't this become a redundant operation when you have a platform layer?
> Seems like in this case you would be copying this data twice
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:385
> > +void OffscreenCanvas::pushPendingCommitDataToCompositor() const
> 
> Isn't this actually the proxy implementation of pushBufferToPlaceholder()?
> Seems like all of this stuff should be in there instead of this new function
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:387
> > +    auto proxyOperation =
> 
> You have a proxyOperation inside a proxyOperation here. Is this how it's
> supposed to work? Seems complex

Yes, I think there's redundancy here and I don't think this needs to be a separate function - quite possibly this is a hangover from an earlier iteration of the patch...

> > Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:57
> > +void PlaceholderRenderingContext::setPlatformLayer(PlatformLayer* platformLayer)
> > +{
> > +    if (m_platformLayer == platformLayer)
> > +        return;
> > +
> 
> Since this is only called twice (once with a platform layer, one with null),
> I would do it differently:
> 
> - PlaceholderRenderingContext::attachOffscreenPlatformLayer(PlatformLayer*)
> with proper asserts that it happens once
> - PlaceholderRenderingContext::releaseOffscreenPlatformLayer()

I like this suggestion, especially with the asserts - really, attach should happen on context creation, not on OffscreenCanvas transfer. If it happened then, these asserts would never be hit (I still think they should be there to reinforce that, of course).

> > Source/WebCore/html/canvas/PlaceholderRenderingContext.cpp:65
> > +                canvasRenderer.contentChanged(CanvasChanged);
> 
> Something about this doesn't seem correct... I would expect the compositor
> to understand this using something like setPlatformLayerNeedsDisplay,
>  RenderHTMLCanvas shouldn't play a part in this.

I think this is ok, at least there's a similar pattern in HTMLCanvasElement::reset, for similar reasons (and the offscreen-controlled path is in HTMLCanvasElement::setImageBufferAndMarkDirty) - I expect this could be relocated to HTMLCanvasElement without much effort, but this seems a reasonable place to have it to me(?)


Zan has suggested an abstraction where we introduce a new interface, ImageBufferPipeline. It would have methods to retrieve a PlatformLayer and to create a 'Source' object (as well as a method to determine if it's supported). A PlaceholderRenderingContext would own one of these and the OffscreenCanvas, instead of setting the platform layer would ask it to create a source and adopt the reference. The source would include the method to take an ImageBuffer and do the necessary copying that this patch is currently doing in OffscreenCanvas.

It's a nice abstraction that simplifies some of the ownership issues this patch and also moves platform-specific code out of OffscreenCanvas (and into a platform-specific implementation of ImageBufferPipeline).

What this doesn't tackle, however, is the accelerated canvas/WebGL case, where we already have a platform layer that we could use. In this situation, the previous solution of attach/detach platformLayer makes more sense (and when there is a platform layer like that, we don't need to involve the copying code at all). I'm trying to reconcile these two approaches, the simplest is to do as Zan has sketched out, but also add an alternate path in PlaceholderRenderingContext that allows for bypassing with an existing platform layer... But that feels pretty inelegant.

An alternative is to still introduce this class, but not have it be created by the placeholder context, and have it used as a fallback when there's no platform layer on the rendering context. This would place its ownership in OffscreenCanvas, but allow us not to complicate PlaceholderRenderingContext and have a single creation/destruction path, which I like the idea of. I think this is the route I'm going to pursue, but if that sounds wrong, please do say!

[Zan Dobersek]

(In reply to Chris Lord from comment #21)
> An alternative is to still introduce this class, but not have it be created
> by the placeholder context, and have it used as a fallback when there's no
> platform layer on the rendering context. This would place its ownership in
> OffscreenCanvas, but allow us not to complicate PlaceholderRenderingContext
> and have a single creation/destruction path, which I like the idea of. I
> think this is the route I'm going to pursue, but if that sounds wrong,
> please do say!

PlaceholderRenderingContext was used for that because it enables the simplest possible way of feeding the platform layer object into the RenderLayerBacking for the purposes of including that platform layer in the GraphicsLayer tree. 

The platform abstraction I provided works best in the case of a self-contained ImageBuffer object, i.e. a non-accelerated ImageBuffer whose contents can be plastered into the texture and composited. Bypass itself would work ideally in that case.

Accelerated ImageBuffer (at least in the case of Cairo-based implementation) and WebGL content handling is more complicated here because in both cases the existing implementation is tied to the (very main-thread-bound) layer flush process calling their `swapBuffersIfNeeded()` implementations to actually flush the rendered content into a GL texture that's then rendered. In both cases a thread-local GL context is required for that (and for other usual operations) -- WebGL uses its own context, but ImageBuffer uses the global sharing context.

Tangential to all this, Cairo-based accelerated ImageBuffer implementation (i.e. ENABLE(ACCELERATED_2D_CANVAS)-guarded code, mostly) is probably ripe for nixing.

[Chris Lord]

(In reply to Zan Dobersek from comment #22)
> (In reply to Chris Lord from comment #21)
> > An alternative is to still introduce this class, but not have it be created
> > by the placeholder context, and have it used as a fallback when there's no
> > platform layer on the rendering context. This would place its ownership in
> > OffscreenCanvas, but allow us not to complicate PlaceholderRenderingContext
> > and have a single creation/destruction path, which I like the idea of. I
> > think this is the route I'm going to pursue, but if that sounds wrong,
> > please do say!
> 
> PlaceholderRenderingContext was used for that because it enables the
> simplest possible way of feeding the platform layer object into the
> RenderLayerBacking for the purposes of including that platform layer in the
> GraphicsLayer tree. 
> 
> The platform abstraction I provided works best in the case of a
> self-contained ImageBuffer object, i.e. a non-accelerated ImageBuffer whose
> contents can be plastered into the texture and composited. Bypass itself
> would work ideally in that case.
> 
> Accelerated ImageBuffer (at least in the case of Cairo-based implementation)
> and WebGL content handling is more complicated here because in both cases
> the existing implementation is tied to the (very main-thread-bound) layer
> flush process calling their `swapBuffersIfNeeded()` implementations to
> actually flush the rendered content into a GL texture that's then rendered.
> In both cases a thread-local GL context is required for that (and for other
> usual operations) -- WebGL uses its own context, but ImageBuffer uses the
> global sharing context.

I'm trying to understand this and have been reading through the code, but I'm still slightly confused here... Every PlatformLayer is different, and there are no particular restrictions that they're expected to abide by it seems, so using one directly is out of the question I suppose?

In this case, the solution you offer is really the only thing we can do - and we would have to either implement again for each type of platform layer we would want to provide, or offer an abstraction that makes it easier to adapt it to multiple different types of data?

I'm wondering if there's the possibility of an abstraction that would work for both WebGL and image buffers, or whether we really just need to handle each case separately - I'd certainly prefer the former...

Perhaps rather than calling it ImageBufferPipeline, we could call it something like ExternalPlatformLayer, and don't specify the 'handle' function, except for in its various sub-classes, where it could have multiple handle functions that marshal different types of data? (so textures, as well as image buffers, for example) Does that sound like something that would work?

> Tangential to all this, Cairo-based accelerated ImageBuffer implementation
> (i.e. ENABLE(ACCELERATED_2D_CANVAS)-guarded code, mostly) is probably ripe
> for nixing.

That'd be great - last time I tried this, it really didn't seem to work... (but it could well be my expectations were wrong or I'd misconfigured something)

[Chris Lord]

So I think I misunderstood ImageBuffer previously and didn't realise it can be hardware-backed - so Zan's suggested idea and interface makes perfect sense with no modifications. I'm implementing it at the moment.

[Chris Lord]

Created attachment 407955 [details]
Patch

[Chris Lord]

I've updated the patch following Zan's advice and it's a lot cleaner like this (and I think less error-prone). Areas of possible improvement for this patch:

- There's still no fast-path for WebGL. This would rely on ImageBuffer::copyToPlatformTexture, I think, which is currently unimplemented.

- There's more copying than I'd like. Currently, separate copies of the image buffer are sent to the placeholder and the compositor. If we could share this buffer, we could reduce the number of copies by one. After this patch, there's an extra surface copy in the case of async drawing being supported, and it'd be great to remove that, but lifecycle management in that case isn't simple/obvious.

Otherwise, I think this is good to go. It removes all platform-specific code, except in platform/graphics/nicosia and at least to my eyes, doesn't have any particularly questionable changes anymore.

[Chris Lord]

Looks like I got the XCode project file edits wrong, will fix and re-upload...

[Chris Lord]

Created attachment 407960 [details]
Patch

[Chris Lord]

Created attachment 407966 [details]
Patch

[Chris Lord]

Created attachment 407969 [details]
Patch

[Zan Dobersek]

Comment on attachment 407969 [details]
Patch

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

> Source/WebCore/platform/graphics/nicosia/NicosiaImageBufferPipe.h:62
> +class NicosiaImageBufferPipeSource final : public WebCore::ImageBufferPipe::Source, public ContentLayerTextureMapperImpl::Client {
> +public:
> +    NicosiaImageBufferPipeSource();
> +    virtual ~NicosiaImageBufferPipeSource();
> +
> +    void handle(std::unique_ptr<WebCore::ImageBuffer>&&) final;
> +
> +    PlatformLayer* platformLayer() const;
> +
> +private:
> +    void swapBuffersIfNeeded() override;
> +
> +    RefPtr<ContentLayer> m_nicosiaLayer;
> +
> +    mutable Lock m_imageBufferLock;
> +    std::unique_ptr<WebCore::ImageBuffer> m_imageBuffer;
> +};
> +
> +class NicosiaImageBufferPipe final : public WebCore::ImageBufferPipe {
> +public:
> +    NicosiaImageBufferPipe();
> +    virtual ~NicosiaImageBufferPipe() = default;
> +
> +private:
> +    RefPtr<WebCore::ImageBufferPipe::Source> source() const final;
> +    PlatformLayer* platformLayer() const final;
> +
> +    RefPtr<NicosiaImageBufferPipeSource> m_source;
> +};

These could all be local in the implementation file, which would make this header unnecessary.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:-68
> -    LockHolder locker(m_lock);

Why is any of changes in TextureMapperPlatformLayer needed?

[Chris Lord]

(In reply to Zan Dobersek from comment #31)
> Comment on attachment 407969 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407969&action=review
> 
> > Source/WebCore/platform/graphics/nicosia/NicosiaImageBufferPipe.h:62
> > +class NicosiaImageBufferPipeSource final : public WebCore::ImageBufferPipe::Source, public ContentLayerTextureMapperImpl::Client {
> > +public:
> > +    NicosiaImageBufferPipeSource();
> > +    virtual ~NicosiaImageBufferPipeSource();
> > +
> > +    void handle(std::unique_ptr<WebCore::ImageBuffer>&&) final;
> > +
> > +    PlatformLayer* platformLayer() const;
> > +
> > +private:
> > +    void swapBuffersIfNeeded() override;
> > +
> > +    RefPtr<ContentLayer> m_nicosiaLayer;
> > +
> > +    mutable Lock m_imageBufferLock;
> > +    std::unique_ptr<WebCore::ImageBuffer> m_imageBuffer;
> > +};
> > +
> > +class NicosiaImageBufferPipe final : public WebCore::ImageBufferPipe {
> > +public:
> > +    NicosiaImageBufferPipe();
> > +    virtual ~NicosiaImageBufferPipe() = default;
> > +
> > +private:
> > +    RefPtr<WebCore::ImageBufferPipe::Source> source() const final;
> > +    PlatformLayer* platformLayer() const final;
> > +
> > +    RefPtr<NicosiaImageBufferPipeSource> m_source;
> > +};
> 
> These could all be local in the implementation file, which would make this
> header unnecessary.

Sure

> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.cpp:-68
> > -    LockHolder locker(m_lock);
> 
> Why is any of changes in TextureMapperPlatformLayer needed?

There's a bug there currently that can cause the first frame not to show, and if drawing isn't updated past that point, it never shows (an update can be scheduled before activation happens - at least, I was seeing this during testing earlier patches, I presume given the fix is fairly mundane that it's still a possibility).

[Chris Lord]

Created attachment 409017 [details]
Patch

[Zan Dobersek]

Comment on attachment 409017 [details]
Patch

LGTM.

Additional review from someone more familiar with how CoreGraphics/CoreAnimations stack could be integrated with the proposed structure would be most appreciated.

[Simon Fraser (smfr)]

Comment on attachment 409017 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBufferPipe.h:38
> +class ImageBufferPipe : public RefCounted<ImageBufferPipe> {

This needs a comment saying what it does.

> Source/WebCore/platform/graphics/ImageBufferPipe.h:47
> +    static bool isSupported();

What is it that is supported?

[Chris Lord]

(In reply to Simon Fraser (smfr) from comment #35)
> Comment on attachment 409017 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409017&action=review
> 
> > Source/WebCore/platform/graphics/ImageBufferPipe.h:38
> > +class ImageBufferPipe : public RefCounted<ImageBufferPipe> {
> 
> This needs a comment saying what it does.

Sure (quick summary: An interface for marshalling ImageBuffer data from one thread to another).

> > Source/WebCore/platform/graphics/ImageBufferPipe.h:47
> > +    static bool isSupported();
> 
> What is it that is supported?

The use of ImageBufferPipe - I guess we could just rely on ::create returning nullptr though.

[Chris Lord]

Created attachment 409984 [details]
Patch

[Chris Lord]

Created attachment 410002 [details]
Patch

[EWS]

Committed r267735: <https://trac.webkit.org/changeset/267735>

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

[Radar WebKit Bug Importer]

<rdar://problem/69749155>