Bug 85893 – [Chromium] Move instantiation of texture uploader to LayerRendererChromium.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[David Reveman]

To clean up the initialization of texture uploaders, create both a throttled and unthrottled uploader in LayerRendererChromium and have the CCProxy make the choice of which one to use when creating a CCTextureUpdater instead of at context initialization time.

[David Reveman]

Created attachment 140734 [details]
Patch

[David Reveman]

Created attachment 140736 [details]
Patch

Correct changelog entry

[Adrienne Walker]

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.

[David Reveman]

(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

[Adrienne Walker]

(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.

[James Robinson]

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.

[David Reveman]

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.

[James Robinson]

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.

[David Reveman]

Created attachment 140799 [details]
Patch

How about this as a compromise?

[James Robinson]

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

[David Reveman]

(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.

[Adrienne Walker]

Comment on attachment 140799 [details]
Patch

How does the fake texture uploader fit into the "bool useThrottledTextureUploader" world?

[David Reveman]

(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.

[Adrienne Walker]

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?

[David Reveman]

Created attachment 141288 [details]
Patch

Add TextureUploaderOption enum and remove duplicate line

[Adrienne Walker]

Comment on attachment 141288 [details]
Patch

Thanks.  R=me.

[WebKit Review Bot]

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

[David Reveman]

Created attachment 141317 [details]
Patch

Rebase for landing

[David Reveman]

Created attachment 141320 [details]
Patch

Add R+

[WebKit Review Bot]

Comment on attachment 141320 [details]
Patch

Clearing flags on attachment: 141320

Committed r116731: <http://trac.webkit.org/changeset/116731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 86178

[Kent Tamura]

(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)
                               ^

[Kent Tamura]

(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

[David Reveman]

Created attachment 141420 [details]
Patch

Fix typo

[Dana Jansens]

Would you mind giving the diff from the previous version?

[David Reveman]

(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)
 {
 }

[Adrienne Walker]

Comment on attachment 141420 [details]
Patch

Ok, let's try this again.

[WebKit Review Bot]

Comment on attachment 141420 [details]
Patch

Clearing flags on attachment: 141420

Committed r116779: <http://trac.webkit.org/changeset/116779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.