Add WebAcceleratedContentLayer backed by a texture to support accelerated content hosting
Created attachment 110948 [details] Patch
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.
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
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 :-(
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.
Created attachment 111099 [details] Patch
(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.
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
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
Created attachment 111115 [details] Patch
Created attachment 111117 [details] Patch
(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.
(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.
Comment on attachment 111117 [details] Patch Clearing flags on attachment: 111117 Committed r97550: <http://trac.webkit.org/changeset/97550>
All reviewed patches have been landed. Closing bug.