[chromium] Generalize input handler messaging for ubercompositor

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Alexandre Elias]

[chromium] Generalize input handler messaging for ubercompositor

[Alexandre Elias]

Created attachment 151160 [details]
Patch

[Alexandre Elias]

This is another offshoot of Bug 88821 based on James's code review suggestions.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 151160 [details]
Patch

Attachment 151160 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13154787

[Alexandre Elias]

Created attachment 151168 [details]
Patch

[Nat Duca]

Comment on attachment 151168 [details]
Patch

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

So my basic question is, if this is supposed to be our GraphicsContext3D equivalent for software mode, why not generalize our GraphicsContext3D and pass that around? What is the FrameHandler giving us that can't be sealed inside the GraphicsContext abstraction?

> Source/WebCore/platform/graphics/chromium/cc/CCFrameHandler.h:35
> +class CCFrameHandler {

Yeah, naming here is a bit of a puzzle. Maybe we can chat tomorrow about names and the rough roles each abstraction assumes? I see what you're trying to do, but I'm trying to understand what this object does in "gpu accelerated mode"

> Source/WebCore/platform/graphics/chromium/cc/CCFrameHandler.h:40
> +    virtual void outputCompositorFrame(const WebKit::WebCompositorFrame&) = 0;

Bit confused about the mixing of Web and CC types here

> Source/WebKit/chromium/public/WebCompositorFrameHandlerClient.h:31
> +class WebCompositorFrame;

Erm, whats this? A renderpass list?

[Nat Duca]

I've changed my mind. I like this. I'm really sorry to switcheroo on you. I agree though, we gotta fix the names. But we do need some mechanism to communicate vsync as well, and this provides a lot of that capability.

[James Robinson]

Comment on attachment 151168 [details]
Patch

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

I like the direction a lot, but this needs a bit of thought around naming.  What about having WebCompositor instances provide the tear-offs?

>> Source/WebKit/chromium/public/WebCompositorFrameHandlerClient.h:31
>> +class WebCompositorFrame;
> 
> Erm, whats this? A renderpass list?

I can't find any definition or use of this type. Do we need it in this patch?

> Source/WebKit/chromium/public/WebCompositorThread.h:36
> +// Given a thread ID, this class provides a set of "tear-off" handlers for

this is misleading - the ID isn't for a thread (all compositor instances run on the same thread), it's for a compositor instance.  I think this needs a better name and better comments

> Source/WebKit/chromium/src/WebCompositorFrameHandlerImpl.cpp:44
> +    ASSERT(CCProxy::isImplThread());

I can't tell where this is picking CCProxy up from - could you add an explicit include? Did you compile test in debug mode?

[Nat Duca]

Comment on attachment 151168 [details]
Patch

Is there a reason we're not pursuing the WebCompositorRenderer as a base class of WGC3D and WGCUbercomp? That would be a much more natural way to do this.

[Nat Duca]

Sorrry, WebCompositorContext.

WebCompositorContext {
   type: ubercomp 2d, ubercomp 3d, webgraphicscontext 3d;
   context3d() { might return null; }
   setFrameData(Framedata) = 0;
}
 

CCCompositorContext {
   .. mostly forwards to wcc...
}

[James Robinson]

There's no reason to expose a context for embedders to use from the impl thread, is there?

[James Robinson]

(In reply to comment #9)
> (From update of attachment 151168 [details])
> Is there a reason we're not pursuing the WebCompositorRenderer as a base class of WGC3D and WGCUbercomp? That would be a much more natural way to do this.

WebGraphicsContext3D is used in places completely unrelated to compositing (i.e. WebGL), so having a compositor-related base class wouldn't make sense.  I don't know what WGCUbercomp is or would be.

[Nat Duca]

Sorry, I feel pretty strongly here.

I think that the compositor is based around a graphics context. That it is 2D or 3D should be only discovered and handled inside the instantiation of the renderer.

IN an ubercomp world, the compositor needs to do resource provider operations and send frame data objects. It needs to also know whether 3D transforms are supported.

Currently, we pass around a mix of GraphicsContext3D, CCGraphicsContext and WebGraphicsContext3D.

What I am arguing is that there is a base class of CCGraphicsContext that can contain the resource provider and a SendFrameData command. We can pass that throughout the *entire* CCLayerTreeHost/etc path that is 2D/3D context agnostic. The ONLY part of the system that would access the 3D bit of CCRenderer would be the LRC.

[Nat Duca]

Also, I'm not proposing that WGC3D derives from a base.

I'm saying WebCompositorContext has a 3D context or a 2D context, depending.
 WebCompositorContext {
   WebCompositorContext2D* context2d();
   WebCompositorContext3D* context3d();
 }

[Antoine Labour]

(In reply to comment #13)
> Sorry, I feel pretty strongly here.
> 
> I think that the compositor is based around a graphics context. That it is 2D or 3D should be only discovered and handled inside the instantiation of the renderer.
> 
> IN an ubercomp world, the compositor needs to do resource provider operations and send frame data objects. It needs to also know whether 3D transforms are supported.
> 
> Currently, we pass around a mix of GraphicsContext3D, CCGraphicsContext and WebGraphicsContext3D.
> 
> What I am arguing is that there is a base class of CCGraphicsContext that can contain the resource provider and a SendFrameData command. We can pass that throughout the *entire* CCLayerTreeHost/etc path that is 2D/3D context agnostic. The ONLY part of the system that would access the 3D bit of CCRenderer would be the LRC.

We already have the CCRenderer abstraction for that, no?

CCLTHI has a CCRenderer which deals with putting the quad list to the screen. An  implementation of it is LRC, which directly renders it to a surface using a 3D context, another implementation would send the FrameData to the parent compositor - that one may not even need a context at all, it just talks to the CCResourceProvider to manage resource transfers, and passes the FrameData to the embedder (which can IPC it over).
Anything above CCLTHI (layers, etc) already don't need a context - they go through CCResourceProvider - except when they need to do GL operations (WebGL, accelerated canvas) - but it doesn't even have to be the same context as the CCRenderer.

[Nat Duca]

The bit I dont grok is the construction logic. E.g. CCLTHC::createCompositorContext() method.

[Nat Duca]

That is to say, how does CCLTHI boot up and know what resource provider and ccrenderer to instantiate? I was just figuring we'd create a context, then go "aha, you're 2d, create this provider and renderer." Maybe you had a different plan to create the right instances of things?

[Antoine Labour]

I see what you mean. I don't think we've thought about the details of the construction.

For the part that forwards to the parent compositor (I don't have a good name for that thing, say the "parent compositor proxy" for lack of better), it doesn't need a context per se, since it just needs to send data over IPC, so that's why the link to a context is not necessarily obvious - a fortiori if in the future we do the uploads in the parent compositor, and we wouldn't even have a compositor context in the child.

That being said, there is an implicit link in terms of capabilities of the parent compositor, i.e. if the parent isn't 3d-capable, then we shouldn't use a 3d context to manage the resources. There's probably more subtleties (e.g. max resource size, anti aliasing concerns, etc), and it sounds like we at least need to share some capabilities description between the 3d context and the parent compositor proxy.

Now, we could merge the context and the parent compositor proxy into a single WebCompositorContext class, I don't have a strong opinion (except I wouldn't call it "context", but whatever), we just need to acknowledge that there may not be the proxy part (for the top-level compositor), nor the context part (for software-only), so it would look more like:
WebCompositorContext {
  WebParentCompositorProxy* proxy();  // may be null
  WebGraphicsContext3D* context();  // may be null
  Capabilities caps();  // e.g. "type", max resource size, and other concerns.
};

For the 2D context concept, I don't think we need any, do we? IOW skia contexts don't allocate resources that need to be kept for frame to frame... Or is it where we want to expose the management of transferable resources (in shm)?

[Nat Duca]

(In reply to comment #18)
> For the part that forwards to the parent compositor (I don't have a good name for that thing, say the "parent compositor proxy" for lack of better), it doesn't need a context per se, since it just needs to send data over IPC, so that's why the link to a context is not necessarily obvious - a fortiori if in the future we do the uploads in the parent compositor, and we wouldn't even have a compositor context in the child.

Dumb question: How does ReourceProvider get implemented when you dont have a wgc3d? Its going to need to get out of cc somehow to do IPCs...

> 
That being said, there is an implicit link in terms of capabilities of the parent compositor, i.e. if the 
> 
> Now, we could merge the context and the parent compositor proxy into a single WebCompositorContext class, I don't have a strong opinion (except I wouldn't call it "context", but whatever), we just need to acknowledge that there may not be the proxy part (for the top-level compositor), nor the context part (for software-only), so it would look more like:
> WebCompositorContext {
>   WebParentCompositorProxy* proxy();  // may be null
>   WebGraphicsContext3D* context();  // may be null
>   Capabilities caps();  // e.g. "type", max resource size, and other concerns.
> };
>
 
I buy that context is the wrong word. Its this where the word Surface or output come into play? E.g. this is the thing that you're outputting into...

Vsync sort of relate here. Originally, I was hijacking the input filter mechanism for it. But, as we saw here, thats sort of odd. And, its not really part of a 2d or 3d api. Its really a property of this WebCompositorSomething that is the parent "thing".


> For the 2D context concept, I don't think we need any, do we?
Sorry, need what?

>IOW skia contexts don't allocate resources that need to be kept for frame to frame... Or is it where we want to expose the management of transferable resources (in shm)?

For the 2D device, there will still be bitmaps that back resources... Skia Android can do stuff with these being persisted rather than us managing them as void*'s...

I think this gets back to the question of how to map ResourceProvider to the shm/software case. I sort of figured we could dangle the resource APIs off of this thing as well.

[Antoine Labour]

(In reply to comment #19)
> (In reply to comment #18)
> > For the part that forwards to the parent compositor (I don't have a good name for that thing, say the "parent compositor proxy" for lack of better), it doesn't need a context per se, since it just needs to send data over IPC, so that's why the link to a context is not necessarily obvious - a fortiori if in the future we do the uploads in the parent compositor, and we wouldn't even have a compositor context in the child.
> 
> Dumb question: How does ReourceProvider get implemented when you dont have a wgc3d? Its going to need to get out of cc somehow to do IPCs...

I guess that'd go to the parent compositor proxy interface. SendFrameData would have:
- the list of quads
- the list of resources that are passed from child to parent
- the list of uploads to do to resources (resource ID + shm handle + rects).

> 
> > 
> That being said, there is an implicit link in terms of capabilities of the parent compositor, i.e. if the 
> > 
> > Now, we could merge the context and the parent compositor proxy into a single WebCompositorContext class, I don't have a strong opinion (except I wouldn't call it "context", but whatever), we just need to acknowledge that there may not be the proxy part (for the top-level compositor), nor the context part (for software-only), so it would look more like:
> > WebCompositorContext {
> >   WebParentCompositorProxy* proxy();  // may be null
> >   WebGraphicsContext3D* context();  // may be null
> >   Capabilities caps();  // e.g. "type", max resource size, and other concerns.
> > };
> >
> 
> I buy that context is the wrong word. Its this where the word Surface or output come into play? E.g. this is the thing that you're outputting into...

I like that. WebCompositorOutput or WebCompositorDestination or something else.

> 
> Vsync sort of relate here. Originally, I was hijacking the input filter mechanism for it. But, as we saw here, thats sort of odd. And, its not really part of a 2d or 3d api. Its really a property of this WebCompositorSomething that is the parent "thing".

Agreed, and we'll also need the back-pressure IPCs (which will also recycle the resources that aren't in use any more by the parent).

> 
> 
> > For the 2D context concept, I don't think we need any, do we?
> Sorry, need what?

A concept of a 2D context...

> 
> >IOW skia contexts don't allocate resources that need to be kept for frame to frame... Or is it where we want to expose the management of transferable resources (in shm)?
> 
> For the 2D device, there will still be bitmaps that back resources... Skia Android can do stuff with these being persisted rather than us managing them as void*'s...
> 
> I think this gets back to the question of how to map ResourceProvider to the shm/software case. I sort of figured we could dangle the resource APIs off of this thing as well.

Ok, I can buy that. Again, not sure if "context" is the right word since we only concern ourselves with the resource allocation, but we can have that concept here - WebResourceAllocator2D?
That can also be used for the renderer-doesn't-upload-anymore extension, to allocate the shm handles (or whatever backing).

[Nat Duca]

I can try to hack up a WebCompositorOutput class and put things on there. I had originally told @aelias that I'd do that, anyway.

Maybe WebCompositorOutputSurface. So that when we talk about it we can say "Oh, you need to put that on the output surface." Saying "put it on the output" doesnt' work too good. :)

> > > For the 2D context concept, I don't think we need any, do we?
> > Sorry, need what?
> 
> A concept of a 2D context...
Probably not. It seems to me that the main output class would have SendFrameData, frame flow control stuff, and also david's WIP upload flow control stuff. Presumably also methods for allocation/shm that are common between ubercomp/2d/3d... for those, maybe we can just put them right on the WebCompositorOutput

> Ok, I can buy that. Again, not sure if "context" is the right word since we only concern ourselves with the resource allocation, but we can have that concept here - WebResourceAllocator2D?

Understood and agreed on the context point. Is there any common APIs between 3D/ubercomp(2d/3d) allocator? It seems to me that the basic shm-based texture upload probably looks the same regardless of ubercomp state?

This is part of me trying ot figure out if we need a specific resource provider/allocator class for 2d, or whether resource managment for the 2d case is really the absence of webgl-specific interop features....

[Nat Duca]

An alternate way to do this posted here:
https://bugs.webkit.org/show_bug.cgi?id=92890

[Alexandre Elias]

For resource provider, there are two ways to do it:
1. Send shared memory to GPU process (current approach) and use a shared context to give them to the root compositor
2. Send shared memory to browser process and hand the software bitmap over to WebLayerTreeView interface of root compositor (required for software compositing, and also Android's desired approach for all modes)

The main advantage of (1) is that it's easily changed to Ganesh-based painting.  So Antoine told me he wants ChromeOS to continue using this approach.  Note that in (1), the shared memory is handled deep inside the GL implementation.

So for (2), let's write a completely separate shared-memory implementation that doesn't try to collapse in any of the existing code, and make the rest of CC indifferent to which resource provider is used.

[Antoine Labour]

(In reply to comment #23)
> For resource provider, there are two ways to do it:
> 1. Send shared memory to GPU process (current approach) and use a shared context to give them to the root compositor
> 2. Send shared memory to browser process and hand the software bitmap over to WebLayerTreeView interface of root compositor (required for software compositing, and also Android's desired approach for all modes)
> 
> The main advantage of (1) is that it's easily changed to Ganesh-based painting.  So Antoine told me he wants ChromeOS to continue using this approach.  Note that in (1), the shared memory is handled deep inside the GL implementation.
> 
> So for (2), let's write a completely separate shared-memory implementation that doesn't try to collapse in any of the existing code, and make the rest of CC indifferent to which resource provider is used.


At least for the pure software case, I'd imagine we don't want to involve a GPU process just to pass SHM handles around.


To be clear, what I see for #1 is that it simplifies hooking up everything, because what we share between the child and the parent is textures, not SHM buffer that need to be uploaded to textures, because the latter needs more logic to schedule the uploads at the right time, extend the IPCs etc, whereas the former would be identical whether the texture comes from a tile, webgl, canvas, pepper, video (software or hardware), etc.

[Nat Duca]

So it sounds like (as is often the case with chrome) we have to do #1 and #2. :) Therefore, +1 to making this pluggable.

[Alexandre Elias]

Comment on attachment 151168 [details]
Patch

Clearing review flag since https://bugs.webkit.org/show_bug.cgi?id=92890 mostly supercedes this.