There are a couple issues with the current way we handle WebGL frames and shuttling them (eventually) to the compositor Host. SurfaceStream (as it exists currently) doesn't work great with our architecture. We should not be blocking the compositor Host (what we do now, except for with E10S/B2G), nor should we block the main thread on frame render completion. This bug details (at least) the design of a replacement for WebGL compositing.

Comment on attachment 8450641 [details] [diff] [review] design-stream Review of attachment 8450641 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/doc/WebGLCompositing.md @@ +130,5 @@ > + > + if (layer::baking): > + if (layer::baking.PollFenceCompletion()): > + layer::newFront = layer::baking > + layer::baking = null what if !layer::backing or !layer::baking.PollFenceCompletion()? We have no layer::newFront, so which buffer are showing on the screen? @@ +149,5 @@ > +The buffer factory can pull from this collection to avoid creating a new buffer in most cases. > +Unfortunately, this requires a more sophisticated approach to refcounting than usual. > +Thankfully, all our refcounted objects are on the same thread, which makes things simpler. > + > +What we need for recycling is a refcountable object that holds a buffer and, on destruction, returns the buffer to the collection of recyclables. We already have two ways to recycle TextureClient objects. I think we should adapt one of them to work well with SurfaceStream rather than add a new way. @@ +158,5 @@ > + > +### Simplify SharedSurface > + > +Since our buffer manipulation is now restricted to the client side of the compositor, we no longer require that SharedSurface specializations support doing host-side operations, or exposing host-side objects. (such as a consumer-side texture of an EGLImage bridge) > +As such, we can at least reduce the complexity of SharedSurface, such that it's minimally able to support translating into a TextureClient/Host for use by Layers. I want to go as far as removing SharedSurface entirely, replace it by TextureClient/Host/MozSurface and improve the latters if it lacks some of SharedSurface's features.

> > @@ +158,5 @@ > > + > > +### Simplify SharedSurface > > + > > +Since our buffer manipulation is now restricted to the client side of the compositor, we no longer require that SharedSurface specializations support doing host-side operations, or exposing host-side objects. (such as a consumer-side texture of an EGLImage bridge) > > +As such, we can at least reduce the complexity of SharedSurface, such that it's minimally able to support translating into a TextureClient/Host for use by Layers. > > I want to go as far as removing SharedSurface entirely, replace it by > TextureClient/Host/MozSurface and improve the latters if it lacks some of > SharedSurface's features. SharedSurface and TextureClient/Host seems different level of abstraction. SharedSurface holds provider spscific members, but TextureClient/Host do not hold them.

(In reply to Nicolas Silva [:nical] from comment #2) > Comment on attachment 8450641 [details] [diff] [review] > design-stream > > Review of attachment 8450641 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/doc/WebGLCompositing.md > @@ +130,5 @@ > > + > > + if (layer::baking): > > + if (layer::baking.PollFenceCompletion()): > > + layer::newFront = layer::baking > > + layer::baking = null > > what if !layer::backing or !layer::baking.PollFenceCompletion()? > We have no layer::newFront, so which buffer are showing on the screen? This should never happen except for on init/resize, where we should be getting a blank buffer immediately anyway. > > @@ +149,5 @@ > > +The buffer factory can pull from this collection to avoid creating a new buffer in most cases. > > +Unfortunately, this requires a more sophisticated approach to refcounting than usual. > > +Thankfully, all our refcounted objects are on the same thread, which makes things simpler. > > + > > +What we need for recycling is a refcountable object that holds a buffer and, on destruction, returns the buffer to the collection of recyclables. > > We already have two ways to recycle TextureClient objects. I think we should > adapt one of them to work well with SurfaceStream rather than add a new way. > > @@ +158,5 @@ > > + > > +### Simplify SharedSurface > > + > > +Since our buffer manipulation is now restricted to the client side of the compositor, we no longer require that SharedSurface specializations support doing host-side operations, or exposing host-side objects. (such as a consumer-side texture of an EGLImage bridge) > > +As such, we can at least reduce the complexity of SharedSurface, such that it's minimally able to support translating into a TextureClient/Host for use by Layers. > > I want to go as far as removing SharedSurface entirely, replace it by > TextureClient/Host/MozSurface and improve the latters if it lacks some of > SharedSurface's features. I don't have a problem with this per se, but I don't know as it should be a blocker for this redesign. In particular: * TexClient/Host doesn't have an abstract API for Fence/Wait/Poll logic. * Neither TextureClientPool nor SimpleTextureClientPool really help with RefCounted-then-recycled buffers. Fixing these would heavily impact the time it takes for this redesign. As an aside, I don't know as we necessarily want to add a sometimes-used-sometimes-not API for Fence/Wait/Poll to TexClient. Really, TexHost is most similar to what WebGL needs in order to create and interact with a surface for compositing. That said, I think ShSurf is a decent place to handle creation and content-side interaction with a buffer, only to hand its transferrable handles to whatever TexClient it chooses. This is nearly what we have now, and is how the design proposed in this bug will work. At this point, 'SharedSurface' is a misnomer, and should be renamed. (When is the renaming for CanvasLayerComposite happening again? :) )

Comment on attachment 8450641 [details] [diff] [review] design-stream Review of attachment 8450641 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks like a good improvement! Just a few questions/concerns. ::: gfx/doc/WebGLCompositing.md @@ +42,5 @@ > +For `canvas`es, Layout instructs Layers to create a `CanvasLayer` for each canvas. > +With OMTC, we will get a `ClientCanvasLayer`, which is a mix of a `CopyableCanvasLayer` and a `ClientLayer`. > +`ClientCanvasLayer` uses the static `CanvasClient::CreateCanvasClient` to construct a `CanvasClient`, which it uses to communicate with the host-side counterpart to `CanvasClient`, which is `ImageHost`. > +The `CanvasClient` manages `TextureClient`s, which are mirrored by `TextureHost`s on the host-side. > +`CanvasLayerComposite` is a compositable[1] on the host-side roughly equivalent to a `ClientCanvasLayer`. This isn't strictly true. ClientCanvasLayer/CanvasLayerComposite are the client/host (respectively) side implementations of the abstract 'Layer' IPDL object (PLayer) which implements the layers API. CanvasClient/ImageHost (not CanvasHost since canvas and image have the same requirements for the host side and can share the same class) are the implementations for the 'Compositable' object (PCompositable). As you say below, compositables manage textures and know how to composite them, which is separate from implementing the layers API. Compositables take input from layers (like a transform, opacity, clipping), take a set of textures (maybe multiple tiles, black/white textures for component alpha) and some information of their own (like how the textures should be interpreted with regards to buffer rotation) and generate draw calls for the compositor. They also manage textures, so act as a swap-chain managing front/back buffers. @@ +130,5 @@ > + > + if (layer::baking): > + if (layer::baking.PollFenceCompletion()): > + layer::newFront = layer::baking > + layer::baking = null Which thread/process does layer::Update() happen on here? I assume it's the compositor thread since that's where the current layer::newFront update happens. Assuming that, once we solve nical's question (by keeping the existing newFront in-place until baking is ready I guess), then we'll lose the invariant that buffers can be recycles one frame after pushing a new one to the compositor. @@ +149,5 @@ > +The buffer factory can pull from this collection to avoid creating a new buffer in most cases. > +Unfortunately, this requires a more sophisticated approach to refcounting than usual. > +Thankfully, all our refcounted objects are on the same thread, which makes things simpler. > + > +What we need for recycling is a refcountable object that holds a buffer and, on destruction, returns the buffer to the collection of recyclables. What concrete c++ object is a 'buffer' in here? If the plan is to continue having SharedSurface only exist for the content:: slots, and have TextureClient/TextureHost for the layer:: ones, then which are we refcounting? If we can't rely on buffers being recycled after a fixed number of frames, then it sounds like we need to refcount TextureClient/Host (cross-process!). I'd definitely prefer if we could find a way to unify our buffer recycling code.

Comment on attachment 8450641 [details] [diff] [review] design-stream Review of attachment 8450641 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/doc/WebGLCompositing.md @@ +1,1 @@ > +Project Bakery {#projectbakery} Butter, Silk, Bakery? Every gfx bug has a code name now? @@ +4,5 @@ > +**This document is work in progress. Some information may be missing or incomplete.** > + > +### Goals > + > +* WebGL should composite quickly and with minimal overhead. Isn't the goal to decouple WebGL frame production from composition? so: * The Gecko compositor should not be blocked waiting for WebGL to complete rendering. @@ +42,5 @@ > +For `canvas`es, Layout instructs Layers to create a `CanvasLayer` for each canvas. > +With OMTC, we will get a `ClientCanvasLayer`, which is a mix of a `CopyableCanvasLayer` and a `ClientLayer`. > +`ClientCanvasLayer` uses the static `CanvasClient::CreateCanvasClient` to construct a `CanvasClient`, which it uses to communicate with the host-side counterpart to `CanvasClient`, which is `ImageHost`. > +The `CanvasClient` manages `TextureClient`s, which are mirrored by `TextureHost`s on the host-side. > +`CanvasLayerComposite` is a compositable[1] on the host-side roughly equivalent to a `ClientCanvasLayer`. A diagram would be great. @@ +60,5 @@ > + > +In some frame N, let A be the buffer currently used as the layer's frontbuffer. > +In the compositor's client-side update for frame N, we tell the host-side to switch to using buffer B as the new frontbuffer. > +If we assume that each client-side update is interleaved with at least one host-side update, then in the client-side update for frame N+1, we can know that A is no longer in use, as it will have been supplanted by B. > +Under this assumption, we do not need to add an explicit signal for when the compositor is done with a buffer, but can instead simply implicitly surmise when the buffer is no longer in use. Once again diagram. I'm having a hard time accepting the assumption. Is it because there is a glFinish between frames that means the buffer has been rendered to? (Because when the compositor has issued it's commands, the GPU still might not have consumed the buffer for rendering) @@ +130,5 @@ > + > + if (layer::baking): > + if (layer::baking.PollFenceCompletion()): > + layer::newFront = layer::baking > + layer::baking = null Why isn't this just a queue? The compositor displays (consumes) the front of the queue, Canvas draws appends new frames (produces)

Comment on attachment 8450641 [details] [diff] [review] design-stream Review of attachment 8450641 [details] [diff] [review]: ----------------------------------------------------------------- removing r? so bugzilla stops haranguing me.

Comment on attachment 8450641 [details] [diff] [review] design-stream My concern is already written by dan.

(In reply to Jeff Gilbert [:jgilbert] from comment #4) > (In reply to Nicolas Silva [:nical] from comment #2) > > I want to go as far as removing SharedSurface entirely, replace it by > > TextureClient/Host/MozSurface and improve the latters if it lacks some of > > SharedSurface's features. > > I don't have a problem with this per se, but I don't know as it should be a > blocker for this redesign. I wouldn't block this redisign on it either, but I'll keep pushing gently until we unify the redundant surface abstractions. > In particular: > > * TexClient/Host doesn't have an abstract API for Fence/Wait/Poll logic. TextureClient has some Fence logic (it has whatever is needed for gralloc, and the API is not set in stone, so we can and should eventually make Fences a work well with TextureClient for non-gralloc stuff as well. > * Neither TextureClientPool nor SimpleTextureClientPool really help with > RefCounted-then-recycled buffers. yeah, I'd like to have a more reusable pool system that is not only useful to tiling at some point. Perhaps we'll make tiling and/or video use whatever SurfaceStream will be using if it's reusable enough. Anyway, recycling buffers should not be a piece of logic that we need to implement 4 times. So some cleanup will have to be done at some point and it's worth evaluating how much of the existing code can be adapted and reused before redoing it for a new system (and in the present case it appears that not much can be reused in the short term). > > As an aside, I don't know as we necessarily want to add a > sometimes-used-sometimes-not API for Fence/Wait/Poll to TexClient. We already have some of it. If TextureClient had a better Fence API probably more layer types could take advantage of it (I say "better" but I haven't studied the question enough to judge whether the current fence stuff in TextureClient is good or bad beyond it's use with gralloc). > Really, > TexHost is most similar to what WebGL needs in order to create and interact > with a surface for compositing. > > That said, I think ShSurf is a decent place to handle creation and > content-side interaction with a buffer, only to hand its transferrable > handles to whatever TexClient it chooses. This is nearly what we have now, > and is how the design proposed in this bug will work. At this point, > 'SharedSurface' is a misnomer, and should be renamed. (When is the renaming > for CanvasLayerComposite happening again? :) ) If the goal of SurfaceStream is to have a stream between a canvas and the layer system and let the layers system have its own stream mechanism to get things to the compositor it makes sense, but then the compositor receives TextureClients rather than SharedSurfaces and we have to be careful about how the two streams interact to get good perf out of it. For example, currently the TextureClients that are used with webgl canvas are destroyed synchronously which is bad. Anyway, my comments are probably about longer term stuff, it wasn't clear to me what the scope of this redesign was.

Comment on attachment 8450641 [details] [diff] [review] design-stream Review of attachment 8450641 [details] [diff] [review]: ----------------------------------------------------------------- Removing the feedback request so that bugzilla stops nagging me, I suppose that you'll make modifications to the design document to address Dan's remarks so I'll wait for it to give another look.

(In reply to Dan Glastonbury :djg :kamidphish from comment #6) > Comment on attachment 8450641 [details] [diff] [review] > design-stream > > Review of attachment 8450641 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/doc/WebGLCompositing.md > @@ +1,1 @@ > > +Project Bakery {#projectbakery} > > Butter, Silk, Bakery? Every gfx bug has a code name now? In some ways, I want to be able to point at one of these larger refactors/projects by name, instead of by bug number (hah) or sometimes-confusing description. I would totally support having a pool of random nouns for project names. > > @@ +4,5 @@ > > +**This document is work in progress. Some information may be missing or incomplete.** > > + > > +### Goals > > + > > +* WebGL should composite quickly and with minimal overhead. > > Isn't the goal to decouple WebGL frame production from composition? so: > > * The Gecko compositor should not be blocked waiting for WebGL to complete > rendering. Fair. > > @@ +42,5 @@ > > +For `canvas`es, Layout instructs Layers to create a `CanvasLayer` for each canvas. > > +With OMTC, we will get a `ClientCanvasLayer`, which is a mix of a `CopyableCanvasLayer` and a `ClientLayer`. > > +`ClientCanvasLayer` uses the static `CanvasClient::CreateCanvasClient` to construct a `CanvasClient`, which it uses to communicate with the host-side counterpart to `CanvasClient`, which is `ImageHost`. > > +The `CanvasClient` manages `TextureClient`s, which are mirrored by `TextureHost`s on the host-side. > > +`CanvasLayerComposite` is a compositable[1] on the host-side roughly equivalent to a `ClientCanvasLayer`. > > A diagram would be great. I don't know if I can draw a diagram which is easier to understand than this text, but I can try. > > @@ +60,5 @@ > > + > > +In some frame N, let A be the buffer currently used as the layer's frontbuffer. > > +In the compositor's client-side update for frame N, we tell the host-side to switch to using buffer B as the new frontbuffer. > > +If we assume that each client-side update is interleaved with at least one host-side update, then in the client-side update for frame N+1, we can know that A is no longer in use, as it will have been supplanted by B. > > +Under this assumption, we do not need to add an explicit signal for when the compositor is done with a buffer, but can instead simply implicitly surmise when the buffer is no longer in use. > > Once again diagram. > > I'm having a hard time accepting the assumption. Is it because there is a > glFinish between frames that means the buffer has been rendered to? (Because > when the compositor has issued it's commands, the GPU still might not have > consumed the buffer for rendering) SwapBuffers should assure this. This seems to be how the existing Layers code deals with frontbuffers. In theory, we should use Fences to be sure that sampling has completed. Alternatively, we could just always create a new surface. This might cause fewer stalls. > > @@ +130,5 @@ > > + > > + if (layer::baking): > > + if (layer::baking.PollFenceCompletion()): > > + layer::newFront = layer::baking > > + layer::baking = null > > Why isn't this just a queue? The compositor displays (consumes) the front of > the queue, Canvas draws appends new frames (produces) I could add a queue, but I don't think it helps organize things in this case. The issue is that two different compositors could both have different buffers that they're baking. (display compositor and snapshotting compositor) Adding a queue would just hide the layer::baking, but nothing else. We need to be able to track content::front separately, so we can't just enqueue buffers from content::back directly.

Here's an update.

Comment on attachment 8463724 [details] [diff] [review] design-stream Review of attachment 8463724 [details] [diff] [review]: ----------------------------------------------------------------- The updates help clarify the situation for why it's not a queue for me. The ASCII art also helped me greatly. Thanks.

r=kamidphish I included updates from :kamidphish here.

Comment on attachment 8469572 [details] [diff] [review] design-stream Review of attachment 8469572 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/doc/WebGLCompositing-ProjectBaking.md @@ +22,5 @@ > +### Requirements > + > +* WebGL should never block either the main thread or compositor thread. > +* WebGL needs to assure that frames are complete before being sampled from by the compositor. > +* WebGL must handle its default framebuffer in accordance with its spec. This is somewhat vague, link to the relevant spec section? @@ +77,5 @@ > +* the host-side counterpart to `CanvasClient`. > + > +`CanvasLayerComposite` > + > +* is a compositable[^1] on the host-side roughly equivalent to a `ClientCanvasLayer`. Probably best not to call this a compositable, since it inherits from PLayer, not PCompositable. @@ +115,5 @@ > +client.front == B : host.front == B > +``` > +In some frame N, let A be the buffer currently used as the layer's frontbuffer. > +In the compositor's client-side update for frame N, we tell the host-side to switch to using buffer B as the new frontbuffer. > +If we assume that each client-side update is interleaved with at least one host-side update, then in the client-side update for frame N+1, we can know that A is no longer in use, as it will have been supplanted by B. I think this assumption is safe, as it should be guaranteed that the compositor has finished trying to composite with that buffer. We can't guarantee that the compositor has processed the update message and has dropped all references to it though, since that'll be racing with us starting the next client side update. I don't think that matters here, but worth noting. This is also dependent on synchronous transactions and/or refresh driver throttling. @@ +212,5 @@ > +The buffer factory can pull from this collection to avoid creating a new buffer in most cases. > +Unfortunately, this requires a more sophisticated approach to refcounting than usual. > +Thankfully, all our refcounted objects are on the same thread, which makes things simpler. > + > +What we need for recycling is a refcountable object that holds a buffer and, on destruction, returns the buffer to the collection of recyclables. I think this should all be fairly easy if we use TextureClient as our recyclable object. CanvasClient has the 3 RefPtr<>'s to them (backing, newFront, oldFront), and when we overwrite oldFront (and send the message to get the host-side to follow suit), we return the previous oldFrame buffer to the pool.