Bug 107776 – [chromium] Allow the compositor's LayerTreeHostClient API to get the SharedGraphicsContext and bind GrContexts

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

[Dana Jansens]

[chromium] Provide compositor offscreen context through the WebLayerTreeViewClient interface

[Dana Jansens]

Created attachment 184385 [details]
Patch

[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.

[Stephen White]

Comment on attachment 184385 [details]
Patch

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

I'm probably not the best person to review this, since I get lost in Chromium's maze of twisty little GraphicsContext3D's (all different).

> Source/Platform/chromium/public/WebGraphicsContext3D.h:480
> +    WEBKIT_EXPORT GrContext* attachToGrContext();

Why is this now called "attach"?  It seems like it's still a get-or-create?  or create?

[Dana Jansens]

Comment on attachment 184385 [details]
Patch

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

>> Source/Platform/chromium/public/WebGraphicsContext3D.h:480
>> +    WEBKIT_EXPORT GrContext* attachToGrContext();
> 
> Why is this now called "attach"?  It seems like it's still a get-or-create?  or create?

I guess because the GrContext it creates is "attached to" the WGC3D ie draws into it. I'd like something in the function name to denote that, maybe "attach" is a bad word?

[James Robinson]

Comment on attachment 184385 [details]
Patch

I'm not really a fan of keeping the GrContext so tightly associated with the context.  That was a mistake with GraphicsContext3D and it's not going to get easier to deal with in the future, IMO.  I'd rather do it like this:

1.) Add something like WGC3D::bindGrContext() to associate a GrContext with a WGC3D
2.) Add ownership of the main-thread GrContext somewhere in WebCore (if it's still off of GraphicsContext3DPrivate that's OK with me - at least that way it only impacts WebCore)
3.) Figure out who can own and provide a GrContext for the filter context.  I think it should be maintained completely outside of WebKit, even if WebKit still owns+operates the off-thread WGC3D

[Dana Jansens]

Created attachment 186516 [details]
Patch

[Dana Jansens]

Here's an intermediate CL that:

1) Adds WEBKIT_EXPORT to WGC3D::createGrGLInterface() so we can call that from chromium land to create and bind a GrContext over there.

2) Lets us grab the impl thread context on the compositor's main thread and pass it over to the impl thread, in order to support the new WebLayerTreeViewClient API.

3) Add the WLTVClient API and have WebViewImpl implement it. The implementation of the methods for the compositor thread will move to RenderWidget later.

[James Robinson]

Comment on attachment 186516 [details]
Patch

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

> Source/Platform/chromium/public/WebGraphicsContext3D.h:475
> +    WEBKIT_EXPORT GrGLInterface* createGrGLInterface();

this is weird to me. WebGraphicsContext3D is mostly a pure virtual interface that is implemented by the embedder and provided to WebKit, not the other way around.  For WEBKIT_EXPORT to make sense the definition of this function would have to exist inside WebKit.dll and the caller to this function would have to be outside.  Is that the structure you're trying to end up with?

> Source/Platform/chromium/public/WebLayerTreeViewClient.h:76
> +    virtual WebKit::WebGraphicsContext3D* createOrGetOffscreenContext3dForMainThread() { return 0; }

Normally "create..." functions return pointers to objects that are then owned by the caller.  That isn't the case for these, since  it's owned by the other side of the interface.  I think they should be named something other than 'create...' and you should document the ownership rules in the header.

It's also a bit weird to me that these are on WebLayerTreeViewClient since they don't have anything to do with the specific WebLayerTreeView instance

[Dana Jansens]

(In reply to comment #8)
> (From update of attachment 186516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186516&action=review
> 
> > Source/Platform/chromium/public/WebGraphicsContext3D.h:475
> > +    WEBKIT_EXPORT GrGLInterface* createGrGLInterface();
> 
> this is weird to me. WebGraphicsContext3D is mostly a pure virtual interface that is implemented by the embedder and provided to WebKit, not the other way around.  For WEBKIT_EXPORT to make sense the definition of this function would have to exist inside WebKit.dll and the caller to this function would have to be outside.  Is that the structure you're trying to end up with?

Well, I was just leaving it as it really, but you make a good point. I can make the pure virtual onCreateGrGLInterface() method public instead, and implement this binding of the GrGLInterface to the WGC3D again in chromium. Code duplication :/ but across the library boundary, so it makes sense I guess.

> > Source/Platform/chromium/public/WebLayerTreeViewClient.h:76
> > +    virtual WebKit::WebGraphicsContext3D* createOrGetOffscreenContext3dForMainThread() { return 0; }
> 
> Normally "create..." functions return pointers to objects that are then owned by the caller.  That isn't the case for these, since  it's owned by the other side of the interface.  I think they should be named something other than 'create...' and you should document the ownership rules in the header.

Yeh, I agree the naming is awkward. I did "createOrGet..." everywhere to differentiate that it's not a "create" and it's not a "get". It's the ownership symmantics of a "get" with the side effects of a "create". I have no idea what other word to use for this though. Is "createAndGet..." any better?

> It's also a bit weird to me that these are on WebLayerTreeViewClient since they don't have anything to do with the specific WebLayerTreeView instance

Hmm... well, I thought this was the goal design. ui::Compositor is a LayerTreeHostClient so we want to go through that API. In WebKit land, this translates into the WebLayerTreeViewClient API. It serves the WebLayerTreeView when it asks for the embedder's offscreen context. I think that this has as much to do with a WebLayerTreeView instance as it does with a LayerTreeHost instance, in that the instance wants the context? Did you picture the compositor bindings WebLayerTreeViewImpl calling the SharedGraphicsContext3D code directly until we have RenderWidget(Compositor?) implementing LayerTreeHostClient instead of WebLayerTreeViewImpl?

[James Robinson]

(In reply to comment #9)
> Yeh, I agree the naming is awkward. I did "createOrGet..." everywhere to differentiate that it's not a "create" and it's not a "get". It's the ownership symmantics of a "get" with the side effects of a "create". I have no idea what other word to use for this though. Is "createAndGet..." any better?

Since this is a getter that does not transfer ownership, I do not think any prefix is needed.  Document the lazy creation aspect in a comment.

> 
> > It's also a bit weird to me that these are on WebLayerTreeViewClient since they don't have anything to do with the specific WebLayerTreeView instance
> 
> Hmm... well, I thought this was the goal design. ui::Compositor is a LayerTreeHostClient so we want to go through that API.

Yes

> In WebKit land, this translates into the WebLayerTreeViewClient API.

I think this is the source of confusion.  The cc:: API and the WebKit API are not the same thing and do not (and in many cases should not) blindly mirror each other.  The way that cc and WebKit interact with their embedders are different in a few ways.

cc needs access to a main and compositor thread context+GrContext.  I think those should be provided via cc::LayerTreeHostClient, since that's how cc makes requests of its embedder and all of the current cc APIs are done on a per-host basis.

WebKit's different.  WebKit only ever needs access to a main thread shared context and GrContext for canvas/etc.  WebKit never needs to use a compositor thread context or really do anything at all related to the compositor thread.  Currently, all of the shared contexts are managed in statics inside WebCore::SharedGraphicsContext3D, but that's in no way set in stone.  WebKit also has this neat WebKit::Platform abstraction by which WebKit can request platform support objects from its embedder.

I think a better factoring of all this would be for WebKit::Platform to provide the shared context/GrContext to WebKit. The compositor thread context pair can be handled purely on the chromium side.

[Dana Jansens]

(In reply to comment #10)
> (In reply to comment #9)
> > Yeh, I agree the naming is awkward. I did "createOrGet..." everywhere to differentiate that it's not a "create" and it's not a "get". It's the ownership symmantics of a "get" with the side effects of a "create". I have no idea what other word to use for this though. Is "createAndGet..." any better?
> 
> Since this is a getter that does not transfer ownership, I do not think any prefix is needed.  Document the lazy creation aspect in a comment.

Okay.

> > 
> > > It's also a bit weird to me that these are on WebLayerTreeViewClient since they don't have anything to do with the specific WebLayerTreeView instance
> > 
> > Hmm... well, I thought this was the goal design. ui::Compositor is a LayerTreeHostClient so we want to go through that API.
> 
> Yes
> 
> > In WebKit land, this translates into the WebLayerTreeViewClient API.
> 
> I think this is the source of confusion.  The cc:: API and the WebKit API are not the same thing and do not (and in many cases should not) blindly mirror each other.  The way that cc and WebKit interact with their embedders are different in a few ways.
> 
> cc needs access to a main and compositor thread context+GrContext.  I think those should be provided via cc::LayerTreeHostClient, since that's how cc makes requests of its embedder and all of the current cc APIs are done on a per-host basis.
> 
> WebKit's different.  WebKit only ever needs access to a main thread shared context and GrContext for canvas/etc.  WebKit never needs to use a compositor thread context or really do anything at all related to the compositor thread.  Currently, all of the shared contexts are managed in statics inside WebCore::SharedGraphicsContext3D, but that's in no way set in stone.  WebKit also has this neat WebKit::Platform abstraction by which WebKit can request platform support objects from its embedder.
> 
> I think a better factoring of all this would be for WebKit::Platform to provide the shared context/GrContext to WebKit. The compositor thread context pair can be handled purely on the chromium side.

Yeh, okay we agree on where this is going I think. But currently the renderer implementation of LTHClient is the WLTVImpl. I know we're going to change that, but I thought/hoped this might be a reasonable intermediate step until we move the LTHClient implementation. At least it allows cc/ to start using the LTHClient interface, and I can work through all the context creation plumbing stuff for the browser compositor without risking anything for the renderer compositor, until we've sorted it out.

[James Robinson]

I think you can implement the parts of cc::LayerTreeHostClient you need without doing this.

[James Robinson]

(In reply to comment #12)
> I think you can implement the parts of cc::LayerTreeHostClient you need without doing this.

For instance, as a first step you could move the WebSharedGraphicsContext3D:: calls on the chromium side from being inside cc/ to being in webkit/compositor_bindings. That would move the problem up a layer and allow you to define the interfaces you want in cc::.

[Dana Jansens]

(In reply to comment #13)
> (In reply to comment #12)
> > I think you can implement the parts of cc::LayerTreeHostClient you need without doing this.
> 
> For instance, as a first step you could move the WebSharedGraphicsContext3D:: calls on the chromium side from being inside cc/ to being in webkit/compositor_bindings. That would move the problem up a layer and allow you to define the interfaces you want in cc::.

Thanks for the clarification. I was hoping that was what you meant.

[Dana Jansens]

Created attachment 186699 [details]
Patch

[Dana Jansens]

Ok I hope this seems like a reasonable first step now. The important things in here:

1. Let the compositor context be queried on the main thread after being created.
2. Make a public virtual method on WGC3D to create a GrGLInterface for the context.

I think the next steps are something like, in some order:

- Make compositor use the LTHClient API to get its contexts.
- Make WebKit use the Platform API to get its contexts.
- Make renderer compositor's implementation of LTHClient skip past webkit.
- Make memory allocation callbacks not route thru WGC3D.

[James Robinson]

Comment on attachment 186699 [details]
Patch

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

> Source/WebCore/platform/chromium/support/GraphicsContext3DPrivate.cpp:125
> +    if (m_grContext)

is this just a set of style changes or is the behavior different somewhere?

[Dana Jansens]

Comment on attachment 186699 [details]
Patch

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

>> Source/WebCore/platform/chromium/support/GraphicsContext3DPrivate.cpp:125
>> +    if (m_grContext)
> 
> is this just a set of style changes or is the behavior different somewhere?

This is just less nesting mostly, it shouldn't behave different.

The non-whitespace changes are:
- moving the interface->fCallback stuff here
- using SkAutoTUnref for m_grContext instead of manual unref in destructor.

[James Robinson]

Comment on attachment 186699 [details]
Patch

OK

[Dana Jansens]

Comment on attachment 186699 [details]
Patch

Thanks :)

[WebKit Review Bot]

Comment on attachment 186699 [details]
Patch

Clearing flags on attachment: 186699

Committed r141943: <http://trac.webkit.org/changeset/141943>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.