Bug 70084 – [Chromium] Add WebAcceleratedContentLayer backed by a texture to support accelerated content hosting

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

Add WebAcceleratedContentLayer backed by a texture to support accelerated content hosting

[Antoine Labour]

Created attachment 110948 [details]
Patch

[Antoine Labour]

Since PluginLayerChromium essentially does the job, I reused it. The naming might be confusing, I'd be ok renaming if that makes it clearer.
I also had to add a path that doesn't flip the texture, because depending on the cross-process texture transport (EGL vs GLX), texture is flipped or not.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110948 [details]
Patch

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

> Source/WebKit/chromium/public/WebAcceleratedContentLayer.h:36
> +class WebAcceleratedContentLayer : public WebLayer {

isn't WebContentLayer accelerated too?  i'm not sure i understand what the
accelerated prefix means here.

perhaps WebExternalContentLayer would be a better name?  it seems like this
is intended to provide the embedder with a way to define a layer from a
given textureId.

perhaps it is obvious to everyone else, but could you explain what setFlip
and flip() are?  it looks like it is a property, but "flip" seems like a
verb to me, and yet you are using it like it is a thing.

[Darin Fisher (:fishd, Google)]

Comment on attachment 110948 [details]
Patch

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

>> Source/WebKit/chromium/public/WebAcceleratedContentLayer.h:36
>> +class WebAcceleratedContentLayer : public WebLayer {
> 
> isn't WebContentLayer accelerated too?  i'm not sure i understand what the
> accelerated prefix means here.
> 
> perhaps WebExternalContentLayer would be a better name?  it seems like this
> is intended to provide the embedder with a way to define a layer from a
> given textureId.
> 
> perhaps it is obvious to everyone else, but could you explain what setFlip
> and flip() are?  it looks like it is a property, but "flip" seems like a
> verb to me, and yet you are using it like it is a thing.

also, since this doesn't extend from WebContentLayer, should its name
really have "Content" in it?  maybe WebExternalLayer?  I admit that I
don't really understand the difference between WebLayer and WebContentLayer
so my advice may be poor :-(

[Nat Duca]

Comment on attachment 110948 [details]
Patch

I like the idea of avoiding content in the name.

It would be cool to convey the unmanaged/externality of the texture in the name. I.e. something that the compositor is not creating for you, just passing it right on through.

[Antoine Labour]

Created attachment 111099 [details]
Patch

[Antoine Labour]

(In reply to comment #4)
> (From update of attachment 110948 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110948&action=review
> 
> > Source/WebKit/chromium/public/WebAcceleratedContentLayer.h:36
> > +class WebAcceleratedContentLayer : public WebLayer {
> 
> isn't WebContentLayer accelerated too?  i'm not sure i understand what the
> accelerated prefix means here.
> 
> perhaps WebExternalContentLayer would be a better name?  it seems like this
> is intended to provide the embedder with a way to define a layer from a
> given textureId.

Done. WebExternalTextureLayer we all agreed upon.
> 
> perhaps it is obvious to everyone else, but could you explain what setFlip
> and flip() are?  it looks like it is a property, but "flip" seems like a
> verb to me, and yet you are using it like it is a thing.

Done. "flipped". Added doc to explain what it means.

[Darin Fisher (:fishd, Google)]

Comment on attachment 111099 [details]
Patch

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

WebKit API LGTM.  You should get someone else to review the WebCore changes.

> Source/WebCore/ChangeLog:3
> +        Add WebAcceleratedContentLayer backed by a texture to support accelerated content hosting

nit: fix up the changelog

[James Robinson]

Comment on attachment 111099 [details]
Patch

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

I think you should explicitly document how the compositor will use the passed-in texture.  I believe that with our current implementation it would be something like:

In single threaded mode, the texture will only be used during an invocation of WebLayerTreeView::composite()
In multi threaded mode, the texture may be used starting at some undetermined time after the texture ID is set (specifically at the first commit) and then used at arbitrary points in time up until an undetermined time after the texture id or layer is destroyed, or until the WebLayerTreeView is destroyed.

I'm not sure if the second is workable - we might pick up weird intermediate states if the compositor tries to draw from a texture id that is bound to an FBO as a color attachment in another context. So far in the WebCore compositor we've been ignoring this problem as other issues are currently higher priority, but if this is exposed as API that's a little less workable.

R=me on the implementation stuff.

> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:58
> +void WebExternalTextureLayerImpl::paintContents(GraphicsContext& gc, const IntRect& clip)

nit: webkit style when an implementation doesn't use a parameter is to omit the name of the parameter. this avoids unused variable warnings on some compilers

[Antoine Labour]

Created attachment 111115 [details]
Patch

[Antoine Labour]

Created attachment 111117 [details]
Patch

[Antoine Labour]

(In reply to comment #10)
> (From update of attachment 111099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111099&action=review
> 
> I think you should explicitly document how the compositor will use the passed-in texture.  I believe that with our current implementation it would be something like:
> 
> In single threaded mode, the texture will only be used during an invocation of WebLayerTreeView::composite()
> In multi threaded mode, the texture may be used starting at some undetermined time after the texture ID is set (specifically at the first commit) and then used at arbitrary points in time up until an undetermined time after the texture id or layer is destroyed, or until the WebLayerTreeView is destroyed.

Done.

> 
> I'm not sure if the second is workable - we might pick up weird intermediate states if the compositor tries to draw from a texture id that is bound to an FBO as a color attachment in another context. So far in the WebCore compositor we've been ignoring this problem as other issues are currently higher priority, but if this is exposed as API that's a little less workable.

There's more than that that needs to be fixed for correct synchronization. Essentially we need to know when a compositing pass is done presenting the latest changes (so that you can have end-to-end synchronization). This is probably already problematic with out-of-thread compositor + plugins.
Right now, the only practical way is to look at the swapBuffers after the next swapBuffers.
Note however 2 things:
- The texture id is still propagated through the commit step. That means you can deal with a queue of textures and retiring/reusing them when you know it's not in use any more (2 swaps), and still get correct frames.
- The method used to generate those textures in the case of cross-process transport has some weak atomicity guarantees by the driver (i.e. the texture should be a complete frame).

> 
> R=me on the implementation stuff.
> 
> > Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:58
> > +void WebExternalTextureLayerImpl::paintContents(GraphicsContext& gc, const IntRect& clip)
> 
> nit: webkit style when an implementation doesn't use a parameter is to omit the name of the parameter. this avoids unused variable warnings on some compilers

Done.

[Antoine Labour]

(In reply to comment #9)
> (From update of attachment 111099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111099&action=review
> 
> WebKit API LGTM.  You should get someone else to review the WebCore changes.
> 
> > Source/WebCore/ChangeLog:3
> > +        Add WebAcceleratedContentLayer backed by a texture to support accelerated content hosting
> 
> nit: fix up the changelog

Done.

[WebKit Review Bot]

Comment on attachment 111117 [details]
Patch

Clearing flags on attachment: 111117

Committed r97550: <http://trac.webkit.org/changeset/97550>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.