[chromium] Remove enableCompositorThread field from CCSettings and derive it from CCThreadProxy::hasThread
Created attachment 112383 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 112383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112383&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:69 > + // Accelerated Painting is not supported in threaded mode. Turn it off. > + m_settings.acceleratePainting = false; This isn't true any more, you can remove this > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:206 > + ASSERT(CCThreadProxy::hasThread()); > + if (CCThreadProxy::hasThread()) > + m_proxy->setNeedsAnimate(); > + else > + m_client->scheduleComposite(); it doesn't make much sense to have an ASSERT() and then have an else clause to deal with the case where the ASSERT fails
Comment on attachment 112383 [details] Patch Attachment 112383 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10210337
Created attachment 112421 [details] Patch
Comment on attachment 112421 [details] Patch R=me. the WebKit/chromium/public/ warning is just for a comment, so I think you're OK submitting without waiting for Darin on this one.
Committed r98429: <http://trac.webkit.org/changeset/98429>
Broke the build: https://bugs.webkit.org/show_bug.cgi?id=70881 The problem: DRT. But also discovered other issues. Will re-attack this as time permits, lower priority than scheduling. :/
Created attachment 115900 [details] Patch
My CCMainThread->CCThread integration caused some problems with the way Aura initializes the compositor. I have modified this patch to make init and shutdown of the compositor explicit, which makes things not just cleaner than before [in my estimation] but also makes it clear when the compositor should be available. This is paired with http://codereview.chromium.org/8558030/ now
Comment on attachment 115900 [details] Patch WebKit API changes LGTM
Comment on attachment 115900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115900&action=review Mostly looks good but the shutdown() semantics seem a little scary. I think that needs to be resolved and documented clearly in WebCompositor.h > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:199 > + if (CCProxy::implThread()) a lot of code in this patch null-checks CCProxy::implThread() to tell if we're in threaded mode or not. should we provide a bool getter with a friendlier name? > Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:70 > + return !mainThreadIsImplThread && currentThread() == s_mainThread->threadID(); this looks weird but maybe it's just because of the variable name. "mainThreadIsImplThread" makes me think that when that flag is set we should only return true if the current thread is the impl thread. This seems to be doing something more permissive. maybe the bool should be "currentThreadIsImplThread" ? > Source/WebCore/platform/graphics/chromium/cc/CCProxy.cpp:76 > + return mainThreadIsImplThread || currentThread() == implThreadID; same here - from the variable name, i'd think that if the bool is true this should check that the current thread == the main thread > Source/WebKit/chromium/public/WebCompositor.h:45 > + // compositing, pass a WebThread. To use single-threaded compositing, pass > + // 0. line wrapping a bit awkward - can you move the '0' up to the previous line at least? > Source/WebKit/chromium/public/WebCompositor.h:48 > + WEBKIT_EXPORT static void shutdown(); the requirements and conditions on this function should be documented here - i.e. it looks like you can't call this with active compositor instances and it's not valid to call any functions on this interface after shutdown() depending on what the initialize/shutdown requirements are > Source/WebKit/chromium/src/WebCompositorImpl.cpp:31 > +#include "WebKit.h" this #include doesn't appear to be used > Source/WebKit/chromium/src/WebCompositorImpl.cpp:42 > +bool webCompositorInitialized = false; can we make this a static member s_initialized? we're doing that for WebComopsitorImpl::s_nextAvailableIndentifier and WebCompositorImpl::s_compositors so it seems nice to be consistent > Source/WebKit/chromium/src/WebCompositorImpl.cpp:59 > + if (webCompositorInitialized) > + ASSERT_NOT_REACHED(); ASSERT(!webCompositorInitialized) ? > Source/WebKit/chromium/src/WebCompositorImpl.cpp:62 > + CCProxy::setMainThread(CCThreadImpl::create(webKitPlatformSupport()->currentThread()).leakPtr()); seems weird to .leakPtr() this here, and then make an explicit delete call in shutdown(). Can we hang on to this pointer explicitly in a static with some comments on the lifetime? > Source/WebKit/chromium/src/WebCompositorImpl.cpp:64 > + CCProxy::setImplThread(CCThreadImpl::create(implThread).leakPtr()); we've been leaking this object since it's pretty small and only exists once per process, but if we're going to delete the mainthread CCThreadImpl it seems to make sense to treat this one the same way. > Source/WebKit/chromium/src/WebCompositorImpl.cpp:69 > +void WebCompositor::shutdown() could you add an ASSERT() that the s_compositors map is empty (i.e. enforce that you can't call this with active compositor instances) ? > Source/WebKit/chromium/src/WebCompositorImpl.cpp:74 > + webCompositorInitialized = false; does this mean we now support shutting down and reinitializing in the same address space? WebKit in general does not support this operation, and I'm not sure that our compositor does either Unless this is really necessary I'd prefer to not support this and leave the initialized flag alone so another call to initialize() will ASSERT(). Alternately if we do need to support this we need some tests to make sure it works and doesn't regress. > Source/WebKit/chromium/src/WebLayerTreeView.cpp:33 > +#include "cc/CCThreadProxy.h" this looks unused > Source/WebKit/chromium/src/WebLayerTreeViewImpl.cpp:96 > + GraphicsContext3DPrivate::ThreadUsage usage = CCThreadProxy::implThread() ? GraphicsContext3DPrivate::ForUseOnAnotherThread : GraphicsContext3DPrivate::ForUseOnThisThread; shouldn't this be CCProxy::implThread() ? > Source/WebKit/chromium/src/WebViewImpl.cpp:137 > +#include "cc/CCThreadProxy.h" I think this should be CCProxy.h, since it's pulling in a header to check CCProxy::implThread()
(In reply to comment #12) > could you add an ASSERT() that the s_compositors map is empty (i.e. enforce that you can't call this with active compositor instances) ? That's not threadsafe. I did something similar though -- checking that there are no CCLTH's is safe. > > Source/WebKit/chromium/src/WebCompositorImpl.cpp:74 > does this mean we now support shutting down and reinitializing in the same address space? WebKit in general does not support this operation, and I'm not sure that our compositor does either So we actually already do this in the CCLayerTreeHostProxy. A lot of my refactoring of CCMainThread/CCThread has enabled this. We should be able to do this, and I think it is good software engineering discipline to encourage us to do so. The compositor feature is emphatically not webkit -- its a small library and if you can't turn it on and off, something is screwed up with our implementation. > Unless this is really necessary I'd prefer to not support this and leave the initialized flag alone so another call to initialize() will ASSERT(). Alternately if we do need to support this we need some tests to make sure it works and doesn't regress. Disagree. And, the CCLayerTreeHost tests test this.
Also, WebKit.h is needed to obtain webPlatformSupport()
Created attachment 115944 [details] Patch
Comment on attachment 115944 [details] Patch Attachment 115944 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10525043
Created attachment 115946 [details] Patch
So, in adding asserts to shutdown, I found an issue with the opacity test and our test harness. The core issue is that opacity test changes the root layer. The shutdown code was hard coded to assume that the root layer was not changed. When it was, the clean up code we had to break the circular reference between layers & treeHost would fail to break the reference and the CCLTH would stay alive. Fixing this proved tricky because some tests expect endTest to "destroy" the CCLayerTreeHost in order to test recovery. Other tests dont particularly care when the layerTreeHost dies. I re-did things so that endTest just ends the test. Tests that want the layerTreeHost to die with messages in flight need to orchestrate that death explicitly. In doing this I also discovered that the continuous redrawing test was set up to just exit right after starting. I.e. not doing anything at all. :'(
Comment on attachment 115946 [details] Patch Attachment 115946 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10515450
(In reply to comment #13) > (In reply to comment #12) > > could you add an ASSERT() that the s_compositors map is empty (i.e. enforce that you can't call this with active compositor instances) ? > That's not threadsafe. I did something similar though -- checking that there are no CCLTH's is safe. > Interesting - what are the threading requirements for the shutdown() call then? I assumed it was the same as the initialize and fromIdentifier() (has to be called from the compositor thread, whatever that is) so checking the s_compositor map should be fine. If this can be called from other threads then it makes things a bit more interesting but still possible. Earlier versions of this code had a mutex guarding s_compositor. From the header docs I would assume shutdown() has to be called from the compositor thread.
(In reply to comment #20) > Interesting - what are the threading requirements for the shutdown() call then? I assumed it was the same as the initialize and fromIdentifier() (has to be called from the compositor thread, whatever that is) so checking the s_compositor map should be fine. If this can be called from other threads then it makes things a bit more interesting but still possible. Earlier versions of this code had a mutex guarding s_compositor. > > From the header docs I would assume shutdown() has to be called from the compositor thread. Ah, the idea was this is a main thread only concept. This is building on the fact that, on ToT, SetImplThread was a main thread function [not documented to be so, however]. To me, WebCompositor contained, from an API point of view, our global interfaces for the compositor. Perhaps the disconnect here is that your mental model is that WebCompositor is some impl-thread concept? I'm fine tweaking API / doc / impl once we settle on something.
Yeah, I think this is a mental model mismatch. In my head WebCompositor is the interface that the embedder can use to communicate directly with the compositor's impl thread. The initialization routine is used to set the impl thread, fromIdentifier() is used to grab an interface to the impl thread, and the instance functions are used to poke at the thread. The normal WebLayer... interfaces are used to communicate with the 'main thread' data structures (when using that API). If a separate compositor thread is not being used then I had envisioned that the WebCompositor interface wouldn't be used at all. Perhaps we need to split out the impl thread interface from common initialization/shutdown routines? Then the API would be in three parts: 1.) General compositor initialization and shutdown. Apply to all compositor instances in the process and both threaded and single threaded uses. 2.) Per-compositor instance APIs - WebLayer, etc. Always use the 'main thread', whatever that means for a given configuration. 2.) Per-compositor instance impl-thread specific APIs, like input handling. Does that make sense?
Recapping our discussion, here's where I think we'll try to end up: > The API would be in three parts: > 1.) General compositor initialization and shutdown. Apply to all compositor instances in the process and both threaded and single threaded uses WebCompositor > 2.) Per-compositor instance APIs - WebLayer, etc. Always use the 'main thread', whatever that means for a given configuration. WebLayer*, WebLayerTreeView CCLayerTreeHost gets renamed to CCLayerTreeView > 2.) Per-compositor instance impl-thread specific APIs, like input handling. The aspects of WebCompositor that do this move out to WebCompositorInputHandler
*thumbsup*
Comment on attachment 115946 [details] Patch WebKit API LGTM + jamesr: "thumbs up" == R+
Comment on attachment 115946 [details] Patch whoops, /me misunderstood the thumbsup
Comment on attachment 115946 [details] Patch Sorry, that was thumbs up on the plan. I think that plan implies some changes to this patch - set r? if you disagree, Nat.
Trying to pull together a new patch that is worthy of thumbs. ;)
Created attachment 116178 [details] Patch
Comment on attachment 116178 [details] Patch Attachment 116178 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10551100
Comment on attachment 116178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116178&action=review Require additional pylons! I mean comments! R=me but please go through comments carefully before landing. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:42 > +namespace { > +static int numLayerTreeInstances; > +} anonymous namespace and static is redundant, normally we just do one or the other. could (should) this be #ifndef NDEBUG or ASSERT_ENABLED (or whatever it's called)? I think it'd be pretty bad if people started basing logic beyond ASSERT()s on this mechanism > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:112 > + // Returns true if any CCLayerTreeHost is alive. > + static bool anyLayerTreeHostInstanceExists(); > + #ifndef NDEBUG? I don't think this is normal 'public' API that we want to support outside of checking things > Source/WebKit/chromium/public/WebCompositor.h:31 > +#include "WebCompositorInputHandler.h" > namespace WebKit { newline between #include and namespace declaration > Source/WebKit/chromium/public/WebCompositor.h:33 > class WebInputEvent; pretty sure you don't need this forward decl any more > Source/WebKit/chromium/public/WebCompositor.h:41 > +// All calls to the WebCompositor must be made from the main thread. 'main thread' is ill defined here. does that mean the WebKit main thread? I think not for browser compositing. I think at the API level this means that all calls must be made from the same thread that the WebLayer* APIs are used on, right? > Source/WebKit/chromium/public/WebCompositor.h:48 > + // Initializes the compositor. Threaded compositing is enabled by passing in > + // a non-null WebThread. No compositor classes or methods should be used second sentence is a little awkward since it uses the passive voice. how about: Pass a non-null WebThread* to enable threaded compositing, otherwise the compositor will run on the thread the WebLayer APIs are used on. or something like that > Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:33 > + // Callbacks invoked from the compositor thread. can you move this do be a class-level comment? > Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:36 > + // Exactly one of the following two callbacks will be invoked after every call to WebCompositor::handleInputEvent(): update comment for new class name > Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:38 > + // Called when the WebCompositor handled the input event and no further processing is required. update comment for new class name > Source/WebKit/chromium/public/WebCompositorInputHandlerClient.h:41 > + // Called when the WebCompositor did not handle the input event. If sendToWidget is true, the input event update comment for new class name > Source/WebKit/chromium/src/WebCompositorImpl.cpp:43 > +OwnPtr<CCThread> WebCompositorImpl::s_mainThread; > +OwnPtr<CCThread> WebCompositorImpl::s_implThread; i know i asked you to make these OwnPtr<> but now that I think about it doing this will create static destructors, which we generally try to avoid. I think raw ptrs would probably be better - if we shut down the process without calling WebCompositor::shutdown() then we don't want to attempt to destroy these > Source/WebKit/chromium/src/WebCompositorImpl.cpp:58 > +/*WebCompositor* WebCompositor::initialize(WebThread* implThread) > { > - ASSERT(CCProxy::isImplThread()); > > - if (!s_compositors) > - s_compositors = new HashSet<WebCompositorImpl*>; > - s_compositors->add(this); > } > - > -WebCompositorImpl::~WebCompositorImpl() > +*/ please don't check in commented-out code. if it's not needed, just kill it with fire > Source/WebKit/chromium/src/WebCompositorImpl.cpp:86 > + s_implThread.clear(); > + s_mainThread.clear(); > + CCProxy::setImplThread(0); > + CCProxy::setMainThread(0); nit: would it be safer to reverse the order of these calls, just in case CCProxy::... wanted to do something with the pointer? > Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1013 > + // clear m_updateCheckLayer so CCLayerTreeHost dies. Capitalize 'clear' please
Oh, and it's probably a good idea to make it compile too.
Committed r101440: <http://trac.webkit.org/changeset/101440>
Created attachment 117274 [details] Patch for landing
Committed r101576: <http://trac.webkit.org/changeset/101576>