Bug 67440 – [chromium] Move contents texture manager from LayerRendererChromium to CCLayerTreeHost

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Adrienne Walker]

Currently the contents texture manager lives on the LayerRendererChromium, but is accessed by both threads.  It needs to be owned by the CCLayerTreeHost, so the main thread layers can access it indiscriminately.  This also means that there needs to be an explicit synchronous shutdown where the main thread can ask for the compositor thread's context in order to delete the textures that it owns.

[James Robinson]

Created attachment 106238 [details]
wip, crashes all the time not fully baked

[Nat Duca]

Comment on attachment 106238 [details]
wip, crashes all the time not fully baked

View in context: https://bugs.webkit.org/attachment.cgi?id=106238&action=review

Thanks James! I got stuck in testing land today :/

What's the distinction between setNeedsCommit and setNeedsCommitAndRedraw?

> Source/WebCore/platform/graphics/chromium/TextureManager.h:48
> +    // FIXME: Make these limit adjustable and give it a useful value.

If this were adjustable, which side would do the adjusting? Or is your vision that this would be state we access on both sides via a mutex/?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:103
> +void CCLayerTreeHost::deleteContentsTextures(GraphicsContext3D* context)

Awkward method name

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:106
> +    m_contentsTextureManager->unprotectAllTextures();

What about m_contentsTextureManager->deleteAllTextures(context)?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-230
> -    m_proxy->setNeedsCommitAndRedraw();

I think we still should propagate visibility to the impl side --- the scheduler is going to live on the impl side in CCThreadMode, and it needs visibility information to decide when to draw.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:252
> +        m_proxy->setNeedsCommit();

Why not just setNeedsCommitAndRedraw? The impl can ignore the redraw if it knows its invisible...

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:150
> +void CCSingleThreadProxy::setNeedsCommit()

You shouldn't need the flag? We've been trying to use the THREADED_COMPOSITING define only when there'd be a compilation error resulting from threaded usage. And, since this code is in CCSingleThreadProxy, you're guaranteed it wont get called during thread mode...

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:140
> +    if (m_commitPending)

A source of your crashes may be that the CCSingleThreadProxy can't reliably catch all setNeedsCommit usage. Most setNeedsComitAndRedraw calls bypass the CCLayerTreeHost and go up to the render_widget. When we finish the inversion

[James Robinson]

Thanks for the feedback.  This definitely isn't ready to go yet, I mostly posted it so that I could ponder about it over the weekend.

[James Robinson]

(In reply to comment #2)
> (From update of attachment 106238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106238&action=review
> 
> Thanks James! I got stuck in testing land today :/
> 
> What's the distinction between setNeedsCommit and setNeedsCommitAndRedraw?
> 

the problem is setNeedsCommitAndRedraw() right now in the single threaded proxy just does scheduleComposite(), which the caller will happily ignore for offscreen content.  What I really need here is a way to say "hey thread, i need access to your context" and I'm trying to reuse the commit mechanism to do so.  It's just interacting awkwardly with the visibility stuff right now.

> > Source/WebCore/platform/graphics/chromium/TextureManager.h:48
> > +    // FIXME: Make these limit adjustable and give it a useful value.
> 
> If this were adjustable, which side would do the adjusting? Or is your vision that this would be state we access on both sides via a mutex/?

At this point I don't know, I just blindly copied that FIXME.

I'll try to pick this up again tomorrow.  Too brain fried today.

[James Robinson]

Created attachment 106820 [details]
Patch

[Adrienne Walker]

Comment on attachment 106820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106820&action=review

So much awesome cleanup.  :)

I'm a little bit unsure about the setNeedsCommit() addition, but it sounds like you're working that out with nduca and I'll wait for an updated patch.

> Source/WebCore/platform/graphics/chromium/TextureManager.h:-97
> -#ifndef NDEBUG
> -    GraphicsContext3D* m_associatedContextDebugOnly;
> -#endif
> -

Why is this going away? I've had that assert trip in the past while refactoring.

[James Robinson]

(In reply to comment #6)
> (From update of attachment 106820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106820&action=review
> 
> So much awesome cleanup.  :)
> 
> I'm a little bit unsure about the setNeedsCommit() addition, but it sounds like you're working that out with nduca and I'll wait for an updated patch.
> 
> > Source/WebCore/platform/graphics/chromium/TextureManager.h:-97
> > -#ifndef NDEBUG
> > -    GraphicsContext3D* m_associatedContextDebugOnly;
> > -#endif
> > -
> 
> Why is this going away? I've had that assert trip in the past while refactoring.

This used to be set at creation time for the contents TextureManager, since the contents TextureManager used to be created in LayerRendererChromium's c'tor.  Now we create the contents TextureManager before we have a context, so I can't set it there.  I could set the pointer in every commit, but that doesn't really seem to be very beneficial since in theory the context _could_ be different and that's OK.  It might make more sense to stash this on every ManagedTexture to match up the contexts on ManagedTexture::bindTexture(), do you think that would be better?  Unfortunately texture destruction doesn't go through ManagedTexture so there's not an obvious way to match the contexts up there.

[Adrienne Walker]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 106820 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106820&action=review
> > 
> > So much awesome cleanup.  :)
> > 
> > I'm a little bit unsure about the setNeedsCommit() addition, but it sounds like you're working that out with nduca and I'll wait for an updated patch.
> > 
> > > Source/WebCore/platform/graphics/chromium/TextureManager.h:-97
> > > -#ifndef NDEBUG
> > > -    GraphicsContext3D* m_associatedContextDebugOnly;
> > > -#endif
> > > -
> > 
> > Why is this going away? I've had that assert trip in the past while refactoring.
> 
> This used to be set at creation time for the contents TextureManager, since the contents TextureManager used to be created in LayerRendererChromium's c'tor.  Now we create the contents TextureManager before we have a context, so I can't set it there.  I could set the pointer in every commit, but that doesn't really seem to be very beneficial since in theory the context _could_ be different and that's OK.  It might make more sense to stash this on every ManagedTexture to match up the contexts on ManagedTexture::bindTexture(), do you think that would be better?  Unfortunately texture destruction doesn't go through ManagedTexture so there's not an obvious way to match the contexts up there.

Yeah, maybe it'd be better to put the context on the ManagedTexture and add a ManagedTexture::deleteTexture() function as a pair to the bindTexture().

That's how I caught the VideoLayerChromium keeping around stale textures and not calling cleanupResources properly, so I'd love if this logic didn't go away.

[James Robinson]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > 
> > This used to be set at creation time for the contents TextureManager, since the contents TextureManager used to be created in LayerRendererChromium's c'tor.  Now we create the contents TextureManager before we have a context, so I can't set it there.  I could set the pointer in every commit, but that doesn't really seem to be very beneficial since in theory the context _could_ be different and that's OK.  It might make more sense to stash this on every ManagedTexture to match up the contexts on ManagedTexture::bindTexture(), do you think that would be better?  Unfortunately texture destruction doesn't go through ManagedTexture so there's not an obvious way to match the contexts up there.
> 
> Yeah, maybe it'd be better to put the context on the ManagedTexture and add a ManagedTexture::deleteTexture() function as a pair to the bindTexture().

That would imply making TextureManager aware of ManagedTexture (right now it just deals with tokens and texture IDs).  I'm worried about that adding another layer of back pointers we have to sort out...

> 
> That's how I caught the VideoLayerChromium keeping around stale textures and not calling cleanupResources properly, so I'd love if this logic didn't go away.

Did it show up as a delete being called after a context recreation?

How about this instead: in debug whenever the TextureManager allocates a texture, it stashes a pointer to the context in its TextureInfo.  The eviction list changes from just a list of texture ids to a list of texture id, context ptr pairs.  When the texture finally gets deleted the context pointer is compared.

One weakness with this (and the existing logic) is that pointer identity != object identity since a new GraphicsContext3D might get allocated at the same address as a previous incarnation, but it would provide some protection.  WDYT?

[James Robinson]

Hah, I added in the assertions and they trip on platform/chromium/compositing/lost-compositor-context*.  We're trying to delete textures from the previous context on the new one.

Good call, I guess we do need the assertions after all.

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #8)

> > Yeah, maybe it'd be better to put the context on the ManagedTexture and add a ManagedTexture::deleteTexture() function as a pair to the bindTexture().
> 
> That would imply making TextureManager aware of ManagedTexture (right now it just deals with tokens and texture IDs).  I'm worried about that adding another layer of back pointers we have to sort out...

Ack, good call.
 
> How about this instead: in debug whenever the TextureManager allocates a texture, it stashes a pointer to the context in its TextureInfo.  The eviction list changes from just a list of texture ids to a list of texture id, context ptr pairs.  When the texture finally gets deleted the context pointer is compared.
> 
> One weakness with this (and the existing logic) is that pointer identity != object identity since a new GraphicsContext3D might get allocated at the same address as a previous incarnation, but it would provide some protection.  WDYT?

That sounds quite reasonable to me.  Pointer aliasing was a problem even with the previous scheme.

[Nat Duca]

Any sense in casting away the type (eg cast it to void* to prevent newcomers from the code thinking that its usable?)

[James Robinson]

Created attachment 106929 [details]
Patch

[James Robinson]

Adding those assertions revealed a lot of issues with managed textures, both for content textures and render surfaces.  I added a new lost context test to hit the render surface case and fixed all the bugs that popped up.  Yay testing!

[James Robinson]

Created attachment 107066 [details]
Patch

[James Robinson]

Patch merged up to ToT, no other changes.

[Adrienne Walker]

Comment on attachment 107066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107066&action=review

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:163
> +#ifndef NDEBUG
> +            ASSERT(m_evictedTextures[i].allocatingContext == context);
> +#endif
> +            if (m_evictedTextures[i].textureId)
> +                GLC(context, context->deleteTexture(m_evictedTextures[i].textureId));

What about textures that have never been bound? Wouldn't they have a null context or am I missing something? If so, would it be better to just skip eviction entries for texture infos without an id/context and then just assert that the context and texture id are non-null here?

[James Robinson]

(In reply to comment #17)
> (From update of attachment 107066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107066&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TextureManager.cpp:163
> > +#ifndef NDEBUG
> > +            ASSERT(m_evictedTextures[i].allocatingContext == context);
> > +#endif
> > +            if (m_evictedTextures[i].textureId)
> > +                GLC(context, context->deleteTexture(m_evictedTextures[i].textureId));
> 
> What about textures that have never been bound? Wouldn't they have a null context or am I missing something? If so, would it be better to just skip eviction entries for texture infos without an id/context and then just assert that the context and texture id are non-null here?

They'll have a null context and a zero ID.  I'll switch the order of the assertion and the if check.

[James Robinson]

Created attachment 107069 [details]
Patch

[Adrienne Walker]

Comment on attachment 107069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107069&action=review

Ok, two more things and then I'm all out of nits.

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:164
> +#if !USE(THREADED_COMPOSITING)
> +    // Commit immediately
> +    {
> +        ScopedSetImplThread impl;
> +        m_layerTreeHostImpl->beginCommit();
> +        m_layerTreeHost->commitTo(m_layerTreeHostImpl.get());
> +        m_layerTreeHostImpl->commitComplete();
> +    }
> +#else
> +    // Single threaded only works with THREADED_COMPOSITING.
> +    CRASH();
> +#endif

Why does this need an #ifdef guard and a CRASH()? The choice of proxy is a runtime decision.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:242
> +void CCThreadProxy::setNeedsCommitOnCCThread()
> +{
> +    TRACE_EVENT("CCThreadProxy::setNeedsCommitOnCCThread", this, 0);
> +    ASSERT(isImplThread());
> +    ASSERT(m_layerTreeHostImpl);
> +    ASSERT_NOT_REACHED();
> +}

Just to check my own understanding, this is really just "// FIXME: not implemented yet", right?

[James Robinson]

(In reply to comment #20)
> (From update of attachment 107069 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107069&action=review
> 
> Ok, two more things and then I'm all out of nits.
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:164
> > +#if !USE(THREADED_COMPOSITING)
> > +    // Commit immediately
> > +    {
> > +        ScopedSetImplThread impl;
> > +        m_layerTreeHostImpl->beginCommit();
> > +        m_layerTreeHost->commitTo(m_layerTreeHostImpl.get());
> > +        m_layerTreeHostImpl->commitComplete();
> > +    }
> > +#else
> > +    // Single threaded only works with THREADED_COMPOSITING.
> > +    CRASH();
> > +#endif
> 
> Why does this need an #ifdef guard and a CRASH()? The choice of proxy is a runtime decision.

Oops, Nat told me this was wrong as well and I forgot to take care of it.  Will fix.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:242
> > +void CCThreadProxy::setNeedsCommitOnCCThread()
> > +{
> > +    TRACE_EVENT("CCThreadProxy::setNeedsCommitOnCCThread", this, 0);
> > +    ASSERT(isImplThread());
> > +    ASSERT(m_layerTreeHostImpl);
> > +    ASSERT_NOT_REACHED();
> > +}
> 
> Just to check my own understanding, this is really just "// FIXME: not implemented yet", right?

Indeed.  I'll write it that way.

[James Robinson]

Created attachment 107072 [details]
Patch

[Adrienne Walker]

Comment on attachment 107072 [details]
Patch

Thanks! Unofficial LGTM from me.

[Kenneth Russell]

Comment on attachment 107072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107072&action=review

After cursory look seems good to me; nice test case. rs=me

> Source/WebCore/platform/graphics/chromium/cc/CCCanvasLayerImpl.h:55
> +    virtual const char* layerTypeAsString() const { return "CanvasLayer"; }

Can we move the definition of virtuals into the .cpp file? Here and elsewhere.

[James Robinson]

That's definitely a good idea to do at this point, I'll do that before landing.

[James Robinson]

Committed r94975: <http://trac.webkit.org/changeset/94975>

[James Robinson]

Was reverted due to browser_tests failure: http://trac.webkit.org/changeset/95014

[James Robinson]

The problem on that test is that if context creation fails, the CCLayerTreeHost's m_contentsTextureManager will be null, but we try to call evictAndDeleteTextures() on it. This issue only shows up if this test page is run and GL initialization fails.  I can repro locally by passing --use-gl=egl on my linux box.

[James Robinson]

Created attachment 107196 [details]
added null checks for CCLayerTreeHost::m_contentsTextureManager

[Nat Duca]

Bawe. Added this type of thing to our test design doc :)

[Adrienne Walker]

Comment on attachment 107196 [details]
added null checks for CCLayerTreeHost::m_contentsTextureManager

That unofficially looks good to me.

[Kenneth Russell]

Comment on attachment 107196 [details]
added null checks for CCLayerTreeHost::m_contentsTextureManager

Looks OK. r/rs=me

[James Robinson]

Committed r95100: <http://trac.webkit.org/changeset/95100>