[webkit-reviews] review denied: [Bug 89045] [chromium] LayerRendererChromium is not getting visibility messages in single threaded compositing mode. : [Attachment 147923] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Michal Mocny
<mmocny at chromium.org>'s request for review:
Bug 89045: [chromium] LayerRendererChromium is not getting visibility messages
in single threaded compositing mode.
https://bugs.webkit.org/show_bug.cgi?id=89045

Attachment 147923: Patch
https://bugs.webkit.org/attachment.cgi?id=147923&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147923&action=review


If I understand the situation before this patch (broken as it is) is:

Before: CCLayerTreeHost::setVisible() reduces the contents memory limit, then
tries to do a forced commit to delete contents textures either down to zero or
the preferred minimum limit (based on contextHasCachedFrontBuffer). The forced
commit doesn't work in the single threaded proxy currently but that aside it's
attempting to do the texture deletions in response to setVisible. 
CCLayerTreeHost::setVisible propagates a visibility bit to the impl side, which
calls into LRC::setVisible which drops render surface textures and calls
setVisibilityCHROMIUM()

After: Contents textures are deleted by the onGpuMemoryAllocationChanged
callback, visibility and framebuffer stuff works the same.

This is a deliberate change, right?  Are we sure that using the GpuMemory....
callback instead of setVisible() for contents textures is going to work
reliably?  If we have to merge a fix back, on which branches can we rely on
these callbacks?

Given this, why are you still calling setContentsMemoryAllocationLimitBytes()
inside CCLayerTreeHost::setVisible()? it seems like this value is just going to
be clobbered by the call from the GPU memory manager.  It makes me really
uneasy that you still have two systems trying to fight over that value in
asynchronous ways.  I think you need to pick one system - either the
CCLTH::setVisible() call or the gpu memory callback system - and have it be in
charge of setContentsMemoryAllocationLimitBytes().  There's even a FIXME in the
CCLTH::setVisible() indicating that some lines of code are just for some m20
merge.	It's m21 branch time already!

There's also _another_ set of ugly in CCLayerTreeHost::updateLayers() that has
to do with m_frameIsForDisplay and different systems fighting over visibility
state.	It feels like this needs to go away too.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:229
> +	   // Purge all memory without requiring a commit.

it's weird to touch all of these internals on m_layerRenderer here - why not
just call a function on LayerRendererChromium and have it do the right thing?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:232
> +	       m_layerRenderer->m_client->setSourceFrameCanBeDrawn(false);

poor Demeter :(

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.cpp:110
> +void TrackingTextureAllocator::deleteAllTextures()
> +{
> +    // Copy so as not to invalidate iteration
> +    std::map< unsigned, std::pair<IntSize, GC3Denum> > textures =
m_textures;
> +    for (std::map< unsigned, std::pair<IntSize, GC3Denum> >::iterator it =
textures.begin(); it != textures.end(); ++it)
> +	   deleteTexture(it->first, it->second.first, it->second.second);

doing a map walk + element-by-element delete seems unnecessary.  Why not
iterate over the list of texture IDs + call m_context->deleteTexture() then
zero out m_currentMemoryUseBytes ?

> Source/WebCore/platform/graphics/chromium/TrackingTextureAllocator.h:63
> +    std::map< unsigned, std::pair<IntSize, GC3Denum> > m_textures;

we don't use std::map<> in WebKit code - see HashMap.

I think all you really need is a set here, however (i.e. HashSet)

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:209
> +void CCThreadProxy::setVisible(bool visible)
> +{
> +    TRACE_EVENT0("cc", "CCThreadProxy::setVisible");
> +    CCProxy::implThread()->postTask(createCCThreadTask(this,
&CCThreadProxy::setVisibleOnImplThread, visible));
> +}

I think this needs to be blocking (i.e. use a completion, etc)