Summary: | [Chromium] Move instantiation of texture uploader to LayerRendererChromium. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Reveman <reveman> | ||||||||||||||||
Component: | WebKit Misc. | Assignee: | David Reveman <reveman> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cc-bugs, danakj, enne, jamesr, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 85848, 86178 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
David Reveman
2012-05-08 10:06:18 PDT
Created attachment 140734 [details]
Patch
Created attachment 140736 [details]
Patch
Correct changelog entry
View in context: https://bugs.webkit.org/attachment.cgi?id=140734&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1402 > + m_unthrottledTextureUploader = UnthrottledTextureUploader::create(); > + m_throttledTextureUploader = ThrottledTextureUploader::create(m_context.get()); It's a little weird to me to have both of these exist simultaneously on LRC. Do you expect that this will ever not be determined statically? It doesn't look like LRC uses these members directly either. It seems like maybe the proxies should own their own uploader. (In reply to comment #3) > View in context: https://bugs.webkit.org/attachment.cgi?id=140734&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1402 > > + m_unthrottledTextureUploader = UnthrottledTextureUploader::create(); > > + m_throttledTextureUploader = ThrottledTextureUploader::create(m_context.get()); > > It's a little weird to me to have both of these exist simultaneously on LRC. Do you expect that this will ever not be determined statically? > > It doesn't look like LRC uses these members directly either. It seems like maybe the proxies should own their own uploader. The problem is that the uploaders need to be created with a GC3D reference. Existing objects that need a GC3D all live in LRC and I think that's where the uploaders belong as well. See this bug for some more context: https://bugs.webkit.org/show_bug.cgi?id=85848 (In reply to comment #4) > (In reply to comment #3) > > View in context: https://bugs.webkit.org/attachment.cgi?id=140734&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1402 > > > + m_unthrottledTextureUploader = UnthrottledTextureUploader::create(); > > > + m_throttledTextureUploader = ThrottledTextureUploader::create(m_context.get()); > > > > It's a little weird to me to have both of these exist simultaneously on LRC. Do you expect that this will ever not be determined statically? > > > > It doesn't look like LRC uses these members directly either. It seems like maybe the proxies should own their own uploader. > > The problem is that the uploaders need to be created with a GC3D reference. Existing objects that need a GC3D all live in LRC and I think that's where the uploaders belong as well. > > See this bug for some more context: https://bugs.webkit.org/show_bug.cgi?id=85848 I saw that, but I still think it's awkward to have them both simultaneously exist on LRC. I think I'd prefer a LayerRendererChromium::setTextureUploader(PassOwnPtr<TextureUploader>) that a proxy could call after initialization. Comment on attachment 140736 [details]
Patch
Yeah this just feels sloppy. Pick one, or pass a bit down to LRC telling it which sort of uploader to create.
The main reason I like this approach is that I think the texture uploaders, just like the texture copiers and texture allocators are LRC specific and the LRC should be in control of what implementations to use. E.g. a software rendering LRC should be able to use completely different implementations of these if it wants to. So in this sense I think of the addition of the throttled uploader as an extension to the LRC rather than a replacement of the existing uploader. Also, lets say we want to use the texture uploader from within LRC. We likely want to control the type of uploader used in that case so having both a throttled and unthrottled uploader available would then make a lot of sense. I find that passing an uploader to the LRC constructor or using setTextureUploader(PassOwnPtr<TextureUploader>) and then destroying it in LayerRendererChromium::cleanupSharedObjects really awkward. If we stick with the current approach, we should at least fix that. I agree with having ownership on the LRC to the extent that it's GL-specific (maybe we need to split that part out?). But no matter what we never need to have two simultaneous uploaders. Created attachment 140799 [details]
Patch
How about this as a compromise?
Comment on attachment 140799 [details]
Patch
Why don't we just wait until we can use the throttled uploader everywhere and then just unconditionally use that? I can't tell what this helps with given that this is still hardcoded to use the unthrottled uploader
(In reply to comment #10) > (From update of attachment 140799 [details]) > Why don't we just wait until we can use the throttled uploader everywhere and then just unconditionally use that? I can't tell what this helps with given that this is still hardcoded to use the unthrottled uploader It doesn't make sense to use the throttled uploader in single thread mode. When we can enable the throttled uploader, we will only enable it in CCThreadProxy. Comment on attachment 140799 [details]
Patch
How does the fake texture uploader fit into the "bool useThrottledTextureUploader" world?
(In reply to comment #12) > (From update of attachment 140799 [details]) > How does the fake texture uploader fit into the "bool useThrottledTextureUploader" world? The fake texture updater currently used for unit testing really behaves as a unthrottled texture updater. It just calls updateRect() and never does any blocking. So replacing it with useThrottledTextureUploader=1 is safe. FYI, my current thinking is that we should eventually remove the updateRect() function from LayerTextureUpdater::Texture when all the texture upload logic has been moved into our TextureUpload classes. Comment on attachment 140799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140799&action=review I like this a lot better than having two uploaders. Thanks. :) > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:215 > -PassOwnPtr<LayerRendererChromium> LayerRendererChromium::create(LayerRendererChromiumClient* client, PassRefPtr<GraphicsContext3D> context, PassOwnPtr<TextureUploader> uploader) > +PassOwnPtr<LayerRendererChromium> LayerRendererChromium::create(LayerRendererChromiumClient* client, PassRefPtr<GraphicsContext3D> context, bool useThrottledTextureUploader) Can you make this boolean an enum, e.g. {ThrottledUploader, UnthrottledUploader}? Non-named boolean args from callsites are really opaque. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1407 > + m_textureCopier = AcceleratedTextureCopier::create(m_context.get()); Duplicated line? Created attachment 141288 [details]
Patch
Add TextureUploaderOption enum and remove duplicate line
Comment on attachment 141288 [details]
Patch
Thanks. R=me.
Comment on attachment 141288 [details] Patch Rejecting attachment 141288 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: hunks FAILED -- saving rejects to file Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp.rej patching file Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp patching file Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp patching file Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adrienne W..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12663543 Created attachment 141317 [details]
Patch
Rebase for landing
Created attachment 141320 [details]
Patch
Add R+
Comment on attachment 141320 [details] Patch Clearing flags on attachment: 141320 Committed r116731: <http://trac.webkit.org/changeset/116731> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 86178 (In reply to comment #20) > (From update of attachment 141320 [details]) > Clearing flags on attachment: 141320 > > Committed r116731: <http://trac.webkit.org/changeset/116731> I rolled it out. http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/23855/steps/compile/logs/stdio third_party/WebKit/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:236:32:error: field is uninitialized when used here [-Werror,-Wuninitialized] , m_textureUploaderSetting(m_textureUploaderSetting) ^ (In reply to comment #23) > (In reply to comment #20) > > (From update of attachment 141320 [details] [details]) > > Clearing flags on attachment: 141320 > > > > Committed r116731: <http://trac.webkit.org/changeset/116731> Two tests crashed on Linux-Debug by r116731. platform/chromium/compositing/lost-compositor-context-permanently.html platform/chromium/compositing/webgl-loses-compositor-context.html crash log for DumpRenderTree (pid 11699): STDOUT: <empty> STDERR: gpu/command_buffer/client/gles2_implementation.cc(371): GPU_CHECK_EQ(0 (0), gles2_implementation_->use_count_(1)) failed. STDERR: DumpRenderTree: gpu/command_buffer/common/logging.cc:20: gpu::Logger::~Logger(): Assertion `false' failed. STDERR: [11699:11699:4182410782745:ERROR:process_util_posix.cc(143)] Received signal 6 STDERR: base::debug::StackTrace::StackTrace() [0x861696] STDERR: base::(anonymous namespace)::StackDumpSignalHandler() [0x8217c9] STDERR: 0x7f54a9548af0 STDERR: 0x7f54a9548a75 STDERR: 0x7f54a954c5c0 STDERR: 0x7f54a9541941 STDERR: gpu::Logger::~Logger() [0x236403b] STDERR: gpu::Logger::CheckEqual<>() [0x23656ab] STDERR: gpu::gles2::GLES2Implementation::SingleThreadChecker::SingleThreadChecker() [0x23695f6] STDERR: gpu::gles2::GLES2Implementation::Finish() [0x236c2dd] STDERR: gpu::gles2::GLES2Implementation::DeleteQueriesEXTHelper() [0x2378a9a] STDERR: gpu::gles2::GLES2Implementation::DeleteQueriesEXT() [0x1f89776] STDERR: webkit::gpu::WebGraphicsContext3DInProcessCommandBufferImpl::deleteQueryEXT() [0x1f73034] STDERR: WebCore::GraphicsContext3DPrivate::deleteQueryEXT() [0x50d3f2] STDERR: WebCore::Extensions3DChromium::deleteQueryEXT() [0x54e6c6] STDERR: WebCore::ThrottledTextureUploader::Query::~Query() [0x124906b] STDERR: WTF::deleteOwnedPtr<>() [0x1249be1] STDERR: WTF::OwnPtr<>::~OwnPtr() [0x124a0d1] STDERR: WTF::VectorDestructor<>::destruct() [0x124a4a1] STDERR: WTF::VectorTypeOperations<>::destruct() [0x124a283] STDERR: WTF::Deque<>::destroyAll() [0x1249f06] STDERR: WTF::Deque<>::~Deque() [0x12498de] STDERR: WebCore::ThrottledTextureUploader::~ThrottledTextureUploader() [0x12494a0] STDERR: WTF::deleteOwnedPtr<>() [0x12444a7] STDERR: WTF::OwnPtr<>::clear() [0x1243cc0] STDERR: WebCore::LayerRendererChromium::cleanupSharedObjects() [0x123d527] STDERR: WebCore::LayerRendererChromium::~LayerRendererChromium() [0x1234e15] STDERR: WTF::deleteOwnedPtr<>() [0x1199153] STDERR: WTF::OwnPtr<>::operator=() [0x11988c5] STDERR: WebCore::CCLayerTreeHostImpl::initializeLayerRenderer() [0x11949ad] STDERR: WebCore::CCSingleThreadProxy::recreateContext() [0x11afd43] STDERR: WebCore::CCLayerTreeHost::recreateContext() [0x1184b87] STDERR: WebCore::CCLayerTreeHost::compositeAndReadback() [0x11853b6] STDERR: WebKit::WebLayerTreeView::compositeAndReadback() [0x53238b] STDERR: WebKit::WebViewImpl::doPixelReadbackToCanvas() [0x4d2492] STDERR: WebKit::WebViewImpl::paint() [0x4d278d] STDERR: WebViewHost::paintRect() [0x48147f] STDERR: WebViewHost::paintInvalidatedRegion() [0x481706] STDERR: LayoutTestController::display() [0x44e47c] STDERR: CppBoundClass::MemberCallback<>::run() [0x45b070] STDERR: CppBoundClass::invoke() [0x42ba78] STDERR: CppNPObject::invoke() [0x42b157] STDERR: WebCore::npObjectInvokeImpl() [0x1339e89] STDERR: WebCore::npObjectMethodHandler() [0x133a004] STDERR: v8::internal::HandleApiCallHelper<>() [0xb2b004] STDERR: v8::internal::Builtin_Impl_HandleApiCall() [0xb25f78] STDERR: v8::internal::Builtin_HandleApiCall() [0xb25f49] STDERR: 0x2df2fd90618e crash log for DumpRenderTree (pid 15467): STDOUT: STDERR: gpu/command_buffer/client/gles2_implementation.cc(371): GPU_CHECK_EQ(0 (0), gles2_implementation_->use_count_(1)) failed. STDERR: DumpRenderTree: gpu/command_buffer/common/logging.cc:20: gpu::Logger::~Logger(): Assertion `false' failed. STDERR: [15467:15467:4182440541082:ERROR:process_util_posix.cc(143)] Received signal 6 STDERR: base::debug::StackTrace::StackTrace() [0x861696] STDERR: base::(anonymous namespace)::StackDumpSignalHandler() [0x8217c9] STDERR: 0x7fb270ffbaf0 STDERR: 0x7fb270ffba75 STDERR: 0x7fb270fff5c0 STDERR: 0x7fb270ff4941 STDERR: gpu::Logger::~Logger() [0x236403b] STDERR: gpu::Logger::CheckEqual<>() [0x23656ab] STDERR: gpu::gles2::GLES2Implementation::SingleThreadChecker::SingleThreadChecker() [0x23695f6] STDERR: gpu::gles2::GLES2Implementation::Finish() [0x236c2dd] STDERR: gpu::gles2::GLES2Implementation::DeleteQueriesEXTHelper() [0x2378a9a] STDERR: gpu::gles2::GLES2Implementation::DeleteQueriesEXT() [0x1f89776] STDERR: webkit::gpu::WebGraphicsContext3DInProcessCommandBufferImpl::deleteQueryEXT() [0x1f73034] STDERR: WebCore::GraphicsContext3DPrivate::deleteQueryEXT() [0x50d3f2] STDERR: WebCore::Extensions3DChromium::deleteQueryEXT() [0x54e6c6] STDERR: WebCore::ThrottledTextureUploader::Query::~Query() [0x124906b] STDERR: WTF::deleteOwnedPtr<>() [0x1249be1] STDERR: WTF::OwnPtr<>::~OwnPtr() [0x124a0d1] STDERR: WTF::VectorDestructor<>::destruct() [0x124a4a1] STDERR: WTF::VectorTypeOperations<>::destruct() [0x124a283] STDERR: WTF::Deque<>::destroyAll() [0x1249f06] STDERR: WTF::Deque<>::~Deque() [0x12498de] STDERR: WebCore::ThrottledTextureUploader::~ThrottledTextureUploader() [0x12494a0] STDERR: WTF::deleteOwnedPtr<>() [0x12444a7] STDERR: WTF::OwnPtr<>::clear() [0x1243cc0] STDERR: WebCore::LayerRendererChromium::cleanupSharedObjects() [0x123d527] STDERR: WebCore::LayerRendererChromium::~LayerRendererChromium() [0x1234e15] STDERR: WTF::deleteOwnedPtr<>() [0x1199153] STDERR: WTF::OwnPtr<>::operator=() [0x11988c5] STDERR: WebCore::CCLayerTreeHostImpl::initializeLayerRenderer() [0x11949ad] STDERR: WebCore::CCSingleThreadProxy::recreateContext() [0x11afd43] STDERR: WebCore::CCLayerTreeHost::recreateContext() [0x1184b87] STDERR: WebCore::CCLayerTreeHost::compositeAndReadback() [0x11853b6] STDERR: WebKit::WebLayerTreeView::compositeAndReadback() [0x53238b] STDERR: WebKit::WebViewImpl::doPixelReadbackToCanvas() [0x4d2492] STDERR: WebKit::WebViewImpl::paint() [0x4d278d] STDERR: WebViewHost::paintRect() [0x48147f] STDERR: WebViewHost::paintInvalidatedRegion() [0x481706] STDERR: TestShell::dump() [0x46cdb0] STDERR: TestShell::testFinished() [0x46b5ba] STDERR: LayoutTestController::completeNotifyDone() [0x44b038] STDERR: LayoutTestController::notifyDone() [0x44af95] STDERR: CppBoundClass::MemberCallback<>::run() [0x45b070] STDERR: CppBoundClass::invoke() [0x42ba78] STDERR: CppNPObject::invoke() [0x42b157] STDERR: WebCore::npObjectInvokeImpl() [0x1339e89] STDERR: WebCore::npObjectMethodHandler() [0x133a004] STDERR: v8::internal::HandleApiCallHelper<>() [0xb2b004] STDERR: v8::internal::Builtin_Impl_HandleApiCall() [0xb25f78] STDERR: v8::internal::Builtin_HandleApiCall() [0xb25f49] STDERR: 0x34c05120618e Created attachment 141420 [details]
Patch
Fix typo
Would you mind giving the diff from the previous version? (In reply to comment #26) > Would you mind giving the diff from the previous version? --- a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp +++ b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp @@ -233,7 +233,7 @@ LayerRendererChromium::LayerRendererChromium(LayerRendererCh , m_sharedGeometryQuad(FloatRect(-0.5f, -0.5f, 1.0f, 1.0f)) , m_isViewportChanged(false) , m_isFramebufferDiscarded(false) - , m_textureUploaderSetting(m_textureUploaderSetting) + , m_textureUploaderSetting(textureUploaderSetting) { } Comment on attachment 141420 [details]
Patch
Ok, let's try this again.
Comment on attachment 141420 [details] Patch Clearing flags on attachment: 141420 Committed r116779: <http://trac.webkit.org/changeset/116779> All reviewed patches have been landed. Closing bug. |