Closed Bug 995239 Opened 6 years ago Closed 6 years ago

MozSurface design document

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch WIP design document (obsolete) — Splinter Review
Work in progress. It's incomplete, I may have to rewrite it in a different form, but we can probably use it to finish formalizing/clarifying the design doc process, try and see what works, what doesn't.

So far, just a simple markdown file under gfx/doc
WIP for me as well, what you've done matches it, so take a look at https://wiki.mozilla.org/Platform/GFX/DesignDocumentationGuidelines and let me know if something is amiss there.
I still maintain that Client/HostTexture are either misnamed, or not the right abstractions we want.
I do not think attachment 8405431 [details] [diff] [review] is MozSurface design document. It is almost current implementation`s document.
As an aside, I think doing keeping a bug like this for designs is great, since I get cc emails on updates. :)
(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> I still maintain that Client/HostTexture are either misnamed, or not the
> right abstractions we want.

Agreed. The document will be updated whenever we move to better names.

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> I do not think attachment 8405431 [details] [diff] [review] is MozSurface
> design document. It is almost current implementation`s document.

Yes, that is the point (If I understood correctly). Currently the closest we have to whatever MozSurface will be is TextureClient/Host. The design document should correspond to the design we have in the code (with some notes about what we want it to be in the future, but we haven't yet detailed any plan that I am aware of). We'll update the design document as we precise our plans and update the code.
(In reply to Nicolas Silva [:nical] from comment #6)
> ...
> (In reply to Sotaro Ikeda [:sotaro] from comment #4)
> > I do not think attachment 8405431 [details] [diff] [review] is MozSurface
> > design document. It is almost current implementation`s document.
> 
> Yes, that is the point (If I understood correctly). Currently the closest we
> have to whatever MozSurface will be is TextureClient/Host. The design
> document should correspond to the design we have in the code (with some
> notes about what we want it to be in the future, but we haven't yet detailed
> any plan that I am aware of). We'll update the design document as we precise
> our plans and update the code.

It's worth mentioning this part in the document - there are plans to change the design, and the work is in progress, and put in as many details about the future we have...
Comment on attachment 8405431 [details] [diff] [review]
WIP design document

Review of attachment 8405431 [details] [diff] [review]:
-----------------------------------------------------------------

I think it is great overally, but some minor question come to my mind while reading.

::: gfx/doc/MozSurface.md
@@ +58,5 @@
> +
> +## Locking semantics
> +
> +In order to access the shared surface data users of MozSurface must acquire and release a lock on the surface, specifying the open mode (read/write/read+write).
> +

Does this lock able to lock across process? For example, 
If content side lock for write first and acquire the lock, then whether compositor locks for read/write valid? Should it wait or retry if failed?

@@ +136,5 @@
> +The sharing state of a MozSurface can be one of the following:
> +
> +* (1) Uninitialized (it doesn't have any shared data)
> +* (2) Local (it isn't shared with the another thread/process)
> +* (3) Shared (the state you would expect it to be most of the time)

How to promote a local surface become share if the underlying buffer is not shared from its construction?
memcpy is involved? That would be terrible in performance.

@@ +156,5 @@
> +
> +Using pure message passing was actually the first approach we tried when we created the first version of TextureClient and TextureHost. This strategy failed in several places, partly because of some legacy in Gecko's architecture, and partly because of some of optimizations we do to avoid copying surfaces.
> +
> +We need a given surface to be accessible on both the client and host for the following reasons:
> +* Gecko can at any time require read access on the client side to a surface that is shared with the host process, for example to build a temporary layer manager and generate a screenshot. This is mostly a legacy problem.

How can host side lock for read while client side still write?
I think this design is for Surface not Stream so it is a single buffer. so if the underlying buffer allow such use case how to sync the content?

@@ +162,5 @@
> +* Our buffer rotation code on scrollable non-tiled layers also requires a synchronization on the client side between the front and back buffers, while the front buffer is used on the host side.
> +
> +## Comparison with other APIs
> +
> +MozSurface is somewhat equivalent to Gralloc on Android/Gonk: it is a reference counted cross-process surface with locking semantics.

To make MozSurface totally equivalent to Gralloc, MozSurface must compatible with rendering backend(GL/D3D).
I think MozSurface is targeting on Multiple process Surface sharing, not rendering backend compatible.
(In reply to Chiajung Hung [:chiajung] from comment #8)
> I think it is great overally, but some minor question come to my mind while
> reading.

Thanks, I'll update the document with more precision around the areas you pointed out.

About the gralloc part, I meant it's somewhat like gralloc for the sharing part. We can think of it as a cross-backend gralloc-like abstraction. Where gralloc uses opengl texture for rendering, MozSurface usea TextureSource (which is also a level of abstraction higher than a gl texture).

I totally agree about a section for future plans. By the way we should talk about this future a bit eventually, because there are very few clear design decisions for the future that I am aware of (apart from making MozSurface more ubiquitous, make it feel less like a layers specific thing, and somehow merging or hiding the difference between TextureClient and TextureHost).
(In reply to Nicolas Silva [:nical] from comment #9)
> (In reply to Chiajung Hung [:chiajung] from comment #8)
> > I think it is great overally, but some minor question come to my mind while
> > reading.
> 
> Thanks, I'll update the document with more precision around the areas you
> pointed out.
> 
> About the gralloc part, I meant it's somewhat like gralloc for the sharing
> part. We can think of it as a cross-backend gralloc-like abstraction. Where
> gralloc uses opengl texture for rendering, MozSurface usea TextureSource
> (which is also a level of abstraction higher than a gl texture).
Gralloc buffer handles the sharing part where Desktop OpenGL PBO not provide.

If the platform is not OOP, I think we can create a PBO as MozSurface's backend. (The normal way to streaming texture on Desktop.) Which make painting algorithms run on CPU, and composition runs on GPU faster. (You can glMapBufferRange to get a virtual address to render to, and glUnmapBuffer after render done. Finally you can glTexImage2D from the buffer object rather than main memory (Better multi-threading))
> 
> I totally agree about a section for future plans. By the way we should talk
> about this future a bit eventually, because there are very few clear design
> decisions for the future that I am aware of (apart from making MozSurface
> more ubiquitous, make it feel less like a layers specific thing, and somehow
> merging or hiding the difference between TextureClient and TextureHost).

That is important! For our media team, they usually need a GraphicBuffer object but knows nothing about Texture{Client|Host}. As a result, they have to spend some time play around the API we exposed to see if that works. And they usually have very hard time when use the buffer. Various buffer die happened even if they hold a strong reference to the GraphicBuffer.

By the way I can provide a use case where Shared buffer may need becomes Local buffer, and make the life time full controlled by child side:
MediaRecoder/WebGL/Canvas2D API with a display:none video tag

In old design, Texture{Client|Host} acts more like a Stream(MozStream?), however, they are much more surface like now. It's great to see Texture{Client|Host} become a buffer abstraction. However, do you have any plan to hide IPC stuff from its API? For the user of such object, they should think the object is just a buffer, and they can use it freely: they can handle its life cycle ourselves, they can share it between processes without copy and can pass it in any IPC channel.
Comment on attachment 8405431 [details] [diff] [review]
WIP design document

Review of attachment 8405431 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/doc/MozSurface.md
@@ +140,5 @@
> +* (3) Shared (the state you would expect it to be most of the time)
> +* (4) Invalid (when for some rare cases we needed to force the deallocation of the shared data before the destruction of the TextureClient object).
> +
> +Surfaces can move from state N to state N+1 and be deallocated in any of these states. It could be possible to move from Shared to Local, but we currently don't have a use case for it.
> +

For the media recording case, the image will be passed to encoder after display. 
Therefore, the image(mozsurface) will become from shared to local state.

@@ +146,5 @@
> +In the other cases:
> +* (1) Unitilialized: There is nothing to do.
> +* (2) Local: The shared data is deallocated by the client side without need for a handshake, since it is not shared with other threads.
> +* (4) Invalid: There is nothing to do (deallocation has already happenned).
> +

I think here needs to add section about the threading model when deallocation happened, for example, the thread to handle  TextureClient and TextureChild deconstruction maybe different.
(In reply to Chiajung Hung [:chiajung] from comment #10)
> If the platform is not OOP, I think we can create a PBO as MozSurface's
> backend. (The normal way to streaming texture on Desktop.) Which make
> painting algorithms run on CPU, and composition runs on GPU faster. (You can
> glMapBufferRange to get a virtual address to render to, and glUnmapBuffer
> after render done. Finally you can glTexImage2D from the buffer object
> rather than main memory (Better multi-threading))

Agreed, but that's an implementation detail of TextureSource.

> That is important! For our media team, they usually need a GraphicBuffer
> object but knows nothing about Texture{Client|Host}. As a result, they have
> to spend some time play around the API we exposed to see if that works. And
> they usually have very hard time when use the buffer. Various buffer die
> happened even if they hold a strong reference to the GraphicBuffer.

At the moment, the best thing to do is use TextureClient and not store GraphicBuffer directly.
TexturelClient in it's current form has been designed to satisfy the needs of media.

> 
> By the way I can provide a use case where Shared buffer may need becomes
> Local buffer, and make the life time full controlled by child side:
> MediaRecoder/WebGL/Canvas2D API with a display:none video tag
> 
> In old design, Texture{Client|Host} acts more like a Stream(MozStream?),
> however, they are much more surface like now. It's great to see
> Texture{Client|Host} become a buffer abstraction. However, do you have any
> plan to hide IPC stuff from its API?

Definitely. That's pretty much one of the biggest goals of MozSurface (to hide IPC mechanisms) as far as I know. Right now we already have most of the features we need (you can use a TextureClient without sharing it over IPC, since all the ipc stuff happens lazily the first time you attach the texture to a compositable), but names are confusing and TextureClient exposes too many methods in its public API which adds to the confusion.

> For the user of such object, they
> should think the object is just a buffer, and they can use it freely: they
> can handle its life cycle ourselves, they can share it between processes
> without copy and can pass it in any IPC channel.

If you mean to share a buffer with several channels at the same time, that's not a reasonable thing to expect in the short/mid term. You can create a TextureClient, and attach it to a channel later, but as soon as you attached it to a channel, you won't be able to attach it to another channel. To lift this restriction we'd need to stop using IPDL which is not a good idea.
I propose that we land the document in its current form so that the next modifications will be easier to review with bugzilla's diff tool.
Attachment #8405431 - Attachment is obsolete: true
Attachment #8408226 - Flags: review?(sotaro.ikeda.g)
Attachment #8408226 - Flags: review?(milan)
Comment on attachment 8408226 [details] [diff] [review]
MozSurfce design document

Review of attachment 8408226 [details] [diff] [review]:
-----------------------------------------------------------------

I was only reviewing the mechanics and the layout of the file, rather than the actual content.  A few nits, unsurprising, given that we're just putting together the guidelines: https://wiki.mozilla.org/Platform/GFX/DesignDocumentationGuidelines

::: gfx/doc/MozSurface.md
@@ +1,1 @@
> +# MozSurface

If you do this:
MozSurface {#mozsurface}
==========

it will be better for doxygen extraction.

Then do:

## This document is work in progress.  Some information may be missing or incomplete.

then continue with the rest...

@@ +31,5 @@
> +TextureClient and TextureHost are the closest abstractions we currently have to MozSurface.
> +Inline documentation about TextureClient and TextureHost can be found in:
> +* http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.h
> +* http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.h
> +

You can just say TextureHost.h, TextureClient.h, and doxygen should produce the proper links.
Comment on attachment 8408226 [details] [diff] [review]
MozSurfce design document

Review of attachment 8408226 [details] [diff] [review]:
-----------------------------------------------------------------

Mean to r+ in the previous comment.
Attachment #8408226 - Flags: review?(milan) → review+
Comment on attachment 8408226 [details] [diff] [review]
MozSurfce design document

Review of attachment 8408226 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. It seems better to note that this document is preliminary and work in progress and is going to be changed a lot in future.

::: gfx/doc/MozSurface.md
@@ +2,5 @@
> +
> +## Goals
> +
> +We need to be able to safely and efficiently render web content into surfaces that can be shared accross processes.
> +MozSurface is a cross-process and backend-independent Surface API and not a stream API.

My understanding of MozSurface goal is the surfaces can be shared in process and across process. Current implementation support only across process.
Attachment #8408226 - Flags: review?(sotaro.ikeda.g) → review+
(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> Comment on attachment 8408226 [details] [diff] [review]
> MozSurfce design document
> 
> Review of attachment 8408226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. It seems better to note that this document is preliminary and
> work in progress and is going to be changed a lot in future.
> 
> ::: gfx/doc/MozSurface.md
> @@ +2,5 @@
> > +
> > +## Goals
> > +
> > +We need to be able to safely and efficiently render web content into surfaces that can be shared accross processes.
> > +MozSurface is a cross-process and backend-independent Surface API and not a stream API.
> 
> My understanding of MozSurface goal is the surfaces can be shared in process
> and across process. Current implementation support only across process.

Correction: 
Current implementation support only across gecko IPC. MozSurface's scope also include on across IPC consumer/producer model.
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> > My understanding of MozSurface goal is the surfaces can be shared in process
> > and across process. Current implementation support only across process.

We currently support not sharing TextureClients, although the API doesn't make it evident. There is a line in the requirements section saying that sharing across processes should not be the only purpose. I added another sentence to make it clearer.

> 
> Correction: 
> Current implementation support only across gecko IPC. MozSurface's scope
> also include on across IPC consumer/producer model.

Do you mean a swap chain abstraction/implementation ?

I would put this as a separate (equally important) design document (MozStream?) which would depend on MozSurface (while MozSurface doesn't need to depend on MozStream).
(In reply to Nicolas Silva [:nical] from comment #19)
> 
> I would put this as a separate (equally important) design document
> (MozStream?) which would depend on MozSurface (while MozSurface doesn't need
> to depend on MozStream).

Is there a reason why MozSurface needs to be as separate document? The original intent of MozSurface was to unify gfx layers and SurfaceStream. From this point of view, creating different documents seems weird. And We did not reach MozSurface point. We just have current gfx laysers. I think MozSurface should describe about MozSurface that we are going to reach in future.
(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> Is there a reason why MozSurface needs to be as separate document?

It doesn't strictly need to be in two different documents, but we need both a surface abstraction and a stream abstraction implemented on top of the surface. The swap chain won't solve all of our problems. For example thebes layers (tiled and non-tiled) need to be implemented on top of a surface abstraction because what we do to avoid copies is not compatible with a swap chain.

> The
> original intent of MozSurface was to unify gfx layers and SurfaceStream.

I thought the original idea was to have a clean and ubiquitous and safe surface abstraction that doesn't look like an implementaion detail of layers.
Back in the Paris workweek we talked about the two problems (having a surface abstraction and having a swap chain abstraction).

> From this point of view, creating different documents seems weird. And We
> did not reach MozSurface point.

I agree that we are not yet where we want to be with MozSurface (even as just a surface abstraction)

> We just have current gfx laysers. I think
> MozSurface should describe about MozSurface that we are going to reach in
> future.

Yes, I think we agree on what we want in the future: a good surface abstraction, and a good stream abstraction.
Where we disagree is whether the two goals should have the same name. I think we should keep those two separate because some features (tiling, etc.) require the surface abstraction without the stream.
MozSurface and MozStream (provided we keep the MozStream term) belong to different abstraction levels.

I really don't mean to take away the priorities of unifying surface-stream and layers stream, but I think the two should have distinct names and distinct design documents.
My understanding about MozSurface is opposite. "some features (tiling, etc.)" is extension of MozSurface.
https://hg.mozilla.org/mozilla-central/rev/452c9f6332bc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.