[chromium] Set default memory allocation limit bytes when GL_CHROMIUM_gpu_memory_manager is not supported by graphics context.
Created attachment 145121 [details] Patch
Nat, I've gotten bit by the "graphics context callback are called on the wrong thread" issue here again. I got around the issue by introducing a new DebugScopedSetMainThreadFromEither which will set main thread and reset to whatever thread it was on during initialization, instead of forcing reset to impl. The real solution would be to make graphics context callbacks run on impl. An alternative workaround would be to remove all ASSERT(is*Thread) from the allocation plumbing. Please advise, thanks.
Comment on attachment 145121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145121&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:318 > + m_client->setContentsMemoryAllocationLimitBytes(TextureManager::highLimitBytes(viewportSize())); How about calling postSetContentsMemoryAllocationBlahballbahbah? Then you can assert impl thread... > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:144 > This smells
Comment on attachment 145121 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145121&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:318 >> + m_client->setContentsMemoryAllocationLimitBytes(TextureManager::highLimitBytes(viewportSize())); > > How about calling postSetContentsMemoryAllocationBlahballbahbah? Then you can assert impl thread... This is possible, but it seems so ugly that I want to make sure we are on the same page: - I would need to add two different postSetContentsMemoryAllocationBlahballbahbah to CCLayerTreeHostImpl -- postSetContentsMemoryAllocationOnImpl, used here (initialize) -- postSetContentsMemoryAllocationOnMainIfSingleThreadedOtherwiseOnImpl, used by wgc memory allocation callback - CCLTHImpl would forward over to two equivalent versions in CCProxy - Then finally CCSingleThreadProxy::postSetContentsMemoryAllocationOnMainIfSingleThreadedOtherwiseOnImpl will know its on the wrong thread, and can DebugScopedImplThread Another possible alternative is that LRC callback could test if we are running with threaded compositing and DebugScopedImplThread if not. This is a lot cleaner, but I am not sure if it is possible.
Have the single threaded memory allocation callback call through an apapter class that DebugSetsImplThread.
Created attachment 145795 [details] Patch
Comment on attachment 145795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145795&action=review does it make sense to have a unit test that verifies the allocation is non-zero in this scenario? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:208 > + if (!CCProxy::hasImplThread()) > + onGpuMemoryAllocationChangedOnMain(allocation); > + else > + onGpuMemoryAllocationChangedOnImpl(allocation); Why not just put a DebugScopedSetImplThread here and avoid the 2 extra functions?
(In reply to comment #7) > Why not just put a DebugScopedSetImplThread here and avoid the 2 extra functions? Oh I see, previous comments.. heh :) Yay bikesheds
Created attachment 145807 [details] Patch
I've changed this to use two adaptors so that we don't have to test the if statement inside the callback every time, just once during initialization. I've also added the unit test as Dana suggested, but unit tests are currently failing since some of the thread assertions have changed. Will fix that and post update.
Created attachment 145824 [details] Patch
Unittests were failing because LRCTest was not initializing WebCompositor, but thats all fixed up now.
Comment on attachment 145824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145824&action=review Looks like it needs a rebase, I like the separate SingleThread class, seems cleaner than the extra methods in the last patch to me. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:200 > + nit: extra whitespace
Created attachment 145835 [details] Patch
Comment on attachment 145835 [details] Patch Rebasing.
Comment on attachment 145835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145835&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67 > +#include "cc/CCSingleThreadProxy.h" Why this and not CCPRoxy? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 > +class LayerRendererGpuMemoryAllocationChangedCallbackAdapterSingleThread : public LayerRendererGpuMemoryAllocationChangedCallbackAdapter { Why not just make the existing adapter have a bool saying "m_bindToImplThread" that you set in the single threaded case? > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:129 > + WebKit::WebCompositor::initialize(0); Eep, whats this? > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:339 > +class NonGpuMemoryManagingFakeContext : public FakeWebGraphicsContext3D { ContextThatDoesNotSupportMemoryManagmentExtensions
Comment on attachment 145835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145835&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67 >> +#include "cc/CCSingleThreadProxy.h" > > Why this and not CCPRoxy? This was needed for ScopedSet*Thread. This is not the only place where CCSingleThreadProxy.h is included just for those, should I move them into CCProxy.h? >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:221 >> +class LayerRendererGpuMemoryAllocationChangedCallbackAdapterSingleThread : public LayerRendererGpuMemoryAllocationChangedCallbackAdapter { > > Why not just make the existing adapter have a bool saying "m_bindToImplThread" that you set in the single threaded case? Sure, I'll do that. >> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:129 >> + WebKit::WebCompositor::initialize(0); > > Eep, whats this? This initializes the main/impl threads and calls CCProxy::setMainThread (among other things) which you need before you can ASSERT(CCProxy::isMainThread()) etc. I copied this from the behaviour of other unittests: CCLayerTreeHostTest.cpp, Canvas2DLayerChromiumTest.cpp, etc etc >> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:339 >> +class NonGpuMemoryManagingFakeContext : public FakeWebGraphicsContext3D { > > ContextThatDoesNotSupportMemoryManagmentExtensions sure.
Comment on attachment 145835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145835&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67 >> +#include "cc/CCSingleThreadProxy.h" > > Why this and not CCPRoxy? Woah, why does LRC need to know about the proxy at all? LayerRendererChromium should call out to the embedder via CCRendererClient and the client implementation (normally CCLayerTreeHostImpl) should be dealing with any proxy interactions > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:347 > + if (CCProxy::hasImplThread()) LayerRendererChromium is only accessed from the thread owning the GC3D, so it never should have to worry about whether it's in threaded mode or not. Its client (CCLTHI) may have to make some threading decisions, but that's not LRC's concern.
Created attachment 145844 [details] Patch
(In reply to comment #2) > Nat, I've gotten bit by the "graphics context callback are called on the wrong thread" issue here again. > Mind expanding on what this issue is? Is it just that our debug thread tracking isn't set to "impl" when running GC3D callbacks, or are we actually running code on the wrong thread? In which mode do you have an issue?
Comment on attachment 145835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145835&action=review >>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:67 >>>> +#include "cc/CCSingleThreadProxy.h" >>> >>> Why this and not CCPRoxy? >> >> This was needed for ScopedSet*Thread. This is not the only place where CCSingleThreadProxy.h is included just for those, should I move them into CCProxy.h? > > Woah, why does LRC need to know about the proxy at all? LayerRendererChromium should call out to the embedder via CCRendererClient and the client implementation (normally CCLayerTreeHostImpl) should be dealing with any proxy interactions CCProxy has been used in LRC for a while now. Grepping the file, its only used to make a few decisions depending on CCProxy::hasImplThread and assert which thread we are on.
(In reply to comment #20) > (In reply to comment #2) > > Nat, I've gotten bit by the "graphics context callback are called on the wrong thread" issue here again. > > > > Mind expanding on what this issue is? Is it just that our debug thread tracking isn't set to "impl" when running GC3D callbacks, or are we actually running code on the wrong thread? In which mode do you have an issue? The former, our debug thread tracking isn't set to impl when running the callback.
Comment on attachment 145844 [details] Patch R=me. It's a bit sad to me that these ASSERT()s are getting in the way - they're supposed to just be helpful - but this seems like a pretty reasonable way to deal with them.
Comment on attachment 145844 [details] Patch Clearing flags on attachment: 145844 Committed r119554: <http://trac.webkit.org/changeset/119554>
All reviewed patches have been landed. Closing bug.