Bug 83382 – [Chromium] Refactor CCTextureUpdater to contain a raw pointer to a GraphicsContext3D.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

Makes it possible for the texture updater class to manage GC3D dependent resources (like query objects for throttling). Also replaces the CCTextureUpdater::update(count) function with updateAll() and updateAtMost(count) to avoid unnecessary complexity from having to deal with both use cases in one function.

[David Reveman]

Created attachment 136048 [details]
Patch

[James Robinson]

Comment on attachment 136048 [details]
Patch

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

Looks good. I'd like to hold a ref explicitly in the updater as well, just to be safe (we currently manage the lifetime rather carefully in CCThreadProxy, but somebody might not be so careful about lifetimes in a future refactor and I don't want to leave the possibility of a dangling pointer).

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.h:70
> +    GraphicsContext3D* m_context;

can this hold a RefPtr<> ? the lifetime should be maintained by the owner, but it's safer to keep a ref here as well IMO

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:506
> +    m_currentTextureUpdaterOnImplThread = adoptPtr(new CCTextureUpdater(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator(), m_layerTreeHostImpl->layerRenderer()->textureCopier()));

Hm, we reset this on every frame?

[Nat Duca]

This is awesome, thanks for pulling this out from the megapatch.

[David Reveman]

(In reply to comment #2)
> (From update of attachment 136048 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136048&action=review
> 
> Looks good. I'd like to hold a ref explicitly in the updater as well, just to be safe (we currently manage the lifetime rather carefully in CCThreadProxy, but somebody might not be so careful about lifetimes in a future refactor and I don't want to leave the possibility of a dangling pointer).
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.h:70
> > +    GraphicsContext3D* m_context;
> 
> can this hold a RefPtr<> ? the lifetime should be maintained by the owner, but it's safer to keep a ref here as well IMO

I agree. The problem is that the LayerRendererChromium owns the GC3D and currently doesn't provide anything but raw pointer access to it. The TextureCopier and the TextureAllocator take raw pointers but are owned by the LRC. I think the texture updater should be handled the same way. What makes this confusing right now is that the CCTextureUpdater has become this object that we use to pass TextureCopier and TextureAllocator pointers to updateCompositorResources() implementations. That need to be fixed.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:506
> > +    m_currentTextureUpdaterOnImplThread = adoptPtr(new CCTextureUpdater(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator(), m_layerTreeHostImpl->layerRenderer()->textureCopier()));
> 
> Hm, we reset this on every frame?

I averted from actually changing the lifetime of texture updater in this patch and instead keep that change in throttling patch. I can included that in this patch if you prefer that?

[James Robinson]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 136048 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136048&action=review
> > 
> > Looks good. I'd like to hold a ref explicitly in the updater as well, just to be safe (we currently manage the lifetime rather carefully in CCThreadProxy, but somebody might not be so careful about lifetimes in a future refactor and I don't want to leave the possibility of a dangling pointer).
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.h:70
> > > +    GraphicsContext3D* m_context;
> > 
> > can this hold a RefPtr<> ? the lifetime should be maintained by the owner, but it's safer to keep a ref here as well IMO
> 
> I agree. The problem is that the LayerRendererChromium owns the GC3D and currently doesn't provide anything but raw pointer access to it. The TextureCopier and the TextureAllocator take raw pointers but are owned by the LRC. I think the texture updater should be handled the same way. What makes this confusing right now is that the CCTextureUpdater has become this object that we use to pass TextureCopier and TextureAllocator pointers to updateCompositorResources() implementations. That need to be fixed.

You can take a RefPtr<> from a raw pointer.

> 
> > 
> > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:506
> > > +    m_currentTextureUpdaterOnImplThread = adoptPtr(new CCTextureUpdater(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator(), m_layerTreeHostImpl->layerRenderer()->textureCopier()));
> > 
> > Hm, we reset this on every frame?
> 
> I averted from actually changing the lifetime of texture updater in this patch and instead keep that change in throttling patch. I can included that in this patch if you prefer that?

I think it's fine to leave as-is for now.

[David Reveman]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 136048 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=136048&action=review
> > > 
> > > Looks good. I'd like to hold a ref explicitly in the updater as well, just to be safe (we currently manage the lifetime rather carefully in CCThreadProxy, but somebody might not be so careful about lifetimes in a future refactor and I don't want to leave the possibility of a dangling pointer).
> > > 
> > > > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.h:70
> > > > +    GraphicsContext3D* m_context;
> > > 
> > > can this hold a RefPtr<> ? the lifetime should be maintained by the owner, but it's safer to keep a ref here as well IMO
> > 
> > I agree. The problem is that the LayerRendererChromium owns the GC3D and currently doesn't provide anything but raw pointer access to it. The TextureCopier and the TextureAllocator take raw pointers but are owned by the LRC. I think the texture updater should be handled the same way. What makes this confusing right now is that the CCTextureUpdater has become this object that we use to pass TextureCopier and TextureAllocator pointers to updateCompositorResources() implementations. That need to be fixed.
> 
> You can take a RefPtr<> from a raw pointer.

Ok, I could do that but wouldn't it be more appropriate to have a LRC function that returned a PassRefPtr<GraphicsContext3D>? It seems inappropriate to take a RefPtr<> when LRC doesn't provide an explicit way to give you one. And we also don't seem to have any GC3D usage on the impl thread that actually rely on it being a reference counted object so I'm a bit uneasy introducing that unless necessary. Should I be less concerned?

[James Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #2)
> > > > (From update of attachment 136048 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=136048&action=review
> > > > 
> > > > Looks good. I'd like to hold a ref explicitly in the updater as well, just to be safe (we currently manage the lifetime rather carefully in CCThreadProxy, but somebody might not be so careful about lifetimes in a future refactor and I don't want to leave the possibility of a dangling pointer).
> > > > 
> > > > > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.h:70
> > > > > +    GraphicsContext3D* m_context;
> > > > 
> > > > can this hold a RefPtr<> ? the lifetime should be maintained by the owner, but it's safer to keep a ref here as well IMO
> > > 
> > > I agree. The problem is that the LayerRendererChromium owns the GC3D and currently doesn't provide anything but raw pointer access to it. The TextureCopier and the TextureAllocator take raw pointers but are owned by the LRC. I think the texture updater should be handled the same way. What makes this confusing right now is that the CCTextureUpdater has become this object that we use to pass TextureCopier and TextureAllocator pointers to updateCompositorResources() implementations. That need to be fixed.
> > 
> > You can take a RefPtr<> from a raw pointer.
> 
> Ok, I could do that but wouldn't it be more appropriate to have a LRC function that returned a PassRefPtr<GraphicsContext3D>?

No, PassRefPtr<> return type is used when the caller *has* to take a reference to the object before using it.  It's fine for LRC to expose a getter that a user can use without having to reference themselves if they don't retain it.  If a caller does want to retain, they can.

> It seems inappropriate to take a RefPtr<> when LRC doesn't provide an explicit way to give you one. And we also don't seem to have any GC3D usage on the impl thread that actually rely on it being a reference counted object so I'm a bit uneasy introducing that unless necessary. Should I be less concerned?

What specifically are you worried about?

[David Reveman]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #2)
> > > > > (From update of attachment 136048 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=136048&action=review
> > > > > 
> > > > > Looks good. I'd like to hold a ref explicitly in the updater as well, just to be safe (we currently manage the lifetime rather carefully in CCThreadProxy, but somebody might not be so careful about lifetimes in a future refactor and I don't want to leave the possibility of a dangling pointer).
> > > > > 
> > > > > > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.h:70
> > > > > > +    GraphicsContext3D* m_context;
> > > > > 
> > > > > can this hold a RefPtr<> ? the lifetime should be maintained by the owner, but it's safer to keep a ref here as well IMO
> > > > 
> > > > I agree. The problem is that the LayerRendererChromium owns the GC3D and currently doesn't provide anything but raw pointer access to it. The TextureCopier and the TextureAllocator take raw pointers but are owned by the LRC. I think the texture updater should be handled the same way. What makes this confusing right now is that the CCTextureUpdater has become this object that we use to pass TextureCopier and TextureAllocator pointers to updateCompositorResources() implementations. That need to be fixed.
> > > 
> > > You can take a RefPtr<> from a raw pointer.
> > 
> > Ok, I could do that but wouldn't it be more appropriate to have a LRC function that returned a PassRefPtr<GraphicsContext3D>?
> 
> No, PassRefPtr<> return type is used when the caller *has* to take a reference to the object before using it.  It's fine for LRC to expose a getter that a user can use without having to reference themselves if they don't retain it.  If a caller does want to retain, they can.

Ok.

> 
> > It seems inappropriate to take a RefPtr<> when LRC doesn't provide an explicit way to give you one. And we also don't seem to have any GC3D usage on the impl thread that actually rely on it being a reference counted object so I'm a bit uneasy introducing that unless necessary. Should I be less concerned?
> 
> What specifically are you worried about?

I just prefer the OwnPtr usage and think it's nice if we can use that when possible. This change would break away from that in the case of GC3D usage on the impl thread unless there's some existing case that I'm not aware of.

[Nat Duca]

Did we get closure here? <peptalk>Lets get this landed.</pep>

[James Robinson]

Turns out we don't need it at all.