[chromium] Expose CCGraphicsContext as WebCompositorOutputSurface
Created attachment 155844 [details] Patch
This patch is a bit awkward because I'm trying to avoid doing a mess of renamings from CCGraphicsContext/context -> WebOutputSurface. I can do that as a cleanup patch, later. In the very near term, CCGraphicsContext contains an output surface instead of a graphics context.
Comment on attachment 155844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155844&action=review I'm happy with the approach. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:484 > + if (m_layerTreeHost->needsSharedContext() && !supports3D && !SharedGraphicsContext3D::haveForImplThread()) should it be: && support3D && (i.e. no !) ? > Source/WebKit/chromium/public/platform/WebCompositorOutputSurface.h:26 > +#include "../../../../Platform/chromium/public/WebCompositorOutputSurface.h" just FMI, do we need this bounce header? I assumed bounce headers in this dir were just for compat for grandfathered headers that moved to Platform/. > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:54 > + m_capabilities.supports3D = false; false? Shouldn't it be true if context is not NULL? Or do I not understand what supports3D mean?
Comment on attachment 155844 [details] Patch Attachment 155844 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13421123
Wrt all the supports3D, you're right. I was overtired. :) I'll have to defer to jamesr on the bounce header. I have to admit completely ignorance on the current GTFO-bkms.
That's only for files that already have #includes on the chromium side pointing to a Source/WebKit/chromium/... path. Do not add for new files.
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
BTW, we should probably be clear that the WebCompositorOutputSurface will be used on the impl thread even though it's created on the main thread.
(In reply to comment #8) > BTW, we should probably be clear that the WebCompositorOutputSurface will be used on the impl thread even though it's created on the main thread. Good point. Does a comment seem like sufficient way to indicate this?
Yes, absolutely.
Comment on attachment 155844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155844&action=review I'm fine with this approach. > Source/Platform/chromium/public/WebCompositorOutputSurface.h:37 > +class WebCompositorOutputSurface { Could we throw in as part of this patch a stub WebCompositorFrame.h : namespace WebKit { struct WebCompositorFrame { }; } Then in this class: virtual void drawCompositorFrame(const WebCompositorFrame&) = 0; That will simplify adding this later and clarify why the implementation of WebCompositorOutputSurface requires a routing_id_. > Source/Platform/chromium/public/WebLayerTreeView.h:74 > + // DEPRECATED. Use WebCompositorOutputSurface::Capabilities instead. Can we just move this over in this patch? It should only be used in a few places. > Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:38 > +// FIXME: rename to CCOutputSurface. I recommend instead doing: namespace WebCore { typedef WebKit::WebCompositorOutputSurface CCGraphicsContext; } Looks like you'd just need to add a create() function to WebCompositorOutputSurface to do this.
Comment on attachment 155844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155844&action=review > Source/Platform/chromium/public/WebCompositorOutputSurface.h:29 > +#include <wtf/PassOwnPtr.h> remove > Source/Platform/chromium/public/WebCompositorOutputSurface.h:39 > + virtual ~WebCompositorOutputSurface() { } should this be public? who has ownership of it - someone with just a WebCompositorOutputSurface* or someone with knowledge of a more concrete type? > Source/Platform/chromium/public/WebCompositorOutputSurface.h:40 > + virtual bool bindToClient(WebCompositorOutputSurfaceClient*) = 0; could you flesh out what this does in a comment? who's supposed to call it, what does the return value mean, etc? > Source/Platform/chromium/public/WebCompositorOutputSurface.h:46 > + struct Capabilities { > + bool supports3D; > + }; > + > + virtual const Capabilities& capabilities() const = 0; what's the difference between capabilities having/not having supports3D and context3D() being or not being NULL? > Source/Platform/chromium/public/WebCompositorOutputSurfaceClient.h:29 > +#include <wtf/PassOwnPtr.h> doesn't look like you need this (and if you did the include would have to be #if WEBKIT_IMPLEMENTATION guarded) > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:64 > + public WebKit::WebCompositorOutputSurfaceClient, should LRC be the client? with CCGraphicsContext, CCLayerTreeHostImpl is the owner and LRC just has a pointer to the 3d context since CCGraphicsContext is independent of the output type (2d or 3d) and LRC is a 3d-specific implementation. If WebCompositorOutputSurface is meant to be a generic thing used by both 2d and 3d, then I think you want a similar model - CCLayerTreeHostImpl is responsible for the output surface and LRC only knows about 3d-specific bits. I'm not sure if that's the case or not. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:288 > + // NOTE: named outputSurface because m_context is used for 3D context. Name chosen avoid renaming, > + // in consideration of the plan to rename CCGraphicsContext to CCOutputSurface. > + CCGraphicsContext* m_outputSurface; > WebKit::WebGraphicsContext3D* m_context; this seems a bit odd - why does LRC need to know about anything other than the 3d context? > Source/WebCore/platform/graphics/chromium/cc/CCRenderer.h:55 > + ~CCRendererClient() { } if you're adding a d'tor, please make it virtual. i'm not 100% sure why you need it though - just for safety?
> Could we throw in as part of this patch a stub > > WebCompositorFrame.h : > namespace WebKit { > struct WebCompositorFrame { }; > } Sure. Ill give it a crack and throw up a new patch. Definitely double-check my work though. :)
Re > Should this be public? who has ownership of it - someone with just a WebCompositorOutputSurface* or someone with knowledge of a more concrete type? The WebCompositorOutputSurface has a lifetime policy like WebGraphicsContext3D... the person that creates it deletes it. So, the destructor is public. I'll update the comments to be a bit more precise about this.
(In reply to comment #14) > Re > > Should this be public? who has ownership of it - someone with just a WebCompositorOutputSurface* or someone with knowledge of a more concrete type? > > The WebCompositorOutputSurface has a lifetime policy like WebGraphicsContext3D... the person that creates it deletes it. So, the destructor is public. I'll update the comments to be a bit more precise about this. I believe that's not true of WebGraphicsContext3D today. It's created by the embedder (on the main thread), but it's destroyed by CC on the impl thread.
Thats what I was thinking. Clearly I didn't explain myself correctly. :)
Comment on attachment 155844 [details] Patch Clearing review? to remove it from the pending review list since I think we've gathered enough feedback on this iteration to go another round.
Created attachment 155982 [details] Patch
Looks good to me, thanks. We can take care of the deprecated stuff in later patches.
Oh, forgot to mention that. As far as I can tell, I can't get rid of those because Chrome-side still references them. Once we've switched chrome to the output surface, we can delete the deprecated stuff.
Comment on attachment 155982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155982&action=review Would you mind rebasing this so EWS can run? Left a number of comments, looking pretty good. > Source/Platform/ChangeLog:17 > + * chromium/public/WebCompositorOutputSurface.h: Copied from Source/WebKit/chromium/tests/FakeCCGraphicsContext.h. I don't think these "copied from" lines are accurate - mind fixing/removing them? > Source/Platform/chromium/public/WebCompositorFrame.h:32 > +struct WebCompositorFrame { I'd rather just leave this whole file out, it's easy to add when we need it and having an empty struct isn't helpful in the meantime since we'll have to patch this file to do anything useful. > Source/Platform/chromium/public/WebCompositorOutputSurface.h:37 > +// Represents the output surface for a compositor. The compositor owns the > +// and manages its destruction. Its lifetime is: "compositor owns the and manages" - I think you accidentally a word > Source/Platform/chromium/public/WebCompositorOutputSurface.h:38 > +// 1. Created on the main thread Could you say who vends this? > Source/Platform/chromium/public/WebCompositorOutputSurface.h:41 > +// 3. If the 3D context is lost, then the entire output surface will be deleted Could you avoid the passive voice here and make it clear who is responsible for this (I'm pretty sure the compositor has to delete it, right)? > Source/Platform/chromium/public/WebLayerTreeView.h:75 > + bool forceSoftwareCompositing; There aren't any chromium uses of this according to cs.chromium.org or git gs. Just kill it. > Source/Platform/chromium/public/WebLayerTreeViewClient.h:69 > + // Creates the output surface. This may be called more than once > + // if the context gets lost. > + virtual WebCompositorOutputSurface* createOutputSurface() = 0; There are chromium-side implementations of WebLayerTreeViewClient. Since you're adding the WebCompositorOutputSurface type in this patch, you'll need to either land this with a default impl in WebLayerTreeViewClient.h and then land a implementation on the chromium side or add a default impl here. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:172 > - , m_context(resourceProvider->graphicsContext3D()) > + , m_context(resourceProvider->context()->context3D()) It seems really bizarre to get the context off of the resourceProvider. Why don't we just pass the context in to the LRC thing directly? > Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:38 > +// FIXME: rename fully to CCOutputSurface. > +typedef WebKit::WebCompositorOutputSurface CCGraphicsContext; Are we going to end up with CCOutputSurface as the type used everywhere (including exposed in the compositor's API)? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:49 > -class CCGraphicsContext; > +typedef WebKit::WebCompositorOutputSurface CCGraphicsContext; there's already a typedef in CCGraphicsContext.h - can you just #include CCGraphicsContext.h for now to avoid a redundant typedef? Alternately, leave a FIXME here > Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:41 > -class CCGraphicsContext; > +typedef WebKit::WebCompositorOutputSurface CCGraphicsContext; This is a third copy of the same typedef :/ > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.cpp:81 > +CCGraphicsContext* CCResourceProvider::context() This just seems straight up bizarre, IMO > Source/WebCore/platform/graphics/chromium/cc/CCResourceProvider.h:45 > -class CCGraphicsContext; > +typedef WebKit::WebCompositorOutputSurface CCGraphicsContext; and number 4 > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:182 > + if (m_layerTreeHost->needsSharedContext() && supports3D && !SharedGraphicsContext3D::haveForImplThread()) > + SharedGraphicsContext3D::createForImplThread(); You'll need to rebase this part (it's pretty straightforward) > Source/WebKit/chromium/public/platform/WebCompositorOutputSurfaceClient.h:26 > +#include "../../../../Platform/chromium/public/WebCompositorOutputSurfaceClient.h" You don't need this header at all - kill it. The only reason to put a forwarding header like this in WebKit/chromium/public/ is if there's already chromium code using it in that location > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:48 > +class TemporaryOutputSurfaceWrapper : public WebCompositorOutputSurface { I know the "temporary" here refers to how long you intend this code to live, but I think it's pretty confusing since the class name doesn't refer to a property of the type (this class is going to live for a fairly normal lifetime) but a property of the state of the code. I'd rather encode this with comments/ChangeLogs rather than in the type itself. > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:50 > + TemporaryOutputSurfaceWrapper(PassOwnPtr<WebGraphicsContext3D> context) explicit > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:75 > + virtual void sendFrameToParentCompositor(const WebCompositorFrame&) OVERRIDE > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:126 > + if (m_usingRealOutputSurface) { > + m_client->didRebindOutputSurface(success); > + return; > + } > + m_client->didRebindGraphicsContext(success); Do we really need this tracking + switching? Why not simply call both client methods and then we can switch the implementations over to ignoring the old call and respecting the new call in one patch.
Comments mostly fixed. A few notes: > Are we going to end up with CCOutputSurface as the type used everywhere (including exposed in the > compositor's API)? Yep. The web type is temporary, and I'm doing a typedef to keep the functionality changes separate from the renamings, as has been done in other patches recently. > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:126 > + if (m_usingRealOutputSurface) { > + m_client->didRebindOutputSurface(success); > + return; > + } > + m_client->didRebindGraphicsContext(success); > Do we really need this tracking + switching? Its not that much complexity, honestly, and it allows the compositor to be *completely without* the old flow. The only code that has the "legacy path" is the Web* data structures. Its just two ways to slice an onion. This is cleaner to me because it keeps the core (and already messy) bits down in compositor free of the old legacy paths.
Created attachment 156874 [details] Patch
Comment on attachment 156874 [details] Patch Attachment 156874 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13441966 New failing tests: compositing/backface-visibility/backface-visibility-hierarchical-transform.html compositing/geometry/bounds-clipped-composited-child.html compositing/geometry/bounds-ignores-hidden-composited-descendant.html compositing/framesets/composited-frame-alignment.html compositing/direct-image-compositing.html compositing/backface-visibility/backface-visibility-non3d.html compositing/backing/no-backing-for-clip-overlap.html compositing/backface-visibility/backface-visibility-3d.html compositing/clip-change.html compositing/layers-inside-overflow-scroll.html compositing/animation/animation-compositing.html compositing/self-painting-layers.html compositing/bounds-in-flipped-writing-mode.html compositing/flat-with-transformed-child.html compositing/geometry/ancestor-overflow-change.html compositing/scrollbar-painting.html compositing/culling/filter-occlusion-blur.html compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html compositing/columns/composited-in-paginated.html compositing/backing/no-backing-for-perspective.html compositing/clip-child-by-non-stacking-ancestor.html compositing/backface-visibility/backface-visibility-image.html compositing/culling/filter-occlusion-blur-large.html compositing/overflow-trumps-transform-style.html compositing/color-matching/image-color-matching.html compositing/backing/no-backing-for-clip.html compositing/backface-visibility/backface-visibility-simple.html compositing/backface-visibility/backface-visibility-webgl.html
Created attachment 156878 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 157129 [details] Patch
Comment on attachment 157129 [details] Patch Attachment 157129 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13446993
Created attachment 157137 [details] Patch
Created attachment 157140 [details] Patch
Comment on attachment 157140 [details] Patch I hate to bikeshed, but what does the word surface here mean?
(In reply to comment #30) > (From update of attachment 157140 [details]) > I hate to bikeshed, but what does the word surface here mean? The chrome-side gpu system uses that word to refer to an actual visible element. E.g. when you have a graphics context that is visible, it has a surface. Hence us picking up on the term here.
Comment on attachment 157140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157140&action=review R=me - needs some more polish in bits but is in good shape. I'm curious what your longer-term plans are w.r.t. which type the compositor implementation will depend on (left some comments inline). > Source/Platform/chromium/public/WebLayerTreeViewClient.h:65 > + virtual void didRebindGraphicsContext(bool success) { return; } nit: don't really need the return and i'd omit the parameter name on "success" since it's unused > Source/Platform/chromium/public/WebLayerTreeViewClient.h:73 > + // Signals a successful rebinding of the output surface (e.g. after a lost > + // 3D context event). > + virtual void didRebindOutputSurface(bool success) = 0; I realize this comment is just copy-paste, but reading it is seems wrong - this signals a rebind, but not necessarily a successful one (hence the parameter). Could maybe use a bit of wordsmithing > Source/WebCore/platform/graphics/chromium/cc/CCGraphicsContext.h:37 > +// FIXME: rename fully to CCOutputSurface. This comment seems to contradict the ChangeLog a bit - is the end state that the compositor all depends on WebCompositorOutputSurface (or some other exposed type) or that it depends on CCOutputSurface? Or perhaps does CCOutputSurface become the exposed type eventually? > Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:37 > +#include <public/WebCompositorOutputSurface.h> > #include <wtf/Noncopyable.h> > #include <wtf/PassOwnPtr.h> > #include <wtf/PassRefPtr.h> > #include <wtf/Threading.h> > > +namespace WebKit { > +class WebCompositorOutputSurface; > +} Do you need the include or the forward decl at all? I can't tell why this class was forward declaring CCGraphicsContext before > Source/WebKit/chromium/src/WebViewImpl.cpp:3696 > +// Adapts a pure WebGraphicsContext3D into a WebCompositorOutputSurface until > +// downstream code can be updated to produce output surfaces directly. > +class WebGraphicsContextToOutputSurfaceAdapter : public WebCompositorOutputSurface { I think if all you did in WebViewImpl was patch createOutputSurface() to defer out to m_client->createOutputSurface() then the following would happen on compositor startup: WebLayerTreeView::initialize() will call into CC which will eventually call back out to WebLayerTreeView::createOutputSurface() WLTV::createOutputSurface() will call WebLayerTreeView::createOutputSurface() WLTVC is WebViewImpl, so its default implementation of createOutputSurface() will return 0 WebLayerTreeView::createOutputSurface() will go down the fallback path and call WebLayerTreeViewClient::createContext3D() WLTVC is WebViewImpl, so it'll return a valid context from WebViewClient::createContext3D() WebLayerTreeView::createOutputSurface() will create its adapter around that context and away we go. Once you've implemented WebLayerTreeViewClient::createOutputSurface() for aura on the chromium side, the WLTV adapter won't be used any more for aura context. Once you've implemented WebViewClient::createOutputSurface() for renderer compositor instances, the WLTV adapter won't be used for those contexts once both are done and rolled in the WLTV adapter can go away. So basically this is a really long-winded way of saying I don't think you need this second adapter :) > Source/WebKit/chromium/src/WebViewImpl.cpp:-3727 > - // If we've already created an onscreen context for this view, return that. > - if (m_temporaryOnscreenGraphicsContext3D) > - webContext = m_temporaryOnscreenGraphicsContext3D.release(); Glad to see this temporary context code die at last > Source/WebKit/chromium/tests/FakeWebCompositorOutputSurface.h:36 > + static inline PassOwnPtr<FakeWebCompositorOutputSurface> create(PassOwnPtr<WebGraphicsContext3D> context3D) does the "inline" here mean anything? > Source/WebKit/chromium/tests/FakeWebCompositorOutputSurface.h:61 > + virtual void sendFrameToParentCompositor(const WebCompositorFrame& frame) OVERRIDE omit parameter name > Source/WebKit/chromium/tests/FakeWebCompositorOutputSurface.h:66 > + FakeWebCompositorOutputSurface(PassOwnPtr<WebGraphicsContext3D> context3D) explicit > Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:53 > + virtual WebGraphicsContext3D* createContext3D() OVERRIDE { ASSERT_NOT_REACHED(); return 0; } > + virtual void didRebindGraphicsContext(bool success) OVERRIDE { ASSERT_NOT_REACHED(); } these already have default impls that do what you want, can you just delete these overrides? one less thing to worry about when you remove the interface > Source/WebKit/chromium/tests/WebLayerTreeViewTest.cpp:59 > + virtual void didRebindOutputSurface(bool success) OVERRIDE { } omit parameter name
> I'm curious what your longer-term plans are w.r.t. which type the compositor implementation will depend on The challenge here is that post-GTFO, WebKit will embed WebLayerTree view, right? If we set WebViewImpl as a client to that, then output surface creation will have to flow through webkit even though its backed by a chromium type. But, in that case, it might be that the WebOutputSurface type is just a pointer wrapper around a chromium-provided output surface implementation? It seems to me that CCOutputSurface will be the final form in the CC code. I'll make sure changelogs reflect that. >> Source/Platform/chromium/public/WebLayerTreeViewClient.h:65 >> + virtual void didRebindGraphicsContext(bool success) { return; } > > nit: don't really need the return and i'd omit the parameter name on "success" since it's unused This s the base class, so i think its good to explain what the parameter means. Got rid of the return though. > Source/WebKit/chromium/src/WebViewImpl.cpp:3696 > +// Adapts a pure WebGraphicsContext3D into a WebCompositorOutputSurface until > +// downstream code can be updated to produce output surfaces directly. > +class WebGraphicsContextToOutputSurfaceAdapter : public WebCompositorOutputSurface { Fixed, but what I actually need to do is make the webviewimpl still pass the old createGraphicsContext3D up to its client. Then it allows the WLTVImpl fixup code to do its job. > does the "static inline" here mean anything? Iirw, you need this for statics that are implemented in a .h so it doesn't give you duplicate symbols during link. This idiom seems pretty heavily used in webkit (e.g. git grep "static inline" gives 800 uses….)
Created attachment 157393 [details] Patch
Created attachment 157402 [details] Patch
Comment on attachment 157402 [details] Patch Rejecting attachment 157402 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/Platform/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13471089
Created attachment 157416 [details] Patch for landing
Comment on attachment 157416 [details] Patch for landing Rejecting attachment 157416 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Tools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/13471208
Committed r125212: <http://trac.webkit.org/changeset/125212>
Reverted r125212 for reason: Compile failure on mac dbg builder Committed r125218: <http://trac.webkit.org/changeset/125218>
Committed r125219: <http://trac.webkit.org/changeset/125219>