RESOLVED FIXED 83382
[Chromium] Refactor CCTextureUpdater to contain a raw pointer to a GraphicsContext3D.
https://bugs.webkit.org/show_bug.cgi?id=83382
Summary [Chromium] Refactor CCTextureUpdater to contain a raw pointer to a GraphicsCo...
David Reveman
Reported 2012-04-06 12:01:32 PDT
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.
Attachments
Patch (47.85 KB, patch)
2012-04-06 12:04 PDT, David Reveman
jamesr: review+
David Reveman
Comment 1 2012-04-06 12:04:18 PDT
James Robinson
Comment 2 2012-04-06 13:10:16 PDT
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
Comment 3 2012-04-06 15:40:15 PDT
This is awesome, thanks for pulling this out from the megapatch.
David Reveman
Comment 4 2012-04-09 10:02:51 PDT
(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
Comment 5 2012-04-09 10:40:47 PDT
(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
Comment 6 2012-04-09 12:09:20 PDT
(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
Comment 7 2012-04-09 12:21:52 PDT
(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
Comment 8 2012-04-09 12:47:29 PDT
(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
Comment 9 2012-04-10 13:33:12 PDT
Did we get closure here? <peptalk>Lets get this landed.</pep>
James Robinson
Comment 10 2012-04-24 20:32:34 PDT
Turns out we don't need it at all.
Note You need to log in before you can comment on or make changes to this bug.