Bug 67440 - [chromium] Move contents texture manager from LayerRendererChromium to CCLayerTreeHost
Summary: [chromium] Move contents texture manager from LayerRendererChromium to CCLaye...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 67438 67984
Blocks: 66995 67417 67883
  Show dependency treegraph
 
Reported: 2011-09-01 14:55 PDT by Adrienne Walker
Modified: 2011-09-14 11:01 PDT (History)
7 users (show)

See Also:


Attachments
wip, crashes all the time not fully baked (22.45 KB, patch)
2011-09-02 19:34 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (54.97 KB, patch)
2011-09-08 17:14 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (60.02 KB, patch)
2011-09-09 15:21 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (60.02 KB, patch)
2011-09-12 11:15 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (60.04 KB, patch)
2011-09-12 11:52 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (60.00 KB, patch)
2011-09-12 12:05 PDT, James Robinson
no flags Details | Formatted Diff | Diff
added null checks for CCLayerTreeHost::m_contentsTextureManager (60.07 KB, patch)
2011-09-13 11:00 PDT, James Robinson
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2011-09-01 14:55:55 PDT
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.
Comment 1 James Robinson 2011-09-02 19:34:27 PDT
Created attachment 106238 [details]
wip, crashes all the time not fully baked
Comment 2 Nat Duca 2011-09-02 21:03:48 PDT
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
Comment 3 James Robinson 2011-09-05 22:39:06 PDT
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.
Comment 4 James Robinson 2011-09-07 18:27:17 PDT
(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.
Comment 5 James Robinson 2011-09-08 17:14:05 PDT
Created attachment 106820 [details]
Patch
Comment 6 Adrienne Walker 2011-09-08 17:42:30 PDT
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.
Comment 7 James Robinson 2011-09-08 17:51:29 PDT
(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.
Comment 8 Adrienne Walker 2011-09-08 17:56:29 PDT
(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.
Comment 9 James Robinson 2011-09-08 18:01:57 PDT
(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?
Comment 10 James Robinson 2011-09-08 18:18:09 PDT
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.
Comment 11 Adrienne Walker 2011-09-08 18:22:56 PDT
(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.
Comment 12 Nat Duca 2011-09-08 21:17:26 PDT
Any sense in casting away the type (eg cast it to void* to prevent newcomers from the code thinking that its usable?)
Comment 13 James Robinson 2011-09-09 15:21:09 PDT
Created attachment 106929 [details]
Patch
Comment 14 James Robinson 2011-09-09 15:22:24 PDT
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!
Comment 15 James Robinson 2011-09-12 11:15:31 PDT
Created attachment 107066 [details]
Patch
Comment 16 James Robinson 2011-09-12 11:16:59 PDT
Patch merged up to ToT, no other changes.
Comment 17 Adrienne Walker 2011-09-12 11:37:03 PDT
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?
Comment 18 James Robinson 2011-09-12 11:51:05 PDT
(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.
Comment 19 James Robinson 2011-09-12 11:52:08 PDT
Created attachment 107069 [details]
Patch
Comment 20 Adrienne Walker 2011-09-12 11:59:35 PDT
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?
Comment 21 James Robinson 2011-09-12 12:03:31 PDT
(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.
Comment 22 James Robinson 2011-09-12 12:05:33 PDT
Created attachment 107072 [details]
Patch
Comment 23 Adrienne Walker 2011-09-12 12:08:29 PDT
Comment on attachment 107072 [details]
Patch

Thanks! Unofficial LGTM from me.
Comment 24 Kenneth Russell 2011-09-12 12:13:43 PDT
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.
Comment 25 James Robinson 2011-09-12 12:51:15 PDT
That's definitely a good idea to do at this point, I'll do that before landing.
Comment 26 James Robinson 2011-09-12 13:29:19 PDT
Committed r94975: <http://trac.webkit.org/changeset/94975>
Comment 27 James Robinson 2011-09-13 10:14:50 PDT
Was reverted due to browser_tests failure: http://trac.webkit.org/changeset/95014
Comment 28 James Robinson 2011-09-13 10:49:37 PDT
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.
Comment 29 James Robinson 2011-09-13 11:00:21 PDT
Created attachment 107196 [details]
added null checks for CCLayerTreeHost::m_contentsTextureManager
Comment 30 Nat Duca 2011-09-13 23:29:50 PDT
Bawe. Added this type of thing to our test design doc :)
Comment 31 Adrienne Walker 2011-09-14 10:23:11 PDT
Comment on attachment 107196 [details]
added null checks for CCLayerTreeHost::m_contentsTextureManager

That unofficially looks good to me.
Comment 32 Kenneth Russell 2011-09-14 10:47:36 PDT
Comment on attachment 107196 [details]
added null checks for CCLayerTreeHost::m_contentsTextureManager

Looks OK. r/rs=me
Comment 33 James Robinson 2011-09-14 11:01:38 PDT
Committed r95100: <http://trac.webkit.org/changeset/95100>